Repository navigation
feat: model pooler mode in ConnectionParams for PgBouncer + custom types - #158
Merged
Merged
Conversation
…pes (#157) Add a `pooler_mode` field to `ConnectionParams` so the plugin can adapt its behavior when the user declares the connection goes through a transaction-mode pooler (e.g. PgBouncer `pool_mode = transaction`). This closes the residual gap from tabularis#1097: rc.9 fixed the primary failure (named user-issued queries splitting across two Syncs), but tokio-postgres's internal `typeinfo_statement` — fired during Row deserialization for non-builtin column types (custom enums, composites, extension types) — still uses a named statement that can be orphaned by pooler reassignment. Plugin-side changes: - `ConnectionParams::pooler_mode` (Option<String>, "transaction"/"session"/ None) parsed from the host's params object. `is_transaction_pooler()` helper. No-op until the host sends the field (backward-compatible: defaults to None). - Gate the `prepare()` at `query.rs:451` (column discovery on empty result sets) — on a transaction pooler, skip it and return `columns: []`. This eliminates the plugin's own named-statement trigger. On direct connections, `prepare()` is still used (safe there, better column discovery). - `format_query_error` rewrites SQLSTATE 26000/42P05 (prepared-statement errors) with actionable guidance when on a transaction pooler: points the user to `max_prepared_statements > 0` in pgbouncer.ini, the official pooler-side mechanism for making named statements safe under transaction mode (PgBouncer 1.21+, 2023). For non-pooler connections or other errors, falls through to `format_pg_error` unchanged. The remaining trigger (tokio-postgres's internal `typeinfo_statement` during Row deserialization on the non-empty-rows path) can't be eliminated without patching tokio-postgres. The error-message rewrite makes that failure legible instead of cryptic, and `max_prepared_statements` handles it pooler-side — the division of responsibility the PostgreSQL ecosystem converged on (client uses unnamed statements where it can; pooler caches named statements it can't avoid). Companion: tabularis#1126 (host-side: add `pooler_mode` to ConnectionParams + expose in connection UI). Fixes #157.
aesslinger
added
the
prerelease:beta
aesslinger
added
enhancement
Four findings from the ensemble review (all 50-74 band, non-blocking):
1. test-coverage: the entire pooler_mode feature path was untested — all
test fixtures set pooler_mode: None, so is_transaction_pooler() always
returned false and the new branches (format_query_error rewrite, prepare()
skip) were exercised by zero tests. Added src/models_tests.rs with 7 unit
tests for is_transaction_pooler() covering: true for "transaction", false
for None/session/empty/typo/non-string-JSON, and case-insensitivity.
2. docs-vs-code-drift: comment at query.rs:476 said "error-message rewrite
below" but format_query_error is called at line 453 — above the comment,
not below. Fixed "below" → "above".
3. boundary-and-edge-cases: case-sensitive == Some("transaction") was a
fragile contract — a host-side capitalization change would silently
disable all pooler-safe behavior. Changed to eq_ignore_ascii_case for
case-insensitive matching. Added a test for "Transaction"/"TRANSACTION".
4. /code-review + error-handling: format_query_error duplicated
format_pg_error's as_db_error formatting inline (and the else branch was
unreachable dead code — SQLSTATE only exists on DbError). Replaced the
inline duplication with a call to format_pg_error(e), which produces the
same prefix string and avoids divergence if format_pg_error changes.
Note: format_query_error's SQLSTATE 26000/42P05 rewrite branch can't be unit
tested without a live DB — tokio_postgres::Error's constructors are all
pub(crate), so there's no public way to construct an Error with a specific
SQLSTATE. The non-pooler fall-through path is covered by existing tests.
The is_transaction_pooler() gate (which controls both the rewrite and the
prepare() skip) is now fully unit-tested.
aesslinger
added
prerelease:rc
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
1 similar comment
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Models pooler mode as a first-class connection property so the plugin can adapt its behavior when the user declares the connection goes through a transaction-mode pooler (e.g. PgBouncer
pool_mode = transaction). Closes the residual gap from tabularis#1097: rc.9 fixed the primary failure (named user-issued queries splitting across two Syncs), but tokio-postgres's internaltypeinfo_statement— fired duringRowdeserialization for non-builtin column types (custom enums, composites, extension types) — still uses a named statement that can be orphaned by pooler reassignment.Changes
ConnectionParams::pooler_mode(src/models.rs) —Option<String>("transaction"/"session"/None), parsed from the host's params object.is_transaction_pooler()helper. No-op until the host sends the field (backward-compatible: defaults to None, all existing tests pass unchanged).Gate
prepare()atquery.rs:451— on a transaction pooler, skipprepare()for empty-result-set column discovery and returncolumns: []. Eliminates the plugin's own named-statement trigger. On direct connections,prepare()is still used (safe there, better column discovery).format_query_error(src/client.rs) — rewrites SQLSTATE 26000/42P05 (prepared-statement errors) with actionable guidance when on a transaction pooler: points the user tomax_prepared_statements > 0in pgbouncer.ini, the official pooler-side mechanism for making named statements safe under transaction mode (PgBouncer 1.21+, 2023). For non-pooler connections or other errors, falls through toformat_pg_errorunchanged.Why this approach (PostgreSQL ecosystem perspective)
The PostgreSQL ecosystem converged on a two-part answer for prepared statements under transaction-mode poolers:
max_prepared_statements > 0for named statements that can't be eliminated from the driver (tokio-postgres's internaltypeinfo_statement).This PR does the client-side part (skip the plugin's own
prepare(), surface actionable errors) and points the user to the pooler-side part. The remaining trigger (tokio-postgres's internaltypeinfo_statementduringRowdeserialization) can't be eliminated without patching tokio-postgres —max_prepared_statementsis the designed solution for exactly this.What this does NOT do
simple_queryfallback (regresses to the simple protocol, loses typed results — wrong direction)SHOW server_versionthrough PgBouncer returns the backend version)max_prepared_statementspooler-side)Verification
cargo buildcargo clippy --all-targets -- -D warningscargo fmt --check--lib)live_db.rs)shutdown.rs)ConnectionParamstest fixtures pass withpooler_mode: NoneCompanion
pooler_modetoConnectionParamsand exposes it in the connection UI. Once that lands, the host sendspooler_mode: "transaction"and this PR's behavior lights up automatically. Until then, the field defaults to None and the plugin behaves exactly as before.References
max_prepared_statements: https://www.pgbouncer.org/config.html#max_prepared_statements🤖 Generated with Claude Code