From ca72c40b257546d4af0edeb4fcf3d72e04a1e8e2 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Tue, 4 Aug 2026 17:27:40 +0300 Subject: [PATCH 01/18] Track a client's parameter changes on the server connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A SET or RESET the client sends once a server is attached changes that server's session, but we never wrote it down. What we did instead was clear the whole parameter cache whenever a CommandComplete said RESET, which trades one problem for another: the cache is what tells us to undo the change for the next client, and a ROLLBACK undoes the RESET anyway. pg_dump -t walks straight into it: SET search_path TO '', then RESET search_path inside its transaction, then ROLLBACK. Postgres brings the empty search_path back, the cache no longer mentions it, and the next client gets the connection with unqualified names silently broken. A SET has the same hole in the other direction: BEGIN, a query, SET statement_timeout, COMMIT — the setting stays on the server and nothing resets it for whoever comes next. So record the change where it happens. RESET now has the transaction handling SET always had (reset vs reset_transaction), so a rollback restores what it cleared and a commit makes it permanent, and the server connection keeps the same record its client does. The existing parameter diff then does the rest: the next client is handed a precise RESET for what it doesn't want, instead of a connection nobody dares reuse. The CommandComplete fallback stays for RESETs we don't see coming — with the query parser off, that is still all we have. --- pgdog/src/backend/pool/connection/binding.rs | 24 ++- pgdog/src/backend/server.rs | 202 +++++++++++++++++- pgdog/src/frontend/client/query_engine/set.rs | 15 +- pgdog/src/net/parameter.rs | 65 +++++- 4 files changed, 293 insertions(+), 13 deletions(-) diff --git a/pgdog/src/backend/pool/connection/binding.rs b/pgdog/src/backend/pool/connection/binding.rs index 4c32e8868..6e986d534 100644 --- a/pgdog/src/backend/pool/connection/binding.rs +++ b/pgdog/src/backend/pool/connection/binding.rs @@ -2,7 +2,7 @@ use crate::{ frontend::{ - ClientRequest, + ClientRequest, SetParam, client::query_engine::{ TwoPcPhase, two_pc::{TwoPcTransaction, statement::phase_control}, @@ -443,6 +443,28 @@ impl Binding { } } + /// Record a client parameter change on every server we hold. + pub fn record_params(&mut self, params: &[SetParam], in_transaction: bool) { + match self { + Binding::Direct(server, ..) => server.record_params(params, in_transaction), + Binding::MultiShard(servers, _) => servers + .iter_mut() + .for_each(|server| server.record_params(params, in_transaction)), + _ => (), + } + } + + /// Record a client `RESET ALL` on every server we hold. + pub fn record_reset_all(&mut self, in_transaction: bool) { + match self { + Binding::Direct(server, ..) => server.record_reset_all(in_transaction), + Binding::MultiShard(servers, _) => servers + .iter_mut() + .for_each(|server| server.record_reset_all(in_transaction)), + _ => (), + } + } + /// Handle transaction end. pub(crate) fn transaction_params_hook(&mut self, rollback: bool) { match self { diff --git a/pgdog/src/backend/server.rs b/pgdog/src/backend/server.rs index 08fa087ac..9213c9a74 100644 --- a/pgdog/src/backend/server.rs +++ b/pgdog/src/backend/server.rs @@ -21,7 +21,7 @@ use crate::{ auth::{md5, scram::Client}, backend::pool::stats::MemoryStats, config::AuthType, - frontend::ClientRequest, + frontend::{ClientRequest, SetParam}, net::{ Close, Liveness, MessageBuffer, Parameter, ProtocolMessage, Sync, messages::{ @@ -141,6 +141,9 @@ pub(crate) struct Server { streaming: bool, schema_changed: bool, sync_prepared: bool, + // The client's parameter change for the statement in flight is already + // recorded, so its CommandComplete tells us nothing we don't know. + params_recorded: bool, in_transaction: bool, re_synced: bool, replication_mode: bool, @@ -432,6 +435,7 @@ impl Server { streaming: false, schema_changed: false, sync_prepared: false, + params_recorded: false, in_transaction: false, statement_executed: false, re_synced: false, @@ -617,6 +621,8 @@ impl Server { match message.code() { 'Z' => { + self.params_recorded = false; + let now = Instant::now(); let rfq = ReadyForQuery::from_bytes(message.payload())?; @@ -677,7 +683,10 @@ impl Server { self.prepared_statements.clear(); self.client_params.clear(); } - "RESET" => self.client_params.clear(), // Someone reset params, we're gonna need to re-sync. + // Someone reset params. If we didn't see which ones (the query + // parser is off, or the RESET came from somewhere we don't + // track), the cache is worthless and we re-sync from scratch. + "RESET" if !self.params_recorded => self.client_params.clear(), _ => (), } self.stats.rows_affected(&cmd); @@ -772,6 +781,37 @@ impl Server { Ok(executed) } + /// Record a parameter change the client is making on this connection, so + /// we know what to undo before handing it to somebody else. + pub fn record_params(&mut self, params: &[SetParam], in_transaction: bool) { + for param in params { + match (¶m.value, in_transaction) { + (Some(value), true) => { + self.client_params + .insert_transaction(¶m.name, value.clone(), param.local); + } + (Some(value), false) => { + self.client_params.insert(¶m.name, value.clone()); + } + (None, true) => self.client_params.reset_transaction(¶m.name), + (None, false) => self.client_params.reset(¶m.name), + } + } + + self.params_recorded = true; + } + + /// Record a `RESET ALL` the client is making on this connection. + pub fn record_reset_all(&mut self, in_transaction: bool) { + if in_transaction { + self.client_params.reset_all_transaction(); + } else { + self.client_params.reset_all(); + } + + self.params_recorded = true; + } + // Handle COMMIT/ROLLBACK for in-transaction params tracking. pub(crate) fn transaction_params_hook(&mut self, rollback: bool) { if rollback { @@ -1327,7 +1367,7 @@ pub(crate) mod test { backend::pool::token_cache::TokenCache, config::Memory, frontend::{PreparedStatements, RewritePlan}, - net::{Prepare, *}, + net::{Prepare, parameter::ParameterValue, *}, }; use super::{Error, *}; @@ -1365,6 +1405,7 @@ pub(crate) mod test { streaming: false, schema_changed: false, sync_prepared: false, + params_recorded: false, in_transaction: false, re_synced: false, replication_mode: false, @@ -3150,6 +3191,161 @@ pub(crate) mod test { ) } + #[tokio::test] + async fn test_recorded_reset_survives_rollback() { + let mut server = test_server().await; + let mut params = Parameters::default(); + params.insert("search_path", ""); + server + .link_client(FrontendPid::new(), ¶ms, None) + .await + .unwrap(); + + server.execute("BEGIN").await.unwrap(); + server.record_params( + &[SetParam { + name: "search_path".into(), + value: None, + local: false, + }], + true, + ); + server.execute("RESET search_path").await.unwrap(); + server.execute("ROLLBACK").await.unwrap(); + server.transaction_params_hook(true); + + // The ROLLBACK brought search_path back, so we still owe the next + // client a RESET for it. + let queries = server + .client_params + .reset_queries(&Parameters::default()) + .into_iter() + .map(|query| query.query().to_string()) + .collect::>(); + assert_eq!(queries, vec![r#"RESET "search_path""#]); + } + + #[tokio::test] + async fn test_recorded_reset_committed_is_permanent() { + let mut server = test_server().await; + let mut params = Parameters::default(); + params.insert("search_path", ""); + server + .link_client(FrontendPid::new(), ¶ms, None) + .await + .unwrap(); + + server.execute("BEGIN").await.unwrap(); + server.record_params( + &[SetParam { + name: "search_path".into(), + value: None, + local: false, + }], + true, + ); + server.execute("RESET search_path").await.unwrap(); + server.execute("COMMIT").await.unwrap(); + server.transaction_params_hook(false); + + // Committed: the server really is back to its default, nothing to undo. + assert!( + server + .client_params + .reset_queries(&Parameters::default()) + .is_empty() + ); + } + + #[tokio::test] + async fn test_recorded_set_is_undone_for_the_next_client() { + let mut server = test_server().await; + server + .link_client(FrontendPid::new(), &Parameters::default(), None) + .await + .unwrap(); + + // A SET that lands after the connection is already ours. + server.record_params( + &[SetParam { + name: "statement_timeout".into(), + value: Some(ParameterValue::String("5s".into())), + local: false, + }], + false, + ); + server + .execute("SET statement_timeout TO '5s'") + .await + .unwrap(); + + let queries = server + .client_params + .reset_queries(&Parameters::default()) + .into_iter() + .map(|query| query.query().to_string()) + .collect::>(); + assert_eq!(queries, vec![r#"RESET "statement_timeout""#]); + } + + #[tokio::test] + async fn test_reset_keeps_other_recorded_params() { + let mut server = test_server().await; + server + .link_client(FrontendPid::new(), &Parameters::default(), None) + .await + .unwrap(); + + server.record_params( + &[SetParam { + name: "statement_timeout".into(), + value: Some(ParameterValue::String("5s".into())), + local: false, + }], + false, + ); + server + .execute("SET statement_timeout TO '5s'") + .await + .unwrap(); + + // Resetting one parameter says nothing about the others. + server.record_params( + &[SetParam { + name: "search_path".into(), + value: None, + local: false, + }], + false, + ); + server.execute("RESET search_path").await.unwrap(); + + let queries = server + .client_params + .reset_queries(&Parameters::default()) + .into_iter() + .map(|query| query.query().to_string()) + .collect::>(); + assert_eq!(queries, vec![r#"RESET "statement_timeout""#]); + } + + #[tokio::test] + async fn test_untracked_reset_still_clears_client_params() { + let mut server = test_server().await; + let mut params = Parameters::default(); + params.insert("search_path", "public"); + server + .link_client(FrontendPid::new(), ¶ms, None) + .await + .unwrap(); + + // Nobody told us what this RESET touched (query parser off), so the + // cache is worthless and we start over. + server.execute("RESET search_path").await.unwrap(); + + assert!(server.client_params.is_empty()); + } + #[tokio::test] async fn test_reset_clears_client_params() { let mut server = test_server().await; diff --git a/pgdog/src/frontend/client/query_engine/set.rs b/pgdog/src/frontend/client/query_engine/set.rs index 6bde6f2b7..622c94594 100644 --- a/pgdog/src/frontend/client/query_engine/set.rs +++ b/pgdog/src/frontend/client/query_engine/set.rs @@ -46,7 +46,11 @@ impl QueryEngine { } } else { fake_command = "RESET"; - context.params.reset(¶m.name); + if context.in_transaction() { + context.params.reset_transaction(¶m.name); + } else { + context.params.reset(¶m.name); + } if is_pin { self.manual_lock = false; } @@ -58,6 +62,10 @@ impl QueryEngine { } if self.backend.connected() { + // The server is ours right now, so its session changes with the + // client's. Record it, or we won't know to undo it for whoever + // gets this connection next. + self.backend.record_params(params, context.in_transaction()); self.execute(context, None).await?; } else { let fake_response = set_config @@ -99,7 +107,9 @@ impl QueryEngine { &mut self, context: &mut QueryEngineContext<'_>, ) -> Result<(), Error> { - if context.in_transaction() || self.backend.connected() { + if context.in_transaction() { + context.params.reset_all_transaction(); + } else if self.backend.connected() { context.params.reset_all(); } else { context.params.restore_startup(context.startup_params); @@ -107,6 +117,7 @@ impl QueryEngine { } if self.backend.connected() { + self.backend.record_reset_all(context.in_transaction()); self.execute(context, None).await?; } else { self.fake_command_response(context, "RESET", None).await?; diff --git a/pgdog/src/net/parameter.rs b/pgdog/src/net/parameter.rs index fc808f47e..5637f1228 100644 --- a/pgdog/src/net/parameter.rs +++ b/pgdog/src/net/parameter.rs @@ -252,6 +252,21 @@ impl Parameters { pub(crate) fn reset(&mut self, name: impl AsRef) { let name = name.as_ref().to_lowercase(); + if self.params.remove(&name).is_some() { + self.hash = Self::compute_hash(&self.params); + } + + self.transaction_params.remove(&name); + self.transaction_local_params.remove(&name); + // Nothing left to restore: the value is gone for good. + self.reset_params.remove(&name); + } + + /// Remove a parameter, but only for the duration of the transaction: + /// a ROLLBACK brings its value back. + pub(crate) fn reset_transaction(&mut self, name: impl AsRef) { + let name = name.as_ref().to_lowercase(); + if let Some(value) = self.params.remove(&name) { self.reset_params.insert(name.clone(), value); self.hash = Self::compute_hash(&self.params); @@ -271,17 +286,27 @@ impl Parameters { /// Reset all tracked parameters. pub(crate) fn reset_all(&mut self) { + for key in self.resettable_keys() { + self.reset(&key); + } + } + + /// Reset all tracked parameters for the duration of the transaction. + pub(crate) fn reset_all_transaction(&mut self) { + for key in self.resettable_keys() { + self.reset_transaction(&key); + } + } + + fn resettable_keys(&self) -> Vec { let mut keys: Vec = self.params.keys().cloned().collect(); keys.extend(self.transaction_params.keys().cloned()); keys.extend(self.transaction_local_params.keys().cloned()); keys.sort(); keys.dedup(); + keys.retain(|key| !UNTRACKED_PARAMS.contains(key)); - for key in keys { - if !UNTRACKED_PARAMS.contains(&key) { - self.reset(&key); - } - } + keys } /// Commit params we saved during the transaction. @@ -951,11 +976,37 @@ mod test { } #[test] - fn test_reset_rollback_restores_param() { + fn test_reset_outside_transaction_is_permanent() { let mut params = Parameters::default(); params.insert("search_path", "public"); params.reset("search_path"); + + // A transaction that comes later has nothing to do with that RESET. + params.rollback(); + + assert_eq!(params.get("search_path"), None); + } + + #[test] + fn test_reset_all_outside_transaction_is_permanent() { + let mut params = Parameters::default(); + params.insert("search_path", "public"); + params.insert("timezone", "UTC"); + + params.reset_all(); + params.rollback(); + + assert_eq!(params.get("search_path"), None); + assert_eq!(params.get("timezone"), None); + } + + #[test] + fn test_reset_rollback_restores_param() { + let mut params = Parameters::default(); + params.insert("search_path", "public"); + + params.reset_transaction("search_path"); assert_eq!(params.get("search_path"), None); params.rollback(); @@ -1057,7 +1108,7 @@ mod test { params.insert("search_path", "public"); params.insert("timezone", "UTC"); - params.reset_all(); + params.reset_all_transaction(); assert_eq!(params.get("search_path"), None); assert_eq!(params.get("timezone"), None); From 64d888854e8d9ce4781ff0b117c1edf9d17be150 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Tue, 4 Aug 2026 17:27:56 +0300 Subject: [PATCH 02/18] Add integration tests for session parameters leaking between clients Runs against a database with a single server connection, so the next client always gets the connection the previous one used. --- integration/pgdog.toml | 12 ++++ .../python/test_session_params_leak.py | 71 +++++++++++++++++++ integration/users.toml | 5 ++ 3 files changed, 88 insertions(+) create mode 100644 integration/python/test_session_params_leak.py diff --git a/integration/pgdog.toml b/integration/pgdog.toml index 429b7ffee..eea62761b 100644 --- a/integration/pgdog.toml +++ b/integration/pgdog.toml @@ -59,6 +59,18 @@ host = "127.0.0.1" role = "replica" read_only = true +# ------------------------------------------------------------------------------ +# ----- Database :: pgdog_leak ------------------------------------------------- +# Exactly one server connection, kept around: tests for session state leaking +# between clients need the next client to get the same connection back. + +[[databases]] +name = "pgdog_leak" +host = "127.0.0.1" +database_name = "pgdog" +pool_size = 1 +min_pool_size = 1 + # ------------------------------------------------------------------------------ # ----- Database :: pgdog_sharded ---------------------------------------------- diff --git a/integration/python/test_session_params_leak.py b/integration/python/test_session_params_leak.py new file mode 100644 index 000000000..74b98e68a --- /dev/null +++ b/integration/python/test_session_params_leak.py @@ -0,0 +1,71 @@ +"""Session parameters must not survive the client that set them. + +Runs against a database with a single server connection, so the next client +always gets the connection the previous one used. current_setting() is used +instead of SHOW because SHOW can be answered by PgDog itself. +""" + +import psycopg + + +def connect(): + conn = psycopg.connect( + user="pgdog", + password="pgdog", + dbname="pgdog_leak", + host="127.0.0.1", + port=6432, + ) + # Without autocommit every statement runs in a transaction that is rolled + # back on close, which would undo the very state we're testing for. + conn.autocommit = True + return conn + + +def read(setting): + conn = connect() + value = conn.execute(f"SELECT current_setting('{setting}')").fetchone()[0] + conn.close() + + return value + + +def test_reset_rolled_back(): + """A ROLLBACK brings back the value the RESET cleared. + + This is the sequence pg_dump -t
emits. SET search_path TO '' leaves + an empty quoted identifier, hence the two spellings of "empty". + """ + conn = connect() + conn.execute("SET search_path TO ''") + with conn.transaction(): + conn.execute("SET TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ ONLY") + conn.execute("RESET search_path") + conn.execute("SELECT 1") + raise psycopg.Rollback() + conn.close() + + assert read("search_path") not in ("", '""') + + +def test_set_committed_after_connecting(): + """A SET that lands once the connection is already ours.""" + conn = connect() + with conn.transaction(): + conn.execute("SELECT 1") + conn.execute("SET statement_timeout TO '5s'") + conn.close() + + assert read("statement_timeout") == "0" + + +def test_reset_committed(): + """A committed RESET is permanent and needs no undoing.""" + conn = connect() + conn.execute("SET search_path TO public") + with conn.transaction(): + conn.execute("SELECT 1") + conn.execute("RESET search_path") + conn.close() + + assert read("search_path") == '"$user", public' diff --git a/integration/users.toml b/integration/users.toml index bba115a85..360246b5b 100644 --- a/integration/users.toml +++ b/integration/users.toml @@ -3,6 +3,11 @@ name = "pgdog" database = "pgdog" password = "pgdog" +[[users]] +name = "pgdog" +database = "pgdog_leak" +password = "pgdog" + [[users]] name = "pgdog_migrator" database = "pgdog" From 134d7bc08b3746dcaa54ed1c4db7ecbaebeaa238 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Tue, 4 Aug 2026 23:34:55 +0300 Subject: [PATCH 03/18] Collect resettable keys in a single filtered pass The keys still have to be lifted out of the maps before we can reset them, but there is no reason to clone the ones we are about to drop. --- pgdog/src/net/parameter.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/pgdog/src/net/parameter.rs b/pgdog/src/net/parameter.rs index 5637f1228..99abb7540 100644 --- a/pgdog/src/net/parameter.rs +++ b/pgdog/src/net/parameter.rs @@ -299,12 +299,18 @@ impl Parameters { } fn resettable_keys(&self) -> Vec { - let mut keys: Vec = self.params.keys().cloned().collect(); - keys.extend(self.transaction_params.keys().cloned()); - keys.extend(self.transaction_local_params.keys().cloned()); + // The keys have to be lifted out before we can reset them: resetting + // borrows the maps we'd be iterating. + let mut keys: Vec = self + .params + .keys() + .chain(self.transaction_params.keys()) + .chain(self.transaction_local_params.keys()) + .filter(|key| !UNTRACKED_PARAMS.contains(key)) + .cloned() + .collect(); keys.sort(); keys.dedup(); - keys.retain(|key| !UNTRACKED_PARAMS.contains(key)); keys } From 80dfa9d673e6d9550f3eebb1701a470f354659c8 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Wed, 5 Aug 2026 00:32:24 +0300 Subject: [PATCH 04/18] Trim comments to what the code doesn't already say --- integration/pgdog.toml | 3 +-- .../python/test_session_params_leak.py | 4 ++-- pgdog/src/backend/server.rs | 23 +++++++------------ pgdog/src/frontend/client/query_engine/set.rs | 5 ++-- pgdog/src/net/parameter.rs | 15 +++++------- 5 files changed, 19 insertions(+), 31 deletions(-) diff --git a/integration/pgdog.toml b/integration/pgdog.toml index eea62761b..1f59a12be 100644 --- a/integration/pgdog.toml +++ b/integration/pgdog.toml @@ -61,8 +61,7 @@ read_only = true # ------------------------------------------------------------------------------ # ----- Database :: pgdog_leak ------------------------------------------------- -# Exactly one server connection, kept around: tests for session state leaking -# between clients need the next client to get the same connection back. +# One server connection, never reaped: the next client has to get the same one. [[databases]] name = "pgdog_leak" diff --git a/integration/python/test_session_params_leak.py b/integration/python/test_session_params_leak.py index 74b98e68a..9af9c4533 100644 --- a/integration/python/test_session_params_leak.py +++ b/integration/python/test_session_params_leak.py @@ -16,8 +16,8 @@ def connect(): host="127.0.0.1", port=6432, ) - # Without autocommit every statement runs in a transaction that is rolled - # back on close, which would undo the very state we're testing for. + # Otherwise psycopg wraps each statement in a transaction and rolls it + # back on close, undoing the state we're testing for. conn.autocommit = True return conn diff --git a/pgdog/src/backend/server.rs b/pgdog/src/backend/server.rs index 9213c9a74..0a9383060 100644 --- a/pgdog/src/backend/server.rs +++ b/pgdog/src/backend/server.rs @@ -141,8 +141,8 @@ pub(crate) struct Server { streaming: bool, schema_changed: bool, sync_prepared: bool, - // The client's parameter change for the statement in flight is already - // recorded, so its CommandComplete tells us nothing we don't know. + // The change in flight is already recorded, so its CommandComplete + // tells us nothing new. params_recorded: bool, in_transaction: bool, re_synced: bool, @@ -683,9 +683,8 @@ impl Server { self.prepared_statements.clear(); self.client_params.clear(); } - // Someone reset params. If we didn't see which ones (the query - // parser is off, or the RESET came from somewhere we don't - // track), the cache is worthless and we re-sync from scratch. + // A RESET nobody told us the contents of: we can't tell + // what it touched, so drop everything. "RESET" if !self.params_recorded => self.client_params.clear(), _ => (), } @@ -781,8 +780,8 @@ impl Server { Ok(executed) } - /// Record a parameter change the client is making on this connection, so - /// we know what to undo before handing it to somebody else. + /// Record a client's parameter change, so we know what to undo before + /// this connection goes to somebody else. pub fn record_params(&mut self, params: &[SetParam], in_transaction: bool) { for param in params { match (¶m.value, in_transaction) { @@ -801,7 +800,7 @@ impl Server { self.params_recorded = true; } - /// Record a `RESET ALL` the client is making on this connection. + /// Record a client's `RESET ALL`. pub fn record_reset_all(&mut self, in_transaction: bool) { if in_transaction { self.client_params.reset_all_transaction(); @@ -3214,8 +3213,6 @@ pub(crate) mod test { server.execute("ROLLBACK").await.unwrap(); server.transaction_params_hook(true); - // The ROLLBACK brought search_path back, so we still owe the next - // client a RESET for it. let queries = server .client_params .reset_queries(&Parameters::default()) @@ -3248,7 +3245,6 @@ pub(crate) mod test { server.execute("COMMIT").await.unwrap(); server.transaction_params_hook(false); - // Committed: the server really is back to its default, nothing to undo. assert!( server .client_params @@ -3265,7 +3261,6 @@ pub(crate) mod test { .await .unwrap(); - // A SET that lands after the connection is already ours. server.record_params( &[SetParam { name: "statement_timeout".into(), @@ -3309,7 +3304,6 @@ pub(crate) mod test { .await .unwrap(); - // Resetting one parameter says nothing about the others. server.record_params( &[SetParam { name: "search_path".into(), @@ -3339,8 +3333,7 @@ pub(crate) mod test { .await .unwrap(); - // Nobody told us what this RESET touched (query parser off), so the - // cache is worthless and we start over. + // A RESET we never recorded, as when the query parser is off. server.execute("RESET search_path").await.unwrap(); assert!(server.client_params.is_empty()); diff --git a/pgdog/src/frontend/client/query_engine/set.rs b/pgdog/src/frontend/client/query_engine/set.rs index 622c94594..275204f30 100644 --- a/pgdog/src/frontend/client/query_engine/set.rs +++ b/pgdog/src/frontend/client/query_engine/set.rs @@ -62,9 +62,8 @@ impl QueryEngine { } if self.backend.connected() { - // The server is ours right now, so its session changes with the - // client's. Record it, or we won't know to undo it for whoever - // gets this connection next. + // The server is ours, so its session changes with the client's: + // record it or we won't know what to undo for the next client. self.backend.record_params(params, context.in_transaction()); self.execute(context, None).await?; } else { diff --git a/pgdog/src/net/parameter.rs b/pgdog/src/net/parameter.rs index 99abb7540..a7ddfc4ae 100644 --- a/pgdog/src/net/parameter.rs +++ b/pgdog/src/net/parameter.rs @@ -247,8 +247,7 @@ impl Parameters { } } - /// Remove parameter from params temporarily. The transaction - /// is comitted, it will be removed permanently. + /// Remove a parameter permanently. pub(crate) fn reset(&mut self, name: impl AsRef) { let name = name.as_ref().to_lowercase(); @@ -258,12 +257,11 @@ impl Parameters { self.transaction_params.remove(&name); self.transaction_local_params.remove(&name); - // Nothing left to restore: the value is gone for good. + // Nothing left to restore on a rollback. self.reset_params.remove(&name); } - /// Remove a parameter, but only for the duration of the transaction: - /// a ROLLBACK brings its value back. + /// Remove a parameter until the transaction ends: a ROLLBACK brings it back. pub(crate) fn reset_transaction(&mut self, name: impl AsRef) { let name = name.as_ref().to_lowercase(); @@ -291,7 +289,7 @@ impl Parameters { } } - /// Reset all tracked parameters for the duration of the transaction. + /// Reset all tracked parameters until the transaction ends. pub(crate) fn reset_all_transaction(&mut self) { for key in self.resettable_keys() { self.reset_transaction(&key); @@ -299,8 +297,7 @@ impl Parameters { } fn resettable_keys(&self) -> Vec { - // The keys have to be lifted out before we can reset them: resetting - // borrows the maps we'd be iterating. + // Lifted out first: resetting borrows the maps we'd be iterating. let mut keys: Vec = self .params .keys() @@ -988,7 +985,7 @@ mod test { params.reset("search_path"); - // A transaction that comes later has nothing to do with that RESET. + // A later transaction has nothing to do with that RESET. params.rollback(); assert_eq!(params.get("search_path"), None); From 85f1ef8b71c048983b6049ad18a6b8db288ac3e0 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Mon, 24 Aug 2026 13:18:09 +0300 Subject: [PATCH 05/18] Drop comments that only restate the function name --- pgdog/src/backend/pool/connection/binding.rs | 2 -- pgdog/src/backend/server.rs | 1 - pgdog/src/net/parameter.rs | 1 - 3 files changed, 4 deletions(-) diff --git a/pgdog/src/backend/pool/connection/binding.rs b/pgdog/src/backend/pool/connection/binding.rs index 6e986d534..8298222ea 100644 --- a/pgdog/src/backend/pool/connection/binding.rs +++ b/pgdog/src/backend/pool/connection/binding.rs @@ -443,7 +443,6 @@ impl Binding { } } - /// Record a client parameter change on every server we hold. pub fn record_params(&mut self, params: &[SetParam], in_transaction: bool) { match self { Binding::Direct(server, ..) => server.record_params(params, in_transaction), @@ -454,7 +453,6 @@ impl Binding { } } - /// Record a client `RESET ALL` on every server we hold. pub fn record_reset_all(&mut self, in_transaction: bool) { match self { Binding::Direct(server, ..) => server.record_reset_all(in_transaction), diff --git a/pgdog/src/backend/server.rs b/pgdog/src/backend/server.rs index 0a9383060..451e22216 100644 --- a/pgdog/src/backend/server.rs +++ b/pgdog/src/backend/server.rs @@ -800,7 +800,6 @@ impl Server { self.params_recorded = true; } - /// Record a client's `RESET ALL`. pub fn record_reset_all(&mut self, in_transaction: bool) { if in_transaction { self.client_params.reset_all_transaction(); diff --git a/pgdog/src/net/parameter.rs b/pgdog/src/net/parameter.rs index a7ddfc4ae..801339782 100644 --- a/pgdog/src/net/parameter.rs +++ b/pgdog/src/net/parameter.rs @@ -289,7 +289,6 @@ impl Parameters { } } - /// Reset all tracked parameters until the transaction ends. pub(crate) fn reset_all_transaction(&mut self) { for key in self.resettable_keys() { self.reset_transaction(&key); From 4bf44a79ca9b1d3a0b396b4c5ce65c76599b6d1f Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Sat, 26 Sep 2026 21:03:11 +0300 Subject: [PATCH 06/18] Keep the new recording methods crate-private like the rest of main main narrowed the crate's API to pub(crate) while this branch was open. --- pgdog/src/backend/pool/connection/binding.rs | 4 ++-- pgdog/src/backend/server.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pgdog/src/backend/pool/connection/binding.rs b/pgdog/src/backend/pool/connection/binding.rs index 8298222ea..d54a5e504 100644 --- a/pgdog/src/backend/pool/connection/binding.rs +++ b/pgdog/src/backend/pool/connection/binding.rs @@ -443,7 +443,7 @@ impl Binding { } } - pub fn record_params(&mut self, params: &[SetParam], in_transaction: bool) { + pub(crate) fn record_params(&mut self, params: &[SetParam], in_transaction: bool) { match self { Binding::Direct(server, ..) => server.record_params(params, in_transaction), Binding::MultiShard(servers, _) => servers @@ -453,7 +453,7 @@ impl Binding { } } - pub fn record_reset_all(&mut self, in_transaction: bool) { + pub(crate) fn record_reset_all(&mut self, in_transaction: bool) { match self { Binding::Direct(server, ..) => server.record_reset_all(in_transaction), Binding::MultiShard(servers, _) => servers diff --git a/pgdog/src/backend/server.rs b/pgdog/src/backend/server.rs index 451e22216..325a21ebc 100644 --- a/pgdog/src/backend/server.rs +++ b/pgdog/src/backend/server.rs @@ -782,7 +782,7 @@ impl Server { /// Record a client's parameter change, so we know what to undo before /// this connection goes to somebody else. - pub fn record_params(&mut self, params: &[SetParam], in_transaction: bool) { + pub(crate) fn record_params(&mut self, params: &[SetParam], in_transaction: bool) { for param in params { match (¶m.value, in_transaction) { (Some(value), true) => { @@ -800,7 +800,7 @@ impl Server { self.params_recorded = true; } - pub fn record_reset_all(&mut self, in_transaction: bool) { + pub(crate) fn record_reset_all(&mut self, in_transaction: bool) { if in_transaction { self.client_params.reset_all_transaction(); } else { From 9ec0638a3f9d633b2525f4d765c2105fdb49cb8d Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Tue, 4 Aug 2026 17:57:30 +0300 Subject: [PATCH 07/18] Resolve set_config() arguments from the Bind message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parser only understood constants, so a parameterized call went through untouched and whatever it changed stayed on the connection: SELECT pg_catalog.set_config($1, $2, false) Read $n from the Bind message instead. The is_local argument is decoded from either wire format. Arguments that still don't resolve — no Bind message, a parameter that isn't text, an expression — leave the statement an ordinary query, same as today: it runs, and we don't know what it changed. --- .../router/parser/query/set_config.rs | 65 +++++-- .../router/parser/query/test/test_set.rs | 158 +++++++++++++++++- 2 files changed, 211 insertions(+), 12 deletions(-) diff --git a/pgdog/src/frontend/router/parser/query/set_config.rs b/pgdog/src/frontend/router/parser/query/set_config.rs index 7aad92b0b..93ed4c78f 100644 --- a/pgdog/src/frontend/router/parser/query/set_config.rs +++ b/pgdog/src/frontend/router/parser/query/set_config.rs @@ -1,16 +1,17 @@ use super::*; +use crate::net::messages::{Bind, Format}; impl QueryParser { /// Handle SELECT set_config('key', 'value', is_local) /// - /// If the function arguments are a form we cannot handle, we warn and - /// pass through + /// Arguments we can't resolve leave the statement an ordinary query: it + /// runs, but we don't know what it changed. pub(super) fn set_config( &mut self, fcall: &nodes::FuncCall, context: &QueryParserContext, ) -> Command { - if let Some(param) = parse_args(fcall) { + if let Some(param) = parse_args(fcall, context.router_context.bind) { Command::Set { params: vec![param], route: Route::write(context.shards_calculator.shard()), @@ -22,28 +23,65 @@ impl QueryParser { ) } } + } /// Returns None if the arguments could not be parsed -fn parse_args(fcall: &nodes::FuncCall) -> Option { - let name = parse_config_name(fcall.args().first()?)?; - let value = parse_config_value(fcall.args().get(1)?)?; - let local = parse_is_local(fcall.args().get(2)?)?; +fn parse_args(fcall: &nodes::FuncCall, bind: Option<&Bind>) -> Option { + let name = parse_config_name(fcall.args().first()?, bind)?; + let value = parse_config_value(fcall.args().get(1)?, bind)?; + let local = parse_is_local(fcall.args().get(2)?, bind)?; Some(SetParam { name, value, local }) } +/// Get the value bound to `$number`. The inner Option is the SQL NULL. +fn bound_text(bind: Option<&Bind>, number: i32) -> Option> { + let index = usize::try_from(number).ok()?.checked_sub(1)?; + let param = bind?.parameter(index).ok()??; + + if param.is_null() { + Some(None) + } else { + Some(Some(param.text()?.to_owned())) + } +} + +fn bound_bool(bind: Option<&Bind>, number: i32) -> Option { + let index = usize::try_from(number).ok()?.checked_sub(1)?; + let param = bind?.parameter(index).ok()??; + + if param.is_null() { + return None; + } + + match param.format() { + Format::Binary => match param.data() { + [0] => Some(false), + [1] => Some(true), + _ => None, + }, + Format::Text => match param.text()?.trim().to_lowercase().as_str() { + "t" | "true" | "y" | "yes" | "on" | "1" => Some(true), + "f" | "false" | "n" | "no" | "off" | "0" => Some(false), + _ => None, + }, + } +} + /// Returns None if the name could not be parsed -fn parse_config_name(arg: Node<'_>) -> Option { +fn parse_config_name(arg: Node<'_>, bind: Option<&Bind>) -> Option { match arg { Node::A_Const(c) => c.val()?.string_value().map(ToOwned::to_owned), - // Only constant strings can be handled for now + Node::ParamRef(nodes::ParamRef { number, .. }) => bound_text(bind, *number)?, _ => None, } } +/// Returns None if the name could not be parsed + /// Returns None if the value could not be parsed, Some(None) if the value /// is NULL, and Some if the value was successfully parsed -fn parse_config_value(arg: Node<'_>) -> Option> { +fn parse_config_value(arg: Node<'_>, bind: Option<&Bind>) -> Option> { match arg { Node::A_Const(c) => match c.val() { Some(value) => Some(Some(ParameterValue::String( @@ -51,14 +89,19 @@ fn parse_config_value(arg: Node<'_>) -> Option> { ))), None => Some(None), }, + Node::ParamRef(nodes::ParamRef { number, .. }) => { + Some(bound_text(bind, *number)?.map(ParameterValue::String)) + } _ => None, } } /// Returns None if the node was not a constant boolean -fn parse_is_local(arg: Node<'_>) -> Option { +fn parse_is_local(arg: Node<'_>, bind: Option<&Bind>) -> Option { match arg { Node::A_Const(c) => c.val()?.bool_value(), + Node::ParamRef(nodes::ParamRef { number, .. }) => bound_bool(bind, *number), _ => None, } } + diff --git a/pgdog/src/frontend/router/parser/query/test/test_set.rs b/pgdog/src/frontend/router/parser/query/test/test_set.rs index 8917f2993..b028027cd 100644 --- a/pgdog/src/frontend/router/parser/query/test/test_set.rs +++ b/pgdog/src/frontend/router/parser/query/test/test_set.rs @@ -7,7 +7,7 @@ use crate::{ route::{OverrideReason, ShardSource}, }, }, - net::parameter::ParameterValue, + net::{Format, messages::Parameter, parameter::ParameterValue}, }; use super::Error; @@ -284,3 +284,159 @@ fn test_single_shard_set() { _ => panic!("not a set"), } } + +#[test] +fn test_set_config_bound_params() { + let mut test = QueryParserTest::new(); + + let command = test.execute(vec![ + Parse::named( + "__test_set_config", + "SELECT pg_catalog.set_config($1, $2, $3)", + ) + .into(), + Bind::new_params( + "__test_set_config", + &[ + Parameter::new(b"search_path"), + Parameter::new(b""), + Parameter::new(b"f"), + ], + ) + .into(), + Execute::new().into(), + Sync.into(), + ]); + + match command { + Command::Set { + ref params, + is_select, + .. + } => { + assert_eq!(params.len(), 1); + assert_eq!(params[0].name, "search_path"); + assert_eq!(params[0].value, Some(ParameterValue::String("".into()))); + assert!(!params[0].local); + assert!(is_select); + } + _ => panic!("expected Command::Set, got {command:#?}"), + } +} + +#[test] +fn test_set_config_bound_null_value() { + let mut test = QueryParserTest::new(); + + let command = test.execute(vec![ + Parse::named("__test_set_config_null", "SELECT set_config($1, $2, false)").into(), + Bind::new_params( + "__test_set_config_null", + &[Parameter::new(b"lock_timeout"), Parameter::new_null()], + ) + .into(), + Execute::new().into(), + Sync.into(), + ]); + + match command { + Command::Set { ref params, .. } => { + assert_eq!(params[0].name, "lock_timeout"); + assert_eq!(params[0].value, None); + } + _ => panic!("expected Command::Set, got {command:#?}"), + } +} + +#[test] +fn test_set_config_unresolvable_args_stay_a_query() { + let mut test = QueryParserTest::new(); + + // No Bind message, so the parameters can't be resolved. + let command = test.execute(vec![ + Query::new("SELECT pg_catalog.set_config($1, $2, false)").into(), + ]); + + assert!( + matches!(command, Command::Query(_)), + "expected Command::Query, got {command:#?}", + ); + assert!(command.route().is_write()); +} + +#[test] +fn test_set_config_expression_stays_a_query() { + let mut test = QueryParserTest::new(); + + let command = test.execute(vec![ + Query::new("SELECT set_config('search_path', current_setting('search_path'), false)") + .into(), + ]); + + assert!( + matches!(command, Command::Query(_)), + "expected Command::Query, got {command:#?}", + ); + assert!(command.route().is_write()); +} + +#[test] +fn test_set_config_bound_binary_is_local() { + let mut test = QueryParserTest::new(); + + let command = test.execute(vec![ + Parse::named("__test_set_config_bin", "SELECT set_config($1, $2, $3)").into(), + Bind::new_params_codes( + "__test_set_config_bin", + &[ + Parameter::new(b"statement_timeout"), + Parameter::new(b"1000"), + Parameter::new(&[1]), + ], + &[Format::Text, Format::Text, Format::Binary], + ) + .into(), + Execute::new().into(), + Sync.into(), + ]); + + match command { + Command::Set { ref params, .. } => { + assert_eq!(params[0].name, "statement_timeout"); + assert_eq!(params[0].value, Some(ParameterValue::String("1000".into()))); + assert!(params[0].local, "binary true must be read as SET LOCAL"); + } + _ => panic!("expected Command::Set, got {command:#?}"), + } +} + +#[test] +fn test_set_config_bound_non_utf8_stays_a_query() { + let mut test = QueryParserTest::new(); + + // set_config() only takes text; a value we can't read as text is one we + // can't track. + let command = test.execute(vec![ + Parse::named( + "__test_set_config_bytes", + "SELECT set_config($1, $2, false)", + ) + .into(), + Bind::new_params( + "__test_set_config_bytes", + &[ + Parameter::new(b"search_path"), + Parameter::new(&[0xff, 0xfe]), + ], + ) + .into(), + Execute::new().into(), + Sync.into(), + ]); + + assert!( + matches!(command, Command::Query(_)), + "expected Command::Query, got {command:#?}", + ); + assert!(command.route().is_write()); +} From 9e93d48ffc5526cbea8cd9318a6cf43e1c16b998 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Sat, 26 Sep 2026 21:04:41 +0300 Subject: [PATCH 08/18] Let Postgres answer set_config() instead of faking it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SELECT set_config(...)` was intercepted and answered locally, which meant inventing a response for a query the client asked Postgres to run: the tag was SET where Postgres sends SELECT 1, and describing the portal claimed no rows and then sent one, which libpq rejects outright. It is a query, so treat it as one — take a server, record what the statement changes on that connection, and forward it. The client gets Postgres' own answer, and the next client gets the connection with that parameter reset. --- integration/pgdog.toml | 7 +++++++ integration/python/test_session_params_leak.py | 9 +++++++++ pgdog/src/frontend/client/query_engine/set.rs | 15 +++++++++++++++ pgdog/src/frontend/router/parser/query/set.rs | 1 + .../frontend/router/parser/query/test/test_set.rs | 4 ++-- 5 files changed, 34 insertions(+), 2 deletions(-) diff --git a/integration/pgdog.toml b/integration/pgdog.toml index 1f59a12be..ad1b04324 100644 --- a/integration/pgdog.toml +++ b/integration/pgdog.toml @@ -516,3 +516,10 @@ password = "pgdog" database = "pgdog" level = "auto" engine = "pg_query_raw" + +# Session state leaks are what these tests are about, so don't let the parser +# opt out of looking at the statements that cause them. +[[query_parsers]] +database = "pgdog_leak" +level = "on" +engine = "pg_query_raw" diff --git a/integration/python/test_session_params_leak.py b/integration/python/test_session_params_leak.py index 9af9c4533..b07059912 100644 --- a/integration/python/test_session_params_leak.py +++ b/integration/python/test_session_params_leak.py @@ -59,6 +59,15 @@ def test_set_committed_after_connecting(): assert read("statement_timeout") == "0" +def test_set_config_bound_params(): + """set_config() arguments arrive in the Bind message, not as constants.""" + conn = connect() + conn.execute("SELECT pg_catalog.set_config(%s, %s, false)", ("search_path", "")) + conn.close() + + assert read("search_path") not in ("", '""') + + def test_reset_committed(): """A committed RESET is permanent and needs no undoing.""" conn = connect() diff --git a/pgdog/src/frontend/client/query_engine/set.rs b/pgdog/src/frontend/client/query_engine/set.rs index 275204f30..141c698c7 100644 --- a/pgdog/src/frontend/client/query_engine/set.rs +++ b/pgdog/src/frontend/client/query_engine/set.rs @@ -24,6 +24,21 @@ impl QueryEngine { return Ok(()); } + // `SELECT set_config(...)` is a query and Postgres answers it, so take a + // server before touching the parameters: syncing a change the statement + // is about to make itself would just send it twice. + if set_config && !self.backend.connected() { + let connected = if context.in_transaction() { + self.connect_transaction(context).await? + } else { + self.connect(context, None).await? + }; + + if !connected { + return Ok(()); + } + } + let mut fake_command = "SET"; for param in params { let is_pin = param.name == PGDOG_PIN; diff --git a/pgdog/src/frontend/router/parser/query/set.rs b/pgdog/src/frontend/router/parser/query/set.rs index 923da72a7..d7c3c8f6d 100644 --- a/pgdog/src/frontend/router/parser/query/set.rs +++ b/pgdog/src/frontend/router/parser/query/set.rs @@ -123,4 +123,5 @@ impl QueryParser { Ok(value) } + } diff --git a/pgdog/src/frontend/router/parser/query/test/test_set.rs b/pgdog/src/frontend/router/parser/query/test/test_set.rs index b028027cd..2d0a63aca 100644 --- a/pgdog/src/frontend/router/parser/query/test/test_set.rs +++ b/pgdog/src/frontend/router/parser/query/test/test_set.rs @@ -311,14 +311,14 @@ fn test_set_config_bound_params() { match command { Command::Set { ref params, - is_select, + set_config, .. } => { assert_eq!(params.len(), 1); assert_eq!(params[0].name, "search_path"); assert_eq!(params[0].value, Some(ParameterValue::String("".into()))); assert!(!params[0].local); - assert!(is_select); + assert!(set_config); } _ => panic!("expected Command::Set, got {command:#?}"), } From 04209544a73122ce58bb9100129458457f3158a2 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Tue, 4 Aug 2026 23:59:47 +0300 Subject: [PATCH 09/18] Assert the value Postgres returns for a NULL set_config() The test pinned down the value PgDog made up while pretending to run the statement: the SetParam it parsed, so NULL for a reset. Postgres resets the setting and answers with the value it landed on, and that is what the client gets now that the statement reaches it. --- integration/rust/tests/integration/set_config.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/integration/rust/tests/integration/set_config.rs b/integration/rust/tests/integration/set_config.rs index 8d294d1a1..4affe217b 100644 --- a/integration/rust/tests/integration/set_config.rs +++ b/integration/rust/tests/integration/set_config.rs @@ -28,12 +28,13 @@ async fn test_set_config_behaves_like_set() { .unwrap(); assert_eq!(lock_timeout, "500s"); - let set_config: Option = - query_scalar("SELECT set_config('lock_timeout', NULL, false);") - .fetch_one(&mut *conn) - .await - .unwrap(); - assert_eq!(set_config, None); + // A NULL resets the setting, and set_config answers with the value the + // setting landed on, not with the NULL it was handed. + let set_config: String = query_scalar("SELECT set_config('lock_timeout', NULL, false);") + .fetch_one(&mut *conn) + .await + .unwrap(); + assert_eq!(set_config, "0"); let lock_timeout: String = query_scalar("SHOW lock_timeout") .fetch_one(&mut *conn) From 8e2f5a05ebffe086b4d49c92916f07d4c0bbed30 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Wed, 5 Aug 2026 00:40:06 +0300 Subject: [PATCH 10/18] Trim comments to what the code doesn't already say --- integration/pgdog.toml | 3 +-- .../rust/tests/integration/set_config.rs | 2 -- pgdog/src/frontend/client/query_engine/set.rs | 5 ++--- .../frontend/router/parser/query/set_config.rs | 18 +++++++++++++++--- .../router/parser/query/test/test_set.rs | 4 +--- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/integration/pgdog.toml b/integration/pgdog.toml index ad1b04324..e748f0bf2 100644 --- a/integration/pgdog.toml +++ b/integration/pgdog.toml @@ -517,8 +517,7 @@ database = "pgdog" level = "auto" engine = "pg_query_raw" -# Session state leaks are what these tests are about, so don't let the parser -# opt out of looking at the statements that cause them. +# These tests are about statements the parser would otherwise opt out of. [[query_parsers]] database = "pgdog_leak" level = "on" diff --git a/integration/rust/tests/integration/set_config.rs b/integration/rust/tests/integration/set_config.rs index 4affe217b..15e9b03d6 100644 --- a/integration/rust/tests/integration/set_config.rs +++ b/integration/rust/tests/integration/set_config.rs @@ -28,8 +28,6 @@ async fn test_set_config_behaves_like_set() { .unwrap(); assert_eq!(lock_timeout, "500s"); - // A NULL resets the setting, and set_config answers with the value the - // setting landed on, not with the NULL it was handed. let set_config: String = query_scalar("SELECT set_config('lock_timeout', NULL, false);") .fetch_one(&mut *conn) .await diff --git a/pgdog/src/frontend/client/query_engine/set.rs b/pgdog/src/frontend/client/query_engine/set.rs index 141c698c7..496599396 100644 --- a/pgdog/src/frontend/client/query_engine/set.rs +++ b/pgdog/src/frontend/client/query_engine/set.rs @@ -24,9 +24,8 @@ impl QueryEngine { return Ok(()); } - // `SELECT set_config(...)` is a query and Postgres answers it, so take a - // server before touching the parameters: syncing a change the statement - // is about to make itself would just send it twice. + // Take a server before touching the parameters: syncing a change the + // statement is about to make itself would send it twice. if set_config && !self.backend.connected() { let connected = if context.in_transaction() { self.connect_transaction(context).await? diff --git a/pgdog/src/frontend/router/parser/query/set_config.rs b/pgdog/src/frontend/router/parser/query/set_config.rs index 93ed4c78f..9553f1d91 100644 --- a/pgdog/src/frontend/router/parser/query/set_config.rs +++ b/pgdog/src/frontend/router/parser/query/set_config.rs @@ -4,8 +4,8 @@ use crate::net::messages::{Bind, Format}; impl QueryParser { /// Handle SELECT set_config('key', 'value', is_local) /// - /// Arguments we can't resolve leave the statement an ordinary query: it - /// runs, but we don't know what it changed. + /// Arguments we can't resolve leave it an ordinary query: it runs, but + /// we don't learn what it changed. pub(super) fn set_config( &mut self, fcall: &nodes::FuncCall, @@ -34,7 +34,19 @@ fn parse_args(fcall: &nodes::FuncCall, bind: Option<&Bind>) -> Option Some(SetParam { name, value, local }) } -/// Get the value bound to `$number`. The inner Option is the SQL NULL. +cfg_select! { + not(feature = "new_parser") => { + fn parse_args(fcall: &FuncCall, bind: Option<&Bind>) -> Option { + let name = parse_config_name(fcall.args.first()?, bind)?; + let value = parse_config_value(fcall.args.get(1)?, bind)?; + let local = parse_is_local(fcall.args.get(2)?, bind)?; + Some(SetParam { name, value, local }) + } + } + _ => {} +} + +/// Value bound to `$number`; the inner Option is the SQL NULL. fn bound_text(bind: Option<&Bind>, number: i32) -> Option> { let index = usize::try_from(number).ok()?.checked_sub(1)?; let param = bind?.parameter(index).ok()??; diff --git a/pgdog/src/frontend/router/parser/query/test/test_set.rs b/pgdog/src/frontend/router/parser/query/test/test_set.rs index 2d0a63aca..75205045e 100644 --- a/pgdog/src/frontend/router/parser/query/test/test_set.rs +++ b/pgdog/src/frontend/router/parser/query/test/test_set.rs @@ -352,7 +352,6 @@ fn test_set_config_bound_null_value() { fn test_set_config_unresolvable_args_stay_a_query() { let mut test = QueryParserTest::new(); - // No Bind message, so the parameters can't be resolved. let command = test.execute(vec![ Query::new("SELECT pg_catalog.set_config($1, $2, false)").into(), ]); @@ -414,8 +413,7 @@ fn test_set_config_bound_binary_is_local() { fn test_set_config_bound_non_utf8_stays_a_query() { let mut test = QueryParserTest::new(); - // set_config() only takes text; a value we can't read as text is one we - // can't track. + // set_config() takes text; what we can't read as text we can't track. let command = test.execute(vec![ Parse::named( "__test_set_config_bytes", From eca21a0ec502ccfda2e15181a2cf42ca977dd79c Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Fri, 7 Aug 2026 16:48:20 +0300 Subject: [PATCH 11/18] Run the leak tests at both parser levels, and against RLS The fixture pinned the leak database to level "on", which is the one level that always parses. Everything a statement has to get past to reach the parser at the default level went untested, and set_config() is a function call inside a SELECT: it matches no statement-start keyword, so the gate drops it and the value stays on the connection. A second copy of the same single-primary database at "auto" covers that path. Both copies run every test. The new test covers the half of the leak that reports nothing: row-level security keyed on a custom GUC is how multi-tenant applications isolate tenants, set_config() is how that GUC gets set, and a value that outlives its client makes the next one read as the previous tenant. Reads go through a plain role because the pooler's user is a superuser and superusers ignore RLS. --- integration/pgdog.toml | 17 +++ .../python/test_session_params_leak.py | 117 +++++++++++++++--- integration/users.toml | 5 + 3 files changed, 123 insertions(+), 16 deletions(-) diff --git a/integration/pgdog.toml b/integration/pgdog.toml index e748f0bf2..fca36f6c5 100644 --- a/integration/pgdog.toml +++ b/integration/pgdog.toml @@ -62,6 +62,8 @@ read_only = true # ------------------------------------------------------------------------------ # ----- Database :: pgdog_leak ------------------------------------------------- # One server connection, never reaped: the next client has to get the same one. +# Two copies of the same single-primary database, differing only in parser level, +# so the leak tests cover both ways a statement reaches the parser. [[databases]] name = "pgdog_leak" @@ -70,6 +72,13 @@ database_name = "pgdog" pool_size = 1 min_pool_size = 1 +[[databases]] +name = "pgdog_leak_auto" +host = "127.0.0.1" +database_name = "pgdog" +pool_size = 1 +min_pool_size = 1 + # ------------------------------------------------------------------------------ # ----- Database :: pgdog_sharded ---------------------------------------------- @@ -522,3 +531,11 @@ engine = "pg_query_raw" database = "pgdog_leak" level = "on" engine = "pg_query_raw" + +# The same tests at the default level. A single primary with no replicas is the +# topology where "auto" doesn't force the parser on, so the statement has to get +# past the regex gate on its own -- the path "on" skips. +[[query_parsers]] +database = "pgdog_leak_auto" +level = "auto" +engine = "pg_query_raw" diff --git a/integration/python/test_session_params_leak.py b/integration/python/test_session_params_leak.py index b07059912..47f6f4cd1 100644 --- a/integration/python/test_session_params_leak.py +++ b/integration/python/test_session_params_leak.py @@ -3,16 +3,27 @@ Runs against a database with a single server connection, so the next client always gets the connection the previous one used. current_setting() is used instead of SHOW because SHOW can be answered by PgDog itself. + +Every test runs against two copies of that database that differ only in parser +level: "on" always parses, "auto" leaves a single-primary cluster to the regex +gate. A statement that only the gate can let through -- set_config(), a function +call inside a SELECT rather than a statement-start keyword -- reaches the parser +on one and has to earn it on the other. """ +import uuid + import psycopg +import pytest +DATABASES = ["pgdog_leak", "pgdog_leak_auto"] -def connect(): + +def connect(dbname): conn = psycopg.connect( user="pgdog", password="pgdog", - dbname="pgdog_leak", + dbname=dbname, host="127.0.0.1", port=6432, ) @@ -22,21 +33,26 @@ def connect(): return conn -def read(setting): - conn = connect() +def read(dbname, setting): + conn = connect(dbname) value = conn.execute(f"SELECT current_setting('{setting}')").fetchone()[0] conn.close() return value -def test_reset_rolled_back(): +@pytest.fixture(params=DATABASES) +def dbname(request): + return request.param + + +def test_reset_rolled_back(dbname): """A ROLLBACK brings back the value the RESET cleared. This is the sequence pg_dump -t
emits. SET search_path TO '' leaves an empty quoted identifier, hence the two spellings of "empty". """ - conn = connect() + conn = connect(dbname) conn.execute("SET search_path TO ''") with conn.transaction(): conn.execute("SET TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ ONLY") @@ -45,36 +61,105 @@ def test_reset_rolled_back(): raise psycopg.Rollback() conn.close() - assert read("search_path") not in ("", '""') + assert read(dbname, "search_path") not in ("", '""') -def test_set_committed_after_connecting(): +def test_set_committed_after_connecting(dbname): """A SET that lands once the connection is already ours.""" - conn = connect() + conn = connect(dbname) with conn.transaction(): conn.execute("SELECT 1") conn.execute("SET statement_timeout TO '5s'") conn.close() - assert read("statement_timeout") == "0" + assert read(dbname, "statement_timeout") == "0" -def test_set_config_bound_params(): +def test_set_config_bound_params(dbname): """set_config() arguments arrive in the Bind message, not as constants.""" - conn = connect() + conn = connect(dbname) conn.execute("SELECT pg_catalog.set_config(%s, %s, false)", ("search_path", "")) conn.close() - assert read("search_path") not in ("", '""') + assert read(dbname, "search_path") not in ("", '""') -def test_reset_committed(): +def test_reset_committed(dbname): """A committed RESET is permanent and needs no undoing.""" - conn = connect() + conn = connect(dbname) conn.execute("SET search_path TO public") with conn.transaction(): conn.execute("SELECT 1") conn.execute("RESET search_path") conn.close() - assert read("search_path") == '"$user", public' + assert read(dbname, "search_path") == '"$user", public' + + +TENANT_A = "11111111-1111-1111-1111-111111111111" +TENANT_B = "22222222-2222-2222-2222-222222222222" + + +@pytest.fixture +def tenants(dbname): + """A table whose rows are visible only to the tenant named in a GUC. + + Reads go through a plain role: the pooler's own user is a superuser, and + superusers ignore row-level security however the table is configured. + + NULLIF() is deliberate: a targeted RESET leaves the placeholder GUC as an + empty string rather than unset, and '' would fail the ::uuid cast. + """ + table = "rls_probe_" + uuid.uuid4().hex[:8] + conn = connect(dbname) + conn.execute( + "DO $$ BEGIN CREATE ROLE rls_tenant NOLOGIN; " + "EXCEPTION WHEN duplicate_object THEN NULL; END $$" + ) + conn.execute(f"CREATE TABLE public.{table} (org_id uuid, note text)") + conn.execute( + f"INSERT INTO public.{table} VALUES ('{TENANT_A}', 'a'), ('{TENANT_B}', 'b')" + ) + conn.execute(f"GRANT SELECT ON public.{table} TO rls_tenant") + conn.execute(f"ALTER TABLE public.{table} ENABLE ROW LEVEL SECURITY") + conn.execute( + f"CREATE POLICY tenant_isolation ON public.{table} USING " + "(org_id = NULLIF(current_setting('app.current_org_id', true), '')::uuid)" + ) + conn.close() + + yield table + + conn = connect(dbname) + conn.execute("RESET ROLE") + conn.execute(f"DROP TABLE public.{table}") + conn.close() + + +def test_tenant_guc_does_not_outlive_its_client(dbname, tenants): + """The silent half of the leak: no error, just another tenant's rows. + + Row-level security keyed on a custom GUC is how multi-tenant applications + isolate tenants, and set_config() with a bound parameter is how that GUC + gets set. A value that survives checkin makes the next client read as the + previous tenant. + """ + first = connect(dbname) + first.execute("SET ROLE rls_tenant") + first.execute( + "SELECT pg_catalog.set_config('app.current_org_id', %s, false)", (TENANT_A,) + ) + mine = first.execute(f"SELECT note FROM public.{tenants}").fetchall() + first.close() + + assert mine == [("a",)], "the tenant that set the GUC sees its own row" + + second = connect(dbname) + second.execute("SET ROLE rls_tenant") + theirs = second.execute(f"SELECT note FROM public.{tenants}").fetchall() + leaked = second.execute( + "SELECT current_setting('app.current_org_id', true)" + ).fetchone()[0] + second.close() + + assert theirs == [], f"next client read as tenant {leaked!r}" diff --git a/integration/users.toml b/integration/users.toml index 360246b5b..25a24d84f 100644 --- a/integration/users.toml +++ b/integration/users.toml @@ -8,6 +8,11 @@ name = "pgdog" database = "pgdog_leak" password = "pgdog" +[[users]] +name = "pgdog" +database = "pgdog_leak_auto" +password = "pgdog" + [[users]] name = "pgdog_migrator" database = "pgdog" From 12c058ebb4ad900016674db2a936424a87b54904 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Sat, 8 Aug 2026 02:26:08 +0300 Subject: [PATCH 12/18] Finish removing the old parser's copies after the rebase main dropped the second parser in #1324, and this branch still carried the changes for both. Resolving the rebase left one cfg_select! block and a stray blank line behind. --- pgdog/src/frontend/router/parser/query/set.rs | 1 - .../frontend/router/parser/query/set_config.rs | 16 ---------------- 2 files changed, 17 deletions(-) diff --git a/pgdog/src/frontend/router/parser/query/set.rs b/pgdog/src/frontend/router/parser/query/set.rs index d7c3c8f6d..923da72a7 100644 --- a/pgdog/src/frontend/router/parser/query/set.rs +++ b/pgdog/src/frontend/router/parser/query/set.rs @@ -123,5 +123,4 @@ impl QueryParser { Ok(value) } - } diff --git a/pgdog/src/frontend/router/parser/query/set_config.rs b/pgdog/src/frontend/router/parser/query/set_config.rs index 9553f1d91..bd117f7fd 100644 --- a/pgdog/src/frontend/router/parser/query/set_config.rs +++ b/pgdog/src/frontend/router/parser/query/set_config.rs @@ -23,7 +23,6 @@ impl QueryParser { ) } } - } /// Returns None if the arguments could not be parsed @@ -34,18 +33,6 @@ fn parse_args(fcall: &nodes::FuncCall, bind: Option<&Bind>) -> Option Some(SetParam { name, value, local }) } -cfg_select! { - not(feature = "new_parser") => { - fn parse_args(fcall: &FuncCall, bind: Option<&Bind>) -> Option { - let name = parse_config_name(fcall.args.first()?, bind)?; - let value = parse_config_value(fcall.args.get(1)?, bind)?; - let local = parse_is_local(fcall.args.get(2)?, bind)?; - Some(SetParam { name, value, local }) - } - } - _ => {} -} - /// Value bound to `$number`; the inner Option is the SQL NULL. fn bound_text(bind: Option<&Bind>, number: i32) -> Option> { let index = usize::try_from(number).ok()?.checked_sub(1)?; @@ -89,8 +76,6 @@ fn parse_config_name(arg: Node<'_>, bind: Option<&Bind>) -> Option { } } -/// Returns None if the name could not be parsed - /// Returns None if the value could not be parsed, Some(None) if the value /// is NULL, and Some if the value was successfully parsed fn parse_config_value(arg: Node<'_>, bind: Option<&Bind>) -> Option> { @@ -116,4 +101,3 @@ fn parse_is_local(arg: Node<'_>, bind: Option<&Bind>) -> Option { _ => None, } } - From 5811423f08134a89b28cbd4e48d9461c85f2de87 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Sat, 8 Aug 2026 02:49:27 +0300 Subject: [PATCH 13/18] Make the tenant test prove the clients shared a connection It passed in CI and failed locally, which means it was answering a question it never asked: with a different server connection there is nothing for the first client to have left behind, and the assertion holds for the wrong reason. Compare pg_backend_pid() across the two clients, and separate the GUC outliving its client from row-level security failing to filter, so a failure says which of the two happened. --- integration/python/test_session_params_leak.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/integration/python/test_session_params_leak.py b/integration/python/test_session_params_leak.py index 47f6f4cd1..e044146a1 100644 --- a/integration/python/test_session_params_leak.py +++ b/integration/python/test_session_params_leak.py @@ -150,6 +150,7 @@ def test_tenant_guc_does_not_outlive_its_client(dbname, tenants): "SELECT pg_catalog.set_config('app.current_org_id', %s, false)", (TENANT_A,) ) mine = first.execute(f"SELECT note FROM public.{tenants}").fetchall() + served_by = first.execute("SELECT pg_backend_pid()").fetchone()[0] first.close() assert mine == [("a",)], "the tenant that set the GUC sees its own row" @@ -160,6 +161,11 @@ def test_tenant_guc_does_not_outlive_its_client(dbname, tenants): leaked = second.execute( "SELECT current_setting('app.current_org_id', true)" ).fetchone()[0] + same_server = second.execute("SELECT pg_backend_pid()").fetchone()[0] second.close() - assert theirs == [], f"next client read as tenant {leaked!r}" + # Without this the test passes whenever the pool happens to hand out a + # different connection, which proves nothing about what the first one left. + assert same_server == served_by, "clients did not share a server connection" + assert leaked in (None, ""), f"the tenant GUC outlived its client: {leaked!r}" + assert theirs == [], "next client read the previous tenant's rows" From 97bf4fd15982ee9ac6fab968288781b871bd81b0 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Mon, 24 Aug 2026 12:59:47 +0300 Subject: [PATCH 14/18] Read set_config() arguments through StatementParameters Upstream generalized the extended-protocol Bind into StatementParameters, which also covers parameters supplied by a SQL-level EXECUTE. Reading the arguments through it keeps the parameterized set_config() case resolvable and extends it to PREPARE/EXECUTE, which the Bind-only version could not see. --- .../router/parser/query/set_config.rs | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/pgdog/src/frontend/router/parser/query/set_config.rs b/pgdog/src/frontend/router/parser/query/set_config.rs index bd117f7fd..56a90da60 100644 --- a/pgdog/src/frontend/router/parser/query/set_config.rs +++ b/pgdog/src/frontend/router/parser/query/set_config.rs @@ -1,5 +1,5 @@ use super::*; -use crate::net::messages::{Bind, Format}; +use crate::net::messages::Format; impl QueryParser { /// Handle SELECT set_config('key', 'value', is_local) @@ -26,17 +26,20 @@ impl QueryParser { } /// Returns None if the arguments could not be parsed -fn parse_args(fcall: &nodes::FuncCall, bind: Option<&Bind>) -> Option { - let name = parse_config_name(fcall.args().first()?, bind)?; - let value = parse_config_value(fcall.args().get(1)?, bind)?; - let local = parse_is_local(fcall.args().get(2)?, bind)?; +fn parse_args( + fcall: &nodes::FuncCall, + params: Option>, +) -> Option { + let name = parse_config_name(fcall.args().first()?, params)?; + let value = parse_config_value(fcall.args().get(1)?, params)?; + let local = parse_is_local(fcall.args().get(2)?, params)?; Some(SetParam { name, value, local }) } /// Value bound to `$number`; the inner Option is the SQL NULL. -fn bound_text(bind: Option<&Bind>, number: i32) -> Option> { +fn bound_text(params: Option>, number: i32) -> Option> { let index = usize::try_from(number).ok()?.checked_sub(1)?; - let param = bind?.parameter(index).ok()??; + let param = params?.parameter(index).ok()??; if param.is_null() { Some(None) @@ -45,9 +48,9 @@ fn bound_text(bind: Option<&Bind>, number: i32) -> Option> { } } -fn bound_bool(bind: Option<&Bind>, number: i32) -> Option { +fn bound_bool(params: Option>, number: i32) -> Option { let index = usize::try_from(number).ok()?.checked_sub(1)?; - let param = bind?.parameter(index).ok()??; + let param = params?.parameter(index).ok()??; if param.is_null() { return None; @@ -68,17 +71,20 @@ fn bound_bool(bind: Option<&Bind>, number: i32) -> Option { } /// Returns None if the name could not be parsed -fn parse_config_name(arg: Node<'_>, bind: Option<&Bind>) -> Option { +fn parse_config_name(arg: Node<'_>, params: Option>) -> Option { match arg { Node::A_Const(c) => c.val()?.string_value().map(ToOwned::to_owned), - Node::ParamRef(nodes::ParamRef { number, .. }) => bound_text(bind, *number)?, + Node::ParamRef(nodes::ParamRef { number, .. }) => bound_text(params, *number)?, _ => None, } } /// Returns None if the value could not be parsed, Some(None) if the value /// is NULL, and Some if the value was successfully parsed -fn parse_config_value(arg: Node<'_>, bind: Option<&Bind>) -> Option> { +fn parse_config_value( + arg: Node<'_>, + params: Option>, +) -> Option> { match arg { Node::A_Const(c) => match c.val() { Some(value) => Some(Some(ParameterValue::String( @@ -87,17 +93,17 @@ fn parse_config_value(arg: Node<'_>, bind: Option<&Bind>) -> Option Some(None), }, Node::ParamRef(nodes::ParamRef { number, .. }) => { - Some(bound_text(bind, *number)?.map(ParameterValue::String)) + Some(bound_text(params, *number)?.map(ParameterValue::String)) } _ => None, } } /// Returns None if the node was not a constant boolean -fn parse_is_local(arg: Node<'_>, bind: Option<&Bind>) -> Option { +fn parse_is_local(arg: Node<'_>, params: Option>) -> Option { match arg { Node::A_Const(c) => c.val()?.bool_value(), - Node::ParamRef(nodes::ParamRef { number, .. }) => bound_bool(bind, *number), + Node::ParamRef(nodes::ParamRef { number, .. }) => bound_bool(params, *number), _ => None, } } From f11a5949bb91fd5ddf8d0ebdbc9ca6a807d364fe Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Mon, 24 Aug 2026 13:21:37 +0300 Subject: [PATCH 15/18] Drop the parse_args comment the signature already makes --- pgdog/src/frontend/router/parser/query/set_config.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/pgdog/src/frontend/router/parser/query/set_config.rs b/pgdog/src/frontend/router/parser/query/set_config.rs index 56a90da60..d0b6f1409 100644 --- a/pgdog/src/frontend/router/parser/query/set_config.rs +++ b/pgdog/src/frontend/router/parser/query/set_config.rs @@ -25,7 +25,6 @@ impl QueryParser { } } -/// Returns None if the arguments could not be parsed fn parse_args( fcall: &nodes::FuncCall, params: Option>, From 7c3d5c42a9a89e17e56a870e332475a5024710cf Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Sat, 26 Sep 2026 21:25:47 +0300 Subject: [PATCH 16/18] Import Parameter by its module path main stopped re-exporting it from net::messages while this branch was open. --- pgdog/src/frontend/router/parser/query/test/test_set.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pgdog/src/frontend/router/parser/query/test/test_set.rs b/pgdog/src/frontend/router/parser/query/test/test_set.rs index 75205045e..9c344a97e 100644 --- a/pgdog/src/frontend/router/parser/query/test/test_set.rs +++ b/pgdog/src/frontend/router/parser/query/test/test_set.rs @@ -7,7 +7,7 @@ use crate::{ route::{OverrideReason, ShardSource}, }, }, - net::{Format, messages::Parameter, parameter::ParameterValue}, + net::{Format, messages::bind::Parameter, parameter::ParameterValue}, }; use super::Error; From 3f20b1f7aa8ec065f2965756214249ca57c1085e Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Sat, 26 Sep 2026 21:28:55 +0300 Subject: [PATCH 17/18] Give the auto-level leak tests a config where the gate decides main now sets prepared_statements = "full" in the shared integration config, and at "auto" that turns the parser on for every database. The pgdog_leak_auto copy stopped reaching the regex gate and passed without anything having fixed it. prepared_statements can't be set per database, so the auto copy moves to its own config under integration/python/gate with "extended", and run.sh runs the leak tests a second time against it. Against it, the two set_config() tests fail as they should until the gate learns about set_config() (#1327); the other three pass. --- integration/pgdog.toml | 17 ----------------- integration/python/gate/pgdog.toml | 18 ++++++++++++++++++ integration/python/gate/users.toml | 9 +++++++++ integration/python/run.sh | 10 ++++++++++ integration/python/test_session_params_leak.py | 14 ++++++++------ integration/users.toml | 5 ----- 6 files changed, 45 insertions(+), 28 deletions(-) create mode 100644 integration/python/gate/pgdog.toml create mode 100644 integration/python/gate/users.toml diff --git a/integration/pgdog.toml b/integration/pgdog.toml index fca36f6c5..e748f0bf2 100644 --- a/integration/pgdog.toml +++ b/integration/pgdog.toml @@ -62,8 +62,6 @@ read_only = true # ------------------------------------------------------------------------------ # ----- Database :: pgdog_leak ------------------------------------------------- # One server connection, never reaped: the next client has to get the same one. -# Two copies of the same single-primary database, differing only in parser level, -# so the leak tests cover both ways a statement reaches the parser. [[databases]] name = "pgdog_leak" @@ -72,13 +70,6 @@ database_name = "pgdog" pool_size = 1 min_pool_size = 1 -[[databases]] -name = "pgdog_leak_auto" -host = "127.0.0.1" -database_name = "pgdog" -pool_size = 1 -min_pool_size = 1 - # ------------------------------------------------------------------------------ # ----- Database :: pgdog_sharded ---------------------------------------------- @@ -531,11 +522,3 @@ engine = "pg_query_raw" database = "pgdog_leak" level = "on" engine = "pg_query_raw" - -# The same tests at the default level. A single primary with no replicas is the -# topology where "auto" doesn't force the parser on, so the statement has to get -# past the regex gate on its own -- the path "on" skips. -[[query_parsers]] -database = "pgdog_leak_auto" -level = "auto" -engine = "pg_query_raw" diff --git a/integration/python/gate/pgdog.toml b/integration/python/gate/pgdog.toml new file mode 100644 index 000000000..6da5a5a34 --- /dev/null +++ b/integration/python/gate/pgdog.toml @@ -0,0 +1,18 @@ +[general] +prepared_statements = "extended" + +[[databases]] +name = "pgdog" +host = "127.0.0.1" + +[[databases]] +name = "pgdog_leak_auto" +host = "127.0.0.1" +database_name = "pgdog" +pool_size = 1 +min_pool_size = 1 + +[[query_parsers]] +database = "pgdog_leak_auto" +level = "auto" +engine = "pg_query_raw" diff --git a/integration/python/gate/users.toml b/integration/python/gate/users.toml new file mode 100644 index 000000000..2a86d85b4 --- /dev/null +++ b/integration/python/gate/users.toml @@ -0,0 +1,9 @@ +[[users]] +name = "pgdog" +database = "pgdog" +password = "pgdog" + +[[users]] +name = "pgdog" +database = "pgdog_leak_auto" +password = "pgdog" diff --git a/integration/python/run.sh b/integration/python/run.sh index 89760d383..efb59a3b2 100644 --- a/integration/python/run.sh +++ b/integration/python/run.sh @@ -11,3 +11,13 @@ wait_for_pgdog source ${SCRIPT_DIR}/dev.sh stop_pgdog +run_pgdog integration/python/gate +wait_for_pgdog + +pushd ${SCRIPT_DIR} +source venv/bin/activate +PGDOG_LEAK_DATABASES=pgdog_leak_auto pytest -x test_session_params_leak.py +deactivate +popd + +stop_pgdog diff --git a/integration/python/test_session_params_leak.py b/integration/python/test_session_params_leak.py index e044146a1..e200a7829 100644 --- a/integration/python/test_session_params_leak.py +++ b/integration/python/test_session_params_leak.py @@ -4,19 +4,21 @@ always gets the connection the previous one used. current_setting() is used instead of SHOW because SHOW can be answered by PgDog itself. -Every test runs against two copies of that database that differ only in parser -level: "on" always parses, "auto" leaves a single-primary cluster to the regex -gate. A statement that only the gate can let through -- set_config(), a function -call inside a SELECT rather than a statement-start keyword -- reaches the parser -on one and has to earn it on the other. +Every test runs at two parser levels: "on" always parses, "auto" leaves a +single-primary cluster to the regex gate. A statement that only the gate can let +through -- set_config(), a function call inside a SELECT rather than a +statement-start keyword -- reaches the parser on one and has to earn it on the +other. "auto" needs its own config (gate/), where nothing else turns the parser +on; run.sh runs these tests a second time against it. """ +import os import uuid import psycopg import pytest -DATABASES = ["pgdog_leak", "pgdog_leak_auto"] +DATABASES = os.environ.get("PGDOG_LEAK_DATABASES", "pgdog_leak").split(",") def connect(dbname): diff --git a/integration/users.toml b/integration/users.toml index 25a24d84f..360246b5b 100644 --- a/integration/users.toml +++ b/integration/users.toml @@ -8,11 +8,6 @@ name = "pgdog" database = "pgdog_leak" password = "pgdog" -[[users]] -name = "pgdog" -database = "pgdog_leak_auto" -password = "pgdog" - [[users]] name = "pgdog_migrator" database = "pgdog" From 6e9bf7a1177b68269a6596099af2e279ef89c1f6 Mon Sep 17 00:00:00 2001 From: Igor Ohrimenko Date: Sat, 26 Sep 2026 21:30:51 +0300 Subject: [PATCH 18/18] Expect the two set_config() tests to fail at auto until #1327 They only pass once the regex gate matches set_config(), which is #1327, still open. strict=True turns them into XPASS failures the moment it lands, so the marker gets removed rather than forgotten. Checked on main: the two RESET/SET tests this branch fixes fail there, the set_config() ones are xfailed. --- integration/python/test_session_params_leak.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/integration/python/test_session_params_leak.py b/integration/python/test_session_params_leak.py index e200a7829..6e9bb084e 100644 --- a/integration/python/test_session_params_leak.py +++ b/integration/python/test_session_params_leak.py @@ -48,6 +48,17 @@ def dbname(request): return request.param +@pytest.fixture +def gated(request, dbname): + if dbname == "pgdog_leak_auto": + request.node.add_marker( + pytest.mark.xfail( + strict=True, + reason="the regex gate doesn't match set_config() yet (#1327)", + ) + ) + + def test_reset_rolled_back(dbname): """A ROLLBACK brings back the value the RESET cleared. @@ -77,7 +88,7 @@ def test_set_committed_after_connecting(dbname): assert read(dbname, "statement_timeout") == "0" -def test_set_config_bound_params(dbname): +def test_set_config_bound_params(dbname, gated): """set_config() arguments arrive in the Bind message, not as constants.""" conn = connect(dbname) conn.execute("SELECT pg_catalog.set_config(%s, %s, false)", ("search_path", "")) @@ -138,7 +149,7 @@ def tenants(dbname): conn.close() -def test_tenant_guc_does_not_outlive_its_client(dbname, tenants): +def test_tenant_guc_does_not_outlive_its_client(dbname, tenants, gated): """The silent half of the leak: no error, just another tenant's rows. Row-level security keyed on a custom GUC is how multi-tenant applications