Opened 3 weeks ago

Closed 3 weeks ago

Last modified 13 days ago

#37075 closed Cleanup/optimization (fixed)

`OPTIONS["pool"]["check"]` cannot be set: the PostgreSQL backend always passes `check=` to `psycopg_pool.ConnectionPool`, then unpacks `**pool_options` on top, raising `TypeError`.

Reported by: Raoni Timo de Castro Cambiaghi Owned by: Raoni Timo de Castro Cambiaghi
Component: Database layer (models, ORM) Version: 5.2
Severity: Normal Keywords: postgresql psycopg pool
Cc: Raoni Timo de Castro Cambiaghi Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Reproduction

django/db/backends/postgresql/base.py (5.2.13) constructs the pool like this — see base.py L209-L215:

enable_checks = self.settings_dict["CONN_HEALTH_CHECKS"]
pool = ConnectionPool(
    kwargs=connect_kwargs,
    open=False,
    configure=self._configure_connection,
    check=ConnectionPool.check_connection if enable_checks else None,
    **pool_options,
)

If a user puts a "check" key in OPTIONS["pool"] to inject a custom validation callable — which is the documented way to extend psycopg_pool.ConnectionPool's liveness probe — the call collides at first cursor open:

TypeError: psycopg_pool.pool.ConnectionPool() got multiple values for keyword argument 'check'

CONN_HEALTH_CHECKS does not gate this. With True, Django passes check=ConnectionPool.check_connection; with False, Django passes check=None. Either way the keyword is set, so any check in pool_options collides.

Minimal settings.py:

def my_check(conn):
    conn.execute("SELECT 1").fetchone()

DATABASES = {
    "default": {
        "ENGINE": "django.db.backends.postgresql",
        "NAME": "...",
        "USER": "...",
        "PASSWORD": "...",
        "HOST": "...",
        "PORT": "5432",
        "CONN_HEALTH_CHECKS": False,  # also fails with True
        "OPTIONS": {
            "pool": {
                "min_size": 4,
                "max_size": 20,
                "check": my_check,
            },
        },
    }
}

Expected behaviour

A "check" callable provided in OPTIONS["pool"] should win over Django's default. The documented contract for OPTIONS["pool"] is that the dict is forwarded to psycopg_pool.ConnectionPool, and check is a public, documented ConnectionPool.__init__ parameter — see psycopg pool docs.

Use case / motivation

The driving use case is AWS Aurora !PostgreSQL writer-flip handling. After a planned or unplanned writer failover, existing TCP connections survive but are now bound to a host that has become read-only. The default liveness probe (SELECT 1) does not detect this — the connection is "alive" but any subsequent INSERT/UPDATE fails with cannot execute X in a read-only transaction. AWS's Fast failover with Aurora PostgreSQL guide recommends chaining a pg_is_in_recovery() check into pool validation — exactly what psycopg_pool's check callable parameter exists for. Django 5.x makes this impossible without subclassing DatabaseWrapper and overriding pool.

Proposed fix

Use setdefault so OPTIONS["pool"]["check"], when present, takes precedence:

enable_checks = self.settings_dict["CONN_HEALTH_CHECKS"]
pool_options.setdefault(
    "check", ConnectionPool.check_connection if enable_checks else None
)
pool = ConnectionPool(
    kwargs=connect_kwargs,
    open=False,
    configure=self._configure_connection,
    **pool_options,
)

This is fully backwards-compatible: users not setting OPTIONS["pool"]["check"] get exactly today's behaviour. Users who do set it get their callable. No new public surface — OPTIONS["pool"] is already documented as forwarding to ConnectionPool.

The same shape would make future custom callables (configure, reset) overridable too, though that's out of scope here — configure in particular needs more thought because Django chains its own connection configuration.

Versions affected

  • Django 5.0 onward (where pool support landed)
  • Confirmed on Django 5.2.13 with psycopg 3.3.3 / psycopg-pool 3.3.0

Happy to put up the patch + a regression test (assert that OPTIONS["pool"]["check"] is the actual callable used, with both CONN_HEALTH_CHECKS values) if the maintainers agree this is the right shape.

Change History (6)

comment:1 by Jacob Walls, 3 weeks ago

Owner: set to Raoni Timo de Castro Cambiaghi
Status: newassigned
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Makes sense. Thanks for offering a PR. (We probably want to unpack the provided kwargs into a copy before mutating.)

I guess we have the same situation with configure, but we don't have to address that now.

comment:2 by Raoni Timo de Castro Cambiaghi, 3 weeks ago

Has patch: set

comment:3 by Jacob Walls, 3 weeks ago

Triage Stage: AcceptedReady for checkin

Looks good -- only just suggest removing the release note.

comment:4 by Jacob Walls <jacobtylerwalls@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In 9f790ef:

Fixed #37075 -- Allowed overriding the PostgreSQL pool's "check" callable.

Setting "check" in OPTIONSpool previously raised TypeError because the
PostgreSQL backend always passed check= to ConnectionPool() and unpacked
pool_options on top, regardless of CONN_HEALTH_CHECKS. The user's callable
now takes precedence via setdefault(); pool_options is copied first to avoid
mutating the user's settings dict.

in reply to:  4 comment:5 by Raoni Timo de Castro Cambiaghi, 13 days ago

Replying to Jacob Walls <jacobtylerwalls@…>:

Any chance this is backported to 5.2.x? Thanks!

comment:6 by Jacob Walls, 13 days ago

No, we don't backport new features.

Note: See TracTickets for help on using tickets.
Back to Top