Add support for Django Connection pool #9953
Conversation
1ae8d1b to
02da622
Compare
auvipy
left a comment
There was a problem hiding this comment.
do this features work from django 4.2 to 6.0?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9953 +/- ##
=======================================
Coverage 78.67% 78.67%
=======================================
Files 153 153
Lines 19299 19304 +5
Branches 2211 2212 +1
=======================================
+ Hits 15184 15188 +4
- Misses 3816 3817 +1
Partials 299 299
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
auvipy
left a comment
There was a problem hiding this comment.
also the change needs test coverage
Connection pool works from Django 5.1+ only. But this Celery change is backward compatible and safe as it checks for
I've added some unit tests. Not sure if you want test some more special cases or direct |
|
thanks |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Django's connection pool functionality by ensuring that psycopg connection pools are properly closed when Celery closes database connections. The change prevents psycopg_pool.PoolTimeout errors that occur when connection pools are shared between multiple processes/workers.
Key changes:
- Modified
_close_database()to callclose_pool()on connections that support it (Django + psycopg[pool]) - Added suppression of
KeyErrorexceptions during pool closure (handles already-closed pools)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| celery/fixups/django.py | Added connection pool closing logic with error suppression in _close_database() |
| t/unit/fixups/test_django.py | Added three test cases covering pool closure, missing pool method, and error suppression |
auvipy
left a comment
There was a problem hiding this comment.
I think we should also document the new pool feature and also point to django documentation for that and how this will work in celery
Did my best. Not sure about Changelog. Tell me if you need addition there as well. |
* Add support for Django Connection pool https://docs.djangoproject.com/en/dev/ref/databases/#postgresql-pool * tests for close_pool * close_pool called only if DB pool is enabled in Django settings * conn pool docs --------- Co-authored-by: Asif Saif Uddin {"Auvi":"অভি"} <auvipy@gmail.com>
|
we got this bug report, can you cross check please #10018 ? |
Hi, I've just hit error
psycopg_pool.PoolTimeout: couldn't get a connection after 30.0 secwhen using Django withpsycopg[pool]Connection pool. Like in psycopg/psycopg#544. It looks like Celery is sharing the pool between multiple processes/workers.This PR is adding explicit pool closing similar like in Django itself https://github.com/django/django/blob/6.0a1/django/db/backends/postgresql/creation.py#L61
I've been testing this PR for few hours on big project with multiple workers with no more connection issues.
For reference:
https://docs.djangoproject.com/en/dev/ref/databases/#postgresql-pool
https://www.psycopg.org/psycopg3/docs/advanced/pool.html