Refs #33497 -- Added connection pool support for PostgreSQL. #17594

sarahboyce · 2023-12-11T10:44:19Z

@apollo13 has done all the hard work in this PR: #16881 hoping to push it over the line.

Also hope the tests are testing the right thing, tried to take inspiration from psycopg_pool tests: https://github.com/psycopg/psycopg/blob/52ed68a9d699c1e43e07a4d04441534197552de5/tests/pool/test_pool.py#L103

But let me know what I'm missing - very new space for me 👍

Related ticket: https://code.djangoproject.com/ticket/33497

tests/requirements/postgres.txt

apollo13 · 2023-12-11T11:39:08Z

Lovely, thank you for picking up my work.

django/db/backends/postgresql/base.py

bluetech

Thanks for working on this @sarahboyce. I left some minor comments, but it looks good to me.

bluetech · 2023-12-12T18:00:38Z

django/db/backends/postgresql/base.py

One subtlety with this trick that I checked is that pool.open is thread safe; luckily it is 👍

django/db/backends/postgresql/base.py

docs/ref/databases.txt

django/db/backends/postgresql/base.py

docs/ref/databases.txt

bluetech

Thanks for the update, I left a few more replies and comments.

django/db/backends/postgresql/base.py

tests/backends/postgresql/tests.py

django/db/backends/postgresql/base.py

apollo13 · 2023-12-13T21:33:49Z

Fwiw, while it is nice if the test suite runs with a pool enabled I don't think it is a requirement to get this merged. The test suite is rather special after all. Nevertheless it might also show us bugs in the implementation.

…

On Wed, Dec 13, 2023, at 22:26, Sarah Boyce wrote: ***@***.**** commented on this pull request. In tests/backends/postgresql/tests.py <#17594 (comment)>: > @@ -223,6 +223,82 @@ def test_connect_non_autocommit(self): finally: new_connection.close() + @unittest.skipUnless(is_psycopg3, "psycopg3 specific test") + def test_connect_pool(self): + from psycopg_pool import PoolTimeout + + new_connection = connection.copy() + self.assertIsNone(new_connection.pool) + + new_connection.settings_dict["OPTIONS"]["pool"] = { + "min_size": 2, + "timeout": 0.1, + } + new_connection._connection_pools = {} I investigated and `cache.tests.DBCacheWithTimeZoneTests` was failing because it was running `createcachetable` management command on test setUp (which then creates a database connection in the pool). If I add similar logic to `_close_all_connections` which I am running on on the test database cloning/creating methods, then this all passes. I think these issues are more around test state clean up which I guess I'm still trying to figure out. I can add tests around the timezone setting so I don't remove the logic again 😁 — Reply to this email directly, view it on GitHub <#17594 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAT5C3RKLYVTNGHM6G6C2TYJIMRLAVCNFSM6AAAAABAPRHCS6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOBQGUYTOMJRGY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

felixxm · 2024-01-12T09:34:45Z

I had the same warning for test_skip_class_unless_db_feature but I was able to fix it via sarahboyce@962566c -- this wouldn't hurt in current main either, @felixxm do you want an extra cleanup PR for that one or can we just keep it as extra commit in this PR?

Merged in 6e520d9.

ridersofrohan · 2024-01-31T15:45:25Z

Thanks for all the work on this! Is there any update on when it will be merged in and included in a Django Release?

apollo13 · 2024-01-31T15:47:46Z

No, we don't give any ETAs since we are doing this in our free time. If you want to get this in faster, please test it and review the code.

…

On Wed, Jan 31, 2024, at 16:45, Rohan Challa wrote: Thanks for all the work on this! Is there any update on when it will be merged in and included in a Django Release? — Reply to this email directly, view it on GitHub <#17594 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAT5C7FYRA6EPESGIZFDOTYRJRKHAVCNFSM6AAAAABAPRHCS6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJZGM3TKMJWG4>. You are receiving this because you were mentioned.Message ID: ***@***.***>

gerardrbentley · 2024-02-12T21:28:50Z

I'm finding this does the trick to limit PG connections when running under ASGI (in that not every request grabs a new connection). Awesome work; much less complicated than pg_bouncer for most needs

Here's my test repo using the patch branch if anyone wants to extend: https://github.com/gerardrbentley/demo-django

charettes · 2024-02-12T21:36:11Z

django/test/utils.py

Any reason why this can't be moved to the Postgres backend destroy_test_db method?

Nothing specific no, should be doable 👍

Got rid of it completely, couldn't find the reason why it was there :)

Oh I found it. When tests do fail it leaves the pool open. I wonder if we should add a hook to Django to prepare the base backend for something like close_pool

Doesn't this hook already exist in the form of destroy_test_db? Why can't it be moved there?

In other words, why can't destroy_test_db simply do

if hasattr(self.connection, "close_pool"): connection.close_pool()

For the time being

because there are more locations where I might wanna do some closing, like here:

django/django/db/backends/base/creation.py

Line 64 in d1be05b

self.connection.close()

But yes, for now I will probably add it inside destroy_test_db.

I made it postgres specific in _destroy_test_db for now.

yorickr · 2024-02-19T12:47:02Z

Hi!
First of all, thanks for all the work that's being done on this.

I'm looking to use this thread pool in the future, in combination with gevent. After some local testing, I found out the current solution opens a new threadpool on every new request, which for my use case is the same as using persistent connections.

Is this something that can/will be considered in the requirements? Is this threadpool even something that can be used with gevent in the first place?

Thank you 🙏

apollo13 · 2024-02-19T13:49:32Z

As far as I know Django doesn't support gevent officially. So no, this is not something I would consider, I have no idea how gevent works. That said if the existing thread locals do work, I am kinda surprised since I am not doing anything magic (I think).

bluetech · 2024-02-19T18:48:30Z

@yorickr

I'm looking to use this thread pool in the future, in combination with gevent. After some local testing, I found out the current solution opens a new threadpool on every new request, which for my use case is the same as using persistent connections.

I think gevent should work without any problem (and correctly) with this implementation, can you provide a way to reproduce what you're seeing?

django/db/backends/postgresql/base.py

felixxm · 2024-02-22T13:05:30Z

I started working on this patch .... 🚧

django/db/backends/postgresql/base.py

apollo13 · 2024-02-22T15:12:12Z

Fair enough:)

…

On Thu, Feb 22, 2024, at 15:31, Mariusz Felisiak wrote: ***@***.**** commented on this pull request. In django/db/backends/postgresql/base.py <#17594 (comment)>: > @@ -256,7 +255,9 @@ def get_database_version(self): def get_connection_params(self): settings_dict = self.settings_dict # None may be used to connect to the default 'postgres' db - if settings_dict["NAME"] == "" and not settings_dict["OPTIONS"].get("service"): + if settings_dict["NAME"] == "" and not settings_dict.get("OPTIONS", {}).get( Sure, maybe it's not needed. However, it has nothing to do with connection pooling, so we should merge it as a separate cleanup. — Reply to this email directly, view it on GitHub <#17594 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAT5C6BXZ6YMBD6SZ5VKGDYU5JCLAVCNFSM6AAAAABAPRHCS6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJWGAYDMMJXGE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

django/db/backends/base/base.py

tests/backends/postgresql/tests.py

django/db/backends/postgresql/base.py

felixxm · 2024-02-28T09:04:51Z

Superseded by #17914.

sarahboyce force-pushed the 33497_postgres_connection_pool branch 2 times, most recently from 2867c32 to e5884b8 Compare December 11, 2023 10:52

ngnpope suggested changes Dec 11, 2023

View reviewed changes

tests/requirements/postgres.txt Outdated Show resolved Hide resolved

sarahboyce force-pushed the 33497_postgres_connection_pool branch from e5884b8 to 96afd8c Compare December 11, 2023 11:12

sarahboyce force-pushed the 33497_postgres_connection_pool branch 2 times, most recently from 9933116 to bf048c2 Compare December 11, 2023 11:58

apollo13 reviewed Dec 11, 2023

View reviewed changes

django/db/backends/postgresql/base.py Outdated Show resolved Hide resolved

apollo13 reviewed Dec 11, 2023

View reviewed changes

django/db/backends/postgresql/base.py Outdated Show resolved Hide resolved

sarahboyce force-pushed the 33497_postgres_connection_pool branch from bf048c2 to 4af72c8 Compare December 11, 2023 12:52

bluetech reviewed Dec 12, 2023

View reviewed changes

sarahboyce force-pushed the 33497_postgres_connection_pool branch from 4af72c8 to 372ff92 Compare December 12, 2023 19:19

ngnpope reviewed Dec 12, 2023

View reviewed changes

docs/ref/databases.txt Outdated Show resolved Hide resolved

sarahboyce force-pushed the 33497_postgres_connection_pool branch 2 times, most recently from 0a24043 to 003b3e2 Compare December 12, 2023 20:35

ngnpope reviewed Dec 12, 2023

View reviewed changes

docs/ref/databases.txt Outdated Show resolved Hide resolved

sarahboyce force-pushed the 33497_postgres_connection_pool branch from 003b3e2 to 1c9a7fa Compare December 12, 2023 22:05

bluetech reviewed Dec 13, 2023

View reviewed changes

sarahboyce force-pushed the 33497_postgres_connection_pool branch 7 times, most recently from f975d56 to 14c65fd Compare December 13, 2023 21:13

sarahboyce force-pushed the 33497_postgres_connection_pool branch 4 times, most recently from 8a4741f to 6e7dbdc Compare December 14, 2023 12:59

nessita mentioned this pull request Jan 16, 2024

Fixed #7732 -- Added support for session pool #17743

Closed

charettes reviewed Feb 12, 2024

View reviewed changes

apollo13 force-pushed the 33497_postgres_connection_pool branch from fc7078b to 5b9263c Compare February 17, 2024 16:38

apollo13 force-pushed the 33497_postgres_connection_pool branch 2 times, most recently from d03886a to 74f2407 Compare February 19, 2024 18:35

felixxm self-assigned this Feb 22, 2024

felixxm reviewed Feb 22, 2024

View reviewed changes

django/db/backends/postgresql/base.py Outdated Show resolved Hide resolved

felixxm force-pushed the 33497_postgres_connection_pool branch from 74f2407 to 340e848 Compare February 22, 2024 13:04

apollo13 reviewed Feb 22, 2024

View reviewed changes

django/db/backends/postgresql/base.py Outdated Show resolved Hide resolved

felixxm reviewed Feb 23, 2024

View reviewed changes

django/db/backends/base/base.py Outdated Show resolved Hide resolved

felixxm reviewed Feb 23, 2024

View reviewed changes

tests/backends/postgresql/tests.py Outdated Show resolved Hide resolved

felixxm force-pushed the 33497_postgres_connection_pool branch 2 times, most recently from ab62fda to 856058e Compare February 23, 2024 12:25

felixxm reviewed Feb 26, 2024

View reviewed changes

django/db/backends/postgresql/base.py Outdated Show resolved Hide resolved

felixxm reviewed Feb 26, 2024

View reviewed changes

django/db/backends/postgresql/base.py Outdated Show resolved Hide resolved

felixxm force-pushed the 33497_postgres_connection_pool branch 3 times, most recently from 0d8ea9d to a691ea1 Compare February 28, 2024 08:57

felixxm closed this Feb 28, 2024

felixxm force-pushed the 33497_postgres_connection_pool branch from a691ea1 to 107aa76 Compare February 28, 2024 09:02

Uh oh!

Refs #33497 -- Added connection pool support for PostgreSQL. #17594

Refs #33497 -- Added connection pool support for PostgreSQL. #17594

Uh oh!

Conversation

sarahboyce commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

apollo13 commented Dec 11, 2023

Uh oh!

Uh oh!

Uh oh!

bluetech left a comment

Choose a reason for hiding this comment

Uh oh!

bluetech Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluetech left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apollo13 commented Dec 13, 2023 via email

Uh oh!

felixxm commented Jan 12, 2024

Uh oh!

ridersofrohan commented Jan 31, 2024

Uh oh!

apollo13 commented Jan 31, 2024 via email

Uh oh!

gerardrbentley commented Feb 12, 2024

Uh oh!

charettes Feb 12, 2024

Choose a reason for hiding this comment

Uh oh!

apollo13 Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

apollo13 Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

apollo13 Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

charettes Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apollo13 Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

apollo13 Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

yorickr commented Feb 19, 2024

Uh oh!

apollo13 commented Feb 19, 2024

Uh oh!

bluetech commented Feb 19, 2024

Uh oh!

Uh oh!

felixxm commented Feb 22, 2024

Uh oh!

Uh oh!

apollo13 commented Feb 22, 2024 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarahboyce commented Dec 11, 2023 •

edited

Loading

charettes Feb 17, 2024 •

edited

Loading