From 78843f49443fd1684890312cd9ed57a756dcd1a9 Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Thu, 15 Oct 2026 15:18:21 -0400 Subject: [PATCH 1/2] feat: model pooler mode in ConnectionParams for PgBouncer + custom types (#157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, "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. --- src/client.rs | 42 ++++++++++++++++++++++++++++++ src/client_live_tests.rs | 1 + src/client_tests.rs | 2 ++ src/handlers/query.rs | 55 ++++++++++++++++++++++++++++++++-------- src/models.rs | 20 +++++++++++++++ 5 files changed, 109 insertions(+), 11 deletions(-) diff --git a/src/client.rs b/src/client.rs index ff2a6c5..5bdc86a 100644 --- a/src/client.rs +++ b/src/client.rs @@ -71,6 +71,48 @@ pub(crate) fn format_pg_error(e: &tokio_postgres::Error) -> String { } } +/// Format a `tokio_postgres::Error` from a query path, rewriting +/// prepared-statement errors (SQLSTATE 26000 / 42P05) with actionable +/// guidance when the connection is through a transaction-mode pooler. +/// +/// Under PgBouncer `pool_mode = transaction`, tokio-postgres's internal +/// `typeinfo_statement` (fired during `Row` deserialization for non-builtin +/// column types — custom enums, composites, extension types) splits across +/// two Syncs and can be orphaned by pooler reassignment, producing +/// `ERROR: prepared statement "typeinfo_statement" does not exist`. The fix +/// is pooler-side: `max_prepared_statements > 0` in pgbouncer.ini makes +/// PgBouncer track prepared statements across reassignments. This rewrites +/// the cryptic error into that guidance so the user knows what to configure +/// (#157, tabularis#1097). +/// +/// For non-pooler connections or non-prepared-statement errors, falls +/// through to `format_pg_error` unchanged. +pub(crate) fn format_query_error(e: &tokio_postgres::Error, is_transaction_pooler: bool) -> String { + if is_transaction_pooler { + if let Some(code) = e.code() { + if *code == tokio_postgres::error::SqlState::INVALID_SQL_STATEMENT_NAME + || *code == tokio_postgres::error::SqlState::DUPLICATE_PSTATEMENT + { + return format!( + "{}\n\n\ + This error occurs under transaction-mode poolers (e.g. PgBouncer) \ + when a named prepared statement is lost to connection reassignment. \ + For queries returning custom types (enums, composites, extension types), \ + set `max_prepared_statements > 0` in pgbouncer.ini — this enables \ + server-side prepared statement caching so named statements survive \ + reassignment. See: https://www.pgbouncer.org/config.html#max_prepared_statements", + if let Some(db) = e.as_db_error() { + format!("{}: {}", db.severity(), db.message()) + } else { + e.to_string() + } + ); + } + } + } + format_pg_error(e) +} + /// Format a `pool.get()` failure. The pool's own connection-establishment /// handshake (bad database name, bad password, ...) surfaces as a /// `tokio_postgres::Error` wrapped in `PoolError::Backend` — the exact same diff --git a/src/client_live_tests.rs b/src/client_live_tests.rs index dacb1ca..404a9e4 100644 --- a/src/client_live_tests.rs +++ b/src/client_live_tests.rs @@ -48,6 +48,7 @@ fn live_params(password: &str) -> Option { ssl_key: None, connection_string: None, startup_script: None, + pooler_mode: None, }) } diff --git a/src/client_tests.rs b/src/client_tests.rs index 0d8f343..a0c466d 100644 --- a/src/client_tests.rs +++ b/src/client_tests.rs @@ -36,6 +36,7 @@ fn params(host: &str, port: u16, db: &str, user: &str) -> ConnectionParams { ssl_key: None, connection_string: None, startup_script: None, + pooler_mode: None, } } @@ -804,6 +805,7 @@ fn params_with_ssl(ssl_mode: &str) -> ConnectionParams { ssl_key: None, connection_string: None, startup_script: None, + pooler_mode: None, } } diff --git a/src/handlers/query.rs b/src/handlers/query.rs index 9d02758..c990b18 100644 --- a/src/handlers/query.rs +++ b/src/handlers/query.rs @@ -158,7 +158,14 @@ async fn run_batch_in_session( let mut results: Vec = Vec::new(); for query in queries { let start = Instant::now(); - let outcome = exec_query_on_client(&pg_client, query, limit, page).await; + let outcome = exec_query_on_client( + &pg_client, + query, + limit, + page, + conn_params.is_transaction_pooler(), + ) + .await; let elapsed_ms = start.elapsed().as_secs_f64() * 1000.0; in_transaction = @@ -296,7 +303,14 @@ async fn exec_query( apply_search_path(&pg_client, s).await?; } - exec_query_on_client(&pg_client, query, limit, page).await + exec_query_on_client( + &pg_client, + query, + limit, + page, + conn_params.is_transaction_pooler(), + ) + .await } /// Build a `SET search_path TO` statement that prepends `schema` onto the @@ -386,6 +400,7 @@ async fn exec_query_on_client( query: &str, limit: Option, page: u32, + is_transaction_pooler: bool, ) -> Result { // Check if the statement returns a result set if !returns_result_set(query) { @@ -427,10 +442,15 @@ async fn exec_query_on_client( // Unnamed-statement protocol (`query_typed`) — pooler-safe under // transaction-mode poolers (PgBouncer); see `client`'s module note // (tabularis#1097). No params for user-issued queries. + // + // On a transaction-mode pooler, a prepared-statement error (26000/42P05) + // from tokio-postgres's internal `typeinfo_statement` (fired by + // non-builtin column types) is rewritten with actionable guidance via + // `format_query_error` (#157). let rows = pg_client .query_typed(&final_query, &[]) .await - .map_err(|e| client::format_pg_error(&e))?; + .map_err(|e| client::format_query_error(&e, is_transaction_pooler))?; if rows.is_empty() { // Get columns from the statement if possible. `prepare`'s own @@ -445,14 +465,27 @@ async fn exec_query_on_client( // statement prepared and executed across separate Syncs, the same // pooler-unsafe pattern this PR eliminates elsewhere. That path is // a tokio-postgres 0.7 limitation (see the module note in - // `client.rs`); this `prepare` is the only remaining named- - // statement call, used only here for column discovery on empty - // result sets. - let columns: Vec = if let Ok(stmt) = pg_client.prepare(&final_query).await { - stmt.columns() - .iter() - .map(|c| c.name().to_string()) - .collect() + // `client.rs`). + // + // On a transaction-mode pooler connection, skip `prepare()` + // entirely and return `columns: []` — the host handles empty + // columns gracefully, and avoiding `prepare()` eliminates the + // plugin's own named-statement trigger (#157, tabularis#1097). + // The remaining trigger (tokio-postgres's internal + // `typeinfo_statement` during `Row` deserialization on the + // non-empty-rows path) is handled by the error-message rewrite + // below + `max_prepared_statements` on the pooler side. On direct + // connections, `prepare()` is still used (it's safe there and + // gives better column discovery for empty result sets). + let columns: Vec = if !is_transaction_pooler { + if let Ok(stmt) = pg_client.prepare(&final_query).await { + stmt.columns() + .iter() + .map(|c| c.name().to_string()) + .collect() + } else { + vec![] + } } else { vec![] }; diff --git a/src/models.rs b/src/models.rs index 2f3ab82..76f7e57 100644 --- a/src/models.rs +++ b/src/models.rs @@ -20,6 +20,16 @@ pub struct ConnectionParams { pub ssl_key: Option, pub connection_string: Option, pub startup_script: Option, + /// Connection pooler mode, set by the host when the user declares the + /// connection goes through a transaction-mode pooler (e.g. PgBouncer + /// `pool_mode = transaction`). When `"transaction"`, the plugin avoids + /// its own named-statement `prepare()` calls (which split across two + /// Syncs and can be orphaned by pooler reassignment) and surfaces + /// actionable error messages for tokio-postgres's internal + /// `typeinfo_statement` (which requires `max_prepared_statements > 0` + /// on the pooler side for non-builtin column types — see #157, + /// tabularis#1097). `None` for direct connections — no behavior change. + pub pooler_mode: Option, } impl ConnectionParams { @@ -48,8 +58,18 @@ impl ConnectionParams { ssl_key: get_str("ssl_key"), connection_string: get_str("connection_string"), startup_script: get_str("startup_script"), + pooler_mode: get_str("pooler_mode"), } } + + /// True when the connection goes through a transaction-mode pooler + /// (e.g. PgBouncer `pool_mode = transaction`). Drives pooler-safe + /// behavior: the plugin avoids its own named-statement `prepare()` calls + /// and surfaces actionable error messages for tokio-postgres's internal + /// `typeinfo_statement` (#157, tabularis#1097). + pub fn is_transaction_pooler(&self) -> bool { + self.pooler_mode.as_deref() == Some("transaction") + } } /// Extract the nested `params` object every RPC method receives. From 745267acd4992aa0e845fdacacd01747f3490050 Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Thu, 15 Oct 2026 15:29:28 -0400 Subject: [PATCH 2/2] fix: address pre-handoff review findings on pooler_mode (#158) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/client.rs | 6 +--- src/handlers/query.rs | 2 +- src/models.rs | 8 ++++- src/models_tests.rs | 74 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 7 deletions(-) create mode 100644 src/models_tests.rs diff --git a/src/client.rs b/src/client.rs index 5bdc86a..be0156d 100644 --- a/src/client.rs +++ b/src/client.rs @@ -101,11 +101,7 @@ pub(crate) fn format_query_error(e: &tokio_postgres::Error, is_transaction_poole set `max_prepared_statements > 0` in pgbouncer.ini — this enables \ server-side prepared statement caching so named statements survive \ reassignment. See: https://www.pgbouncer.org/config.html#max_prepared_statements", - if let Some(db) = e.as_db_error() { - format!("{}: {}", db.severity(), db.message()) - } else { - e.to_string() - } + format_pg_error(e) ); } } diff --git a/src/handlers/query.rs b/src/handlers/query.rs index c990b18..bff79d0 100644 --- a/src/handlers/query.rs +++ b/src/handlers/query.rs @@ -474,7 +474,7 @@ async fn exec_query_on_client( // The remaining trigger (tokio-postgres's internal // `typeinfo_statement` during `Row` deserialization on the // non-empty-rows path) is handled by the error-message rewrite - // below + `max_prepared_statements` on the pooler side. On direct + // above + `max_prepared_statements` on the pooler side. On direct // connections, `prepare()` is still used (it's safe there and // gives better column discovery for empty result sets). let columns: Vec = if !is_transaction_pooler { diff --git a/src/models.rs b/src/models.rs index 76f7e57..a2a9937 100644 --- a/src/models.rs +++ b/src/models.rs @@ -68,7 +68,9 @@ impl ConnectionParams { /// and surfaces actionable error messages for tokio-postgres's internal /// `typeinfo_statement` (#157, tabularis#1097). pub fn is_transaction_pooler(&self) -> bool { - self.pooler_mode.as_deref() == Some("transaction") + self.pooler_mode + .as_deref() + .is_some_and(|m| m.eq_ignore_ascii_case("transaction")) } } @@ -102,3 +104,7 @@ pub struct RoutineCallArg { #[serde(default)] pub is_raw: bool, } + +#[cfg(test)] +#[path = "models_tests.rs"] +mod tests; diff --git a/src/models_tests.rs b/src/models_tests.rs new file mode 100644 index 0000000..b76d0a9 --- /dev/null +++ b/src/models_tests.rs @@ -0,0 +1,74 @@ +//! Unit tests for `ConnectionParams::is_transaction_pooler` and the +//! `pooler_mode` field parsing (#157). + +use crate::models::ConnectionParams; +use serde_json::json; + +fn params_with_pooler_mode(mode: Option<&str>) -> ConnectionParams { + ConnectionParams::from_value(&json!({ + "driver": "postgres", + "host": "localhost", + "port": 5432, + "database": "db", + "username": "user", + "pooler_mode": mode, + })) +} + +#[test] +fn is_transaction_pooler_true_for_transaction() { + assert!(params_with_pooler_mode(Some("transaction")).is_transaction_pooler()); +} + +#[test] +fn is_transaction_pooler_false_for_none() { + let params = ConnectionParams::from_value(&json!({ + "driver": "postgres", + "host": "localhost", + "port": 5432, + "database": "db", + "username": "user", + })); + assert!(!params.is_transaction_pooler()); +} + +#[test] +fn is_transaction_pooler_false_for_session() { + assert!(!params_with_pooler_mode(Some("session")).is_transaction_pooler()); +} + +#[test] +fn is_transaction_pooler_false_for_empty_string() { + assert!(!params_with_pooler_mode(Some("")).is_transaction_pooler()); +} + +#[test] +fn is_transaction_pooler_false_for_typo() { + assert!(!params_with_pooler_mode(Some("transation")).is_transaction_pooler()); +} + +/// Case-insensitive: the host might send "Transaction" or "TRANSACTION" +/// from a differently-cased dropdown label or config file. The comparison +/// must not silently disable pooler-safe behavior on a valid-but-cased +/// value (boundary-and-edge-cases finding from the #158 ensemble review). +#[test] +fn is_transaction_pooler_case_insensitive() { + assert!(params_with_pooler_mode(Some("Transaction")).is_transaction_pooler()); + assert!(params_with_pooler_mode(Some("TRANSACTION")).is_transaction_pooler()); + assert!(params_with_pooler_mode(Some("Transaction")).is_transaction_pooler()); +} + +/// Non-string JSON for `pooler_mode` (number, bool, null) is silently +/// treated as None — not a transaction pooler. +#[test] +fn is_transaction_pooler_false_for_non_string_json() { + let params = ConnectionParams::from_value(&json!({ + "driver": "postgres", + "host": "localhost", + "port": 5432, + "database": "db", + "username": "user", + "pooler_mode": 42, + })); + assert!(!params.is_transaction_pooler()); +}