Skip to content

fix(ingest): disable psycopg prepared statements behind the pgbouncer pooler#1032

Merged
cbcoutinho merged 1 commit into
masterfrom
fix/procrastinate-prepare-threshold-pooler
Jul 5, 2026
Merged

fix(ingest): disable psycopg prepared statements behind the pgbouncer pooler#1032
cbcoutinho merged 1 commit into
masterfrom
fix/procrastinate-prepare-threshold-pooler

Conversation

@cbcoutinho

Copy link
Copy Markdown
Owner

Problem

After the Deck #424 rollout repointed tenant ingest workers at the shared-postgres PgBouncer pooler (transaction mode), the fast workers CrashLoopBackOff on:

psycopg_pool.PoolTimeout: pool initialization incomplete after 30.0 sec

Root cause (reproduced + validated from a live tenant pod)

psycopg3 auto-prepares statements as server-side NAMED statements (_pg3_0, _pg3_1… — a per-client counter). Under transaction pooling, many client connections share the same backend server connections, so two clients each create their own _pg3_0 on the same backend:

DuplicatePreparedStatement: prepared statement "_pg3_0" already exists
ProtocolViolation: bind message supplies 2 parameters, but prepared statement "_pg3_0" requires 1

Each error aborts the transaction. In transaction pooling, an aborted-but-open transaction never releases its server connection (it sits idle in transaction (aborted)), so the tenant's small server pool (default_pool_size=6 / max_db_connections=12) fills with stuck connections → a new worker's pool-init query can't get a server connection → PoolTimeout → crashloop.

This is why it works direct (each client owns its server connection; names never collide) but fails through the pooler. #424 correctly disabled LISTEN/NOTIFY for transaction pooling but missed prepared statements — the second thing psycopg needs behind pgbouncer transaction mode.

Reproduction from a pod (8 clients × 25 prepared execs):

Target Result
Pooler, auto-prepare on (current) 8/8 fail (DuplicatePreparedStatement)
Pooler, prepare_threshold=None 0 errors
Direct postgres 0 errors

Fix

Set prepare_threshold=None on the procrastinate PsycopgConnector (via the pool's per-connection kwargs), so psycopg uses unnamed statements — safe under transaction pooling. Applied unconditionally: it's harmless connecting direct (a negligible re-parse cost for this poll-heavy workload) and it removes the footgun of pointing a worker at a transaction-mode pooler without it.

  • vector/queue/procrastinate.py: new _psycopg_connector() helper used by both build_app_for_url() and get_procrastinate_app().
  • Tests assert both build paths pass kwargs={"prepare_threshold": None}.
  • ruff + ty + unit tests green.

Follow-up

Belt-and-suspenders: enable pgbouncer max_prepared_statements on shared-postgres-pooler (gitops) so named statements are safe pooler-wide even for other apps.

Deck: board 7 #538 (repo:nextcloud-mcp-server), follow-up to #424.


This PR was generated with the help of AI, and reviewed by a Human

… pooler

psycopg3 auto-prepares statements as server-side NAMED statements (`_pg3_N`, a
per-client counter). Behind the shared-postgres PgBouncer transaction-mode pooler
(Deck #424) many clients share backend server connections, so those names collide
(`DuplicatePreparedStatement`, or a param-count `ProtocolViolation`). Each aborts
its transaction; in transaction pooling an aborted-but-open transaction never
releases its server connection, so the tenant's small server pool
(default_pool_size=6 / max_db_connections=12) fills with stuck
`idle in transaction (aborted)` backends and a new worker's pool init then times
out (`psycopg_pool.PoolTimeout`) → CrashLoopBackOff.

Set `prepare_threshold=None` on the procrastinate PsycopgConnector (via the pool's
per-connection kwargs). Unconditional: harmless connecting direct (a negligible
re-parse cost for this poll-heavy workload) and it removes the footgun of pointing
a worker at a transaction-mode pooler without it — exactly how #424 regressed
(LISTEN/NOTIFY was disabled for the pooler, prepared statements were not).

Reproduced + validated from a live tenant pod (8 clients x25 prepared execs):
through the pooler 8/8 failed with DuplicatePreparedStatement; with
prepare_threshold=None → 0 errors; direct postgres → 0 errors.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Chris Coutinho <chris@coutinho.io>
@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

Claude finished @cbcoutinho's task in 2m 39s —— View job


Review complete

  • Gather context (read diff, changed files, CLAUDE.md conventions)
  • Review nextcloud_mcp_server/vector/queue/procrastinate.py changes
  • Review tests/unit/vector/test_procrastinate_producer.py changes
  • Check for bugs, security, performance, test coverage
  • Post final review

Note: ruff/ty/pytest could not be executed in this job (sandbox blocked uv run and other subprocess invocations) — this review is based on manual code reading. The PR description states these already pass locally; worth double-checking CI reflects that.


Summary

This is a well-targeted, minimal fix. The root-cause analysis (per-client _pg3_N named prepared statements colliding across shared backend connections under pgbouncer transaction pooling) matches the documented psycopg3 guidance for PgBouncer: setting prepare_threshold=None via the pool's kwargs is the officially recommended workaround, not a workaround invented for this repo. I verified:

  • PsycopgConnector(conninfo=..., kwargs={...}) forwards kwargs to psycopg_pool.AsyncConnectionPool, which forwards them to psycopg.AsyncConnection.connect(**kwargs) per new connection — prepare_threshold is a real connect() parameter, so this is wired correctly.
  • Both real construction sites (build_app_for_url, get_procrastinate_app) now go through the new _psycopg_connector() helper — grepped the repo and found no other PsycopgConnector(...) construction site that was missed.
  • get_procrastinate_conninfo() (config.py:1949) only produces the libpq conninfo string; it's orthogonal to the new kwargs dict, so there's no overlap/conflict between the two.

Code quality

  • nextcloud_mcp_server/vector/queue/procrastinate.py:504-520: The comment block above _psycopg_connector is long, but it explains a genuinely non-obvious production incident (aborted transactions never releasing pooled server connections) — consistent with this file's existing style of similarly detailed "why" comments elsewhere (e.g. lines 471-477, 482-485). No issue.
  • Applying the fix unconditionally (rather than gating on "are we behind a pooler") is the right call — simpler, and per the PR description the re-parse cost is negligible for this poll-heavy workload.

Test coverage

  • Both new tests (test_build_app_for_url_disables_psycopg_prepared_statements, test_get_procrastinate_app_disables_psycopg_prepared_statements) correctly patch pq.PsycopgConnector and assert kwargs={"prepare_threshold": None} was passed — this exercises exactly the regression path (a missed call site would fail these).
  • The singleton (pq._app) reset via monkeypatch.setattr(pq, "_app", None) before and after is consistent with this test file's existing patterns for module-level state and correctly prevents a mock-backed singleton leaking into other tests.

Bugs / issues found

None. No security or performance concerns — this only disables an auto-prepare optimization, it doesn't change query semantics, and the negligible per-query re-parse cost is explicitly called out and justified in the PR description.

Minor / optional

@sonarqubecloud

sonarqubecloud Bot commented Jul 5, 2026

Copy link
Copy Markdown

@cbcoutinho cbcoutinho merged commit 5d5ca02 into master Jul 5, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant