Resolve set_config() arguments from the Bind message and let Postgres answer it

[IgorOhrimenko]

Based on #1299 — its two commits are included here and the diff shows both. The two commits that belong to this PR are the last ones.

Problem

set_config() interception only understood constant arguments, so a parameterized call went through untouched:

SELECT pg_catalog.set_config($1, $2, false)
-- Bind: $1 = 'search_path', $2 = ''

Whatever it changed stayed on the connection, and the next client inherited it — 42P01 on unqualified names until the connection was recycled.

The interception itself had a second problem. SELECT set_config(...) was 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 with D message without prior T.

Fix

Read $n from the Bind message, so a parameterized set_config() resolves like a constant one. The is_local argument is decoded from either wire format.

Then stop faking the answer. It is a query, so treat it as one: take a server, record what the statement changes on that connection (#1299's mechanism), and forward it. The client gets Postgres' own reply, and the next client gets the connection with that parameter reset — the invented response and its protocol bug are gone rather than fixed.

Arguments that still don't resolve — no Bind message, a parameter that isn't text, an expression such as set_config('search_path', current_setting('x'), false) — leave the statement an ordinary query. That is unchanged from today: it runs, and we don't know what it changed. Covering it would need a way to say "this parameter is now something we can't name", which I left out of this PR.

The flag marking these statements is renamed: it no longer describes how we imitate a SELECT, it says the statement is one.

Testing

• Parser: $1,$2,$3 resolve to the bound values; a NULL parameter becomes a reset; a binary is_local is decoded; arguments with no Bind, a non-UTF-8 value, or an expression stay an ordinary query.
• integration/python/test_session_params_leak.py::test_set_config_bound_params: a client poisons the pool through bound parameters, a later client must see a clean search_path. Fails on Track a client's parameter changes on the server connection #1299 alone, passes here.
• On a live pool with one server connection, the wire now shows the statement itself going to Postgres and a single RESET "search_path" when the connection is handed over — no synthesized rows, no RESET ALL.

Tests now cover both parser levels

@Bougerous reproduced this independently and pointed out that the test config
here pinned pgdog_leak to level = "on" — the one level that skips the regex
gate, so nothing exercised the path a statement takes at the default level.
That was a fair hit: set_config() is a function call inside a SELECT, it
matches no statement-start pattern, and on a single-primary cluster auto
leaves it to the gate, which drops it.

So the fixture is now two copies of the same single-primary database differing
only in parser level — pgdog_leak at on, pgdog_leak_auto at auto — and
every test runs against both.

A second test covers the half of this that reports nothing. 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
outlives its client makes the next one read as the previous tenant, with no
error at all. Reads go through a plain role, since the pooler's own user is a
superuser and superusers ignore RLS.

This leaves two tests failing at auto, and they stay that way until the
gate learns about set_config(). That fix belongs to @Bougerous, who found it
and offered the patch — it is one pattern in regex_parser.rs, and it is not
this PR's code (this PR doesn't touch that file). Merging his change first
turns both tests green here with nothing else to do.

[codecov]

Codecov Report

❌ Patch coverage is 92.64305% with 27 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pgdog/src/backend/pool/connection/binding.rs	43.75%	9 Missing ⚠️
pgdog/src/backend/server.rs	94.40%	8 Missing ⚠️
pgdog/src/frontend/client/query_engine/set.rs	81.81%	4 Missing ⚠️
...dog/src/frontend/router/parser/query/set_config.rs	91.66%	3 Missing ⚠️
.../src/frontend/router/parser/query/test/test_set.rs	97.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[levkk]

When set_config() arguments can't be resolved to constants

This is a bug in the parser. It should be able to extract Bind message arguments.

[IgorOhrimenko]

Thanks — done. The parser now pulls $n from the Bind message, so a parameterized set_config() is tracked like a constant one (the is_local argument is decoded from either wire format).

DirtyQuery is kept as a fallback for the cases the parser still can't resolve: no Bind message, a missing or non-text parameter, or an expression argument such as set_config('search_path', current_setting('search_path'), false).

Making those calls take the Command::Set path surfaced a second problem, fixed in the same commit: describing a portal that returns a row answered with NoData, so libpq rejected the synthesized reply with D message without prior T. Statements that return no rows now get NoData instead of an empty row description.

Tests added at three levels: parser (bound values, NULL, both fallbacks), protocol (the full extended-protocol exchange — fails without the fake.rs change), and an integration test that poisons the pool from one client and checks another, against a database with a single server connection so the reuse is deterministic.

[IgorOhrimenko]

Related: #1302 fixes a third way session state survives checkin — a client's SQL-level PREPARE (which pg_dump relies on) stayed on the connection and collided with the next client's statement of the same name.

[IgorOhrimenko]

The failing job here is ci (complex), and it is not this PR: integration/complex/shutdown.sh allows exactly one second for the process to exit after the shutdown scenario, and a loaded runner sometimes needs longer. The log shows the pooler closing every server connection, the check firing a second later, and the process exiting right after:

07:29:18.48  closing server connection: state=idle, reason=pool offline  (last one)
07:29:19.49  Shutdown failed
07:29:19.49  🐕 PgDog is shutting down immediately [SIGTERM]

The same job passed on this exact code six minutes earlier, and it has failed on main too (e37160da).

I also measured whether the extra test database this PR adds to integration/pgdog.toml slows shutdown down: three pairs of runs, same binary, config with and without it — 18/24, 26/31, 25/13 ms. The difference is noise.

#1303 replaces the fixed sleep with a poll, which should stop this from flipping.

[levkk]

Same comment as on #1299. We should not be using the "dirty" flag here. We should keep track of parameters and change them as needed for each client.

I think for this particular scenario it would be easier to:

1. Extract parameter from query
2. Forward the query as-is to Postgres - don't intercept / provide fake response
3. Record parameter change on connection

This is all that's needed for our existing logic to handle the parameter diff. For this to work, you may just need to add params to Command::Query and handle that in the query engine.

[IgorOhrimenko]

Done, both parts.

The parser reads $n from the Bind message now, so a parameterized set_config() resolves like a constant one (is_local decoded from either wire format).

And it is forwarded rather than intercepted: take a server, record the parameter change on that connection, send the statement, let Postgres answer. That removes the invented response along with its protocol bug — describing the portal claimed no rows and then sent one, so libpq rejected the reply with D message without prior T. Nothing left to fix there once we stop making the answer up.

This is stacked on #1299, which carries the recording mechanism; the diff here shows both, its last two commits are this PR.

Arguments that still don't resolve — no Bind, a non-text parameter, an expression — leave the statement an ordinary query, as today. Covering that case would need a way to record "this parameter is now something we can't name", and I didn't want to invent one here. Say the word if you'd rather it were handled.

[Bougerous]

I reproduced this independently against c2c6302 and wanted to share the results,
because they confirm the fix but turn up a gap it inherits.

Setup: pool_size = 1 so two clients provably share one server connection
(compared pg_backend_pid()), node-pg, PostgreSQL 18.4 with log_statement=all
so I could see what actually reached the server between checkouts. Baseline is
71167aa8 (main at the time), branch is c2c6302 (carries #1299).

#1299 fixes the mid-transaction SET leak. BEGIN; SELECT 1; SET app.x = 'v'; COMMIT
leaks on main and is clean on this branch, at every level and topology I tried.

#1298 fixes bind-parameter set_config wherever the statement reaches the
parser. On a cluster with a replica, set_config('app.x', $1, false) leaks on
main and is clean here. That part works.

The gap: SELECT set_config(...) only reaches QueryParser::set_config if
the regex fast path first decides the statement is worth parsing
(cluster.rs:605-620 → regex_parser.rs:62-79). That gate's pattern lists
(regex_parser.rs:12-28) have SET, RESET, BEGIN, COMMIT, LISTEN, … and
the pg_advisory_* family, but nothing for set_config. SET matches because
it's anchored at statement start; set_config is a function call inside a
SELECT, so it matches nothing and the statement never gets parsed at all.

Where that bites, stated precisely — it's topology-dependent, and I measured both
halves rather than assuming:

• At session_control / session_control_and_locks, set_config is never
  parsed, regardless of topology — those levels consult only the regex gate.
• At auto, the parser is forced on by router_needed() when the cluster has
  replicas or shards, so set_config is intercepted there. It falls through to
  the gate only for a single-shard, primary-only cluster (no read/write split, no
  multi_tenant, no dry_run, prepared_statements not "full").

Measured on this branch, single primary, no replicas:

case	this branch
set_config('app.x', 'v', false)	leaks at auto and scl
set_config('app.x', $1, false)	clean at on, leaks at auto and scl

Adding a replica to the same config flips the first row to clean, which is what
pointed me at router_needed().

This is pre-existing, not something this PR introduces — the PR touches 12 files
and regex_parser.rs isn't one of them.

I think the reason CI doesn't surface it is the new test config, which pins
pgdog_leak to level = "on":

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.

Sensible for determinism, but on is the one level that bypasses the gate, so
neither gate path gets exercised.

One pattern closes it:

/// `SELECT set_config(...)` changes session state exactly like `SET`, but it is
/// a function call inside a `SELECT`, so it never matches the statement-start
/// patterns above.
static CMD_SET_CONFIG: &[&str] = &[r"(?i)\bset_config\b"];

fn cmd_base_patterns() -> impl Iterator<Item = String> {
    CMD_BASE
        .iter()
        .map(|cmd| format!("{}{}", COMMENT_PREFIX, cmd))
        .chain(CMD_SET_CONFIG.iter().map(|s| s.to_string()))
}

Chaining into cmd_base_patterns() puts it in both CMD_RE and
CMD_RE_ADVISORY, so it covers session_control and auto as well as
session_control_and_locks — set_config is session control, not a lock, so the
base list is where it belongs. The \b matters: without it
SELECT offset_configuration FROM t matches. With this on top of the branch,
every case above is clean at all three levels and both topologies. regex_parser
is 17/17, and a full-suite run against an unmodified-main control produced an
identical failure set, so no new failures. fmt and clippy clean. Happy to open it
as a follow-up PR or just hand over the patch.

One note on priority. The 42P01 symptom in the description fails loudly. The
same mechanism has a silent mode — multi-tenant apps enforce tenant isolation with
RLS keyed on a custom GUC:

CREATE POLICY tenant_isolation ON patients
  USING (org_id = current_setting('app.current_org_id')::uuid);

and SELECT set_config('app.current_org_id', $1, ...) is the standard way to set
it. If that value survives a checkout, the next request evaluates RLS as the
previous tenant and returns their rows with no error at all. The topology where
the default level leaves that unparsed — unsharded, single primary — is also the
most common small production deployment.

Separately, and maybe worth a docs line: after a targeted RESET "app.x",
current_setting('app.x', true) returns '' rather than NULL, because RESET on
a placeholder GUC leaves an empty string. An RLS predicate written the obvious way
fails open on that. NULLIF(current_setting('app.current_org_id', true), '')::uuid
handles both spellings.

[IgorOhrimenko]

Thank you — this is the most useful thing anyone has done to this PR. Measuring
both topologies instead of assuming, and pinning it on router_needed() rather
than on the level alone, is what made the shape of it clear.

You were right about the test config, and that was the part I'd got wrong: I
pinned pgdog_leak to level = "on" for determinism and, in doing so, chose
the one level that never consults the gate. The tests looked thorough and
covered exactly the path that can't fail.

Fixed, and the fixture now says so: two copies of the same single-primary
database differing only in parser level — pgdog_leak at on,
pgdog_leak_auto at auto — with every test running against both.

I also added a test for the silent mode you described, because it deserved to
be in the suite rather than in a comment: a table with RLS keyed on
app.current_org_id, one client setting it through a bound parameter, and the
next client reading the table. When the GUC survives checkin that read returns
the previous tenant's rows and reports nothing. It needs a plain role — the
pooler's own user is a superuser, and superusers ignore RLS however the table
is configured, which cost me a first version of the test.

Both of those fail at auto on this branch, exactly as your table predicts,
and they stay red until the gate learns about set_config().

Which is why I'd rather not take your patch into this PR. You found it, you
measured it, and regex_parser.rs isn't a file this PR touches — please open
it as its own PR. I'll rebase on it and the two tests turn green with nothing
else to do. I did write the same pattern locally while checking your report and
arrived at your version character for character, which I take as a good sign
about the pattern rather than a reason to claim it.

Two things I found while confirming your results, both outside what that patch
can reach:

• PREPARE p(text) AS SELECT set_config('search_path', $1, false) followed by
  EXECUTE p('') leaks. EXECUTE carries no trace of what it runs.
• A function whose body calls set_config, and a DO block that does the
  same, both leak. The DO block leaks even though its text contains
  set_config and the gate lets it through — the parser sees the block and has
  nothing to record.

All three leak at on as well, so no parser level protects against them, and I
don't think any amount of pattern matching reaches them. I'll open a separate
issue with the measurements rather than pile it onto this PR. Two dead ends
worth recording there: the server doesn't report search_path or custom GUCs
in ParameterStatus, and custom GUCs don't appear in pg_settings at all, so
neither passive observation nor enumeration at checkin can see them.

And thank you for the NULLIF(current_setting(...), '') note — I hit exactly
that while writing the RLS test, and it's in the fixture with a comment saying
why.

[Bougerous]

On the two dead ends you listed for the PREPARE/EXECUTE, DO block and function-body cases — I measured both against PostgreSQL 18.4, and they split.

search_path is reported. It's in the server's GUC_REPORT set, announced in the startup burst and again on every change, including all three of the indirect routes:

REPORTED  direct SET (control)               ParameterStatus -> "public"
REPORTED  PREPARE + EXECUTE -> search_path   ParameterStatus -> "pg_catalog"
REPORTED  DO block -> search_path            ParameterStatus -> "information_schema"
REPORTED  function body -> search_path       ParameterStatus -> "public"

The full startup set is 15: application_name, client_encoding, DateStyle, default_transaction_read_only, in_hot_standby, integer_datetimes, IntervalStyle, is_superuser, scram_iterations, search_path, server_encoding, server_version, session_authorization, standard_conforming_strings, TimeZone.

So PREPARE p(text) AS SELECT set_config('search_path', $1, false) — your own example — does reach PgDog, through a channel it already reads.

Custom GUCs are not, and you're right about pg_settings. Silent for every form, direct or indirect:

SILENT  DO block -> custom app.* GUC       actual="x"
SILENT  function body -> custom app.* GUC  actual="y"

And with two app.* GUCs confirmed set and readable via current_setting(), pg_settings had 0 rows matching %.% out of 398 — placeholder GUCs never materialise there, so enumeration at checkin can't see them either. That half looks genuinely unreachable.

Why the search_path case still leaks despite being observable. server.rs:645-646 inserts every ParameterStatus into changed_params. cleanup_backend then copies those into the client's params:

if !changed_params.is_empty() {
    for (name, value) in changed_params.iter() {
        context.params.insert(name.clone(), value.clone());
    }

i.e. the change is recorded as the client's new intent rather than as state left on the connection. At the next checkout link_client opens with self.changed_params.clear() (server.rs:689) and then diffs the incoming client's params against self.client_params — which is only ever written from a client's declared params at line 714, never from changed_params. So the observation is made, filed against the wrong thing, and discarded before the diff that would have used it.

That's a code read, not a tested fix — I haven't tried closing it, and it's close enough to the parameter-tracking rework in your thread that it may already be covered by whatever shape that takes.

Splitting it that way: the reported GUCs look reachable with the existing machinery, and the custom ones need something else entirely.

I opened #1327 for the regex gate, so the auto cases have somewhere to land.