fix(dbconn): strip explicit pg_catalog from pooled search_path

[Kiran01bm]

Removes a pg_catalog entry from every pooled session's search_path when a user schema is listed ahead of it, so that schema no longer shadows the catalog, while every other entry is left as configured. A leading or sole pg_catalog is left alone, and every transaction-local search_path pg-sprite sets is built through the same rewrite.

Why

PostgreSQL searches pg_catalog implicitly before every search_path entry unless the path names it explicitly. Then it is searched at that position, and a schema listed earlier shadows catalog names. A role or database configured with search_path = app, pg_catalog made every pooled session resolve unqualified pg_class to app.pg_class: a decoy table could turn an introspection or preflight read into a confidently wrong answer. Most catalog reads in pkg/preflight and pkg/schemadiff already qualify with pg_catalog., but the connection layer is where the guarantee belongs, so a future unqualified read cannot reintroduce the hole. The guarantee is now registered as invariant CO-9 — Decision reads resolve to the real catalog, upheld at two layers: the connection layer rewrites the path, and reads that run under a path pg-sprite did not set qualify every catalog name.

Stripping the explicit entry, rather than prepending pg_catalog, is deliberate: prepending would make current_schema() return pg_catalog, so unqualified CREATE TABLE and migrate's target resolution (which resolves unqualified names through the session search_path on purpose) would change meaning. Removing the entry restores PostgreSQL's implicit-first rule and touches nothing else.

What

• pkg/dbconn: new AfterConnect hook unshadowCatalog runs once per physical connection. It reads SHOW search_path, removes every pg_catalog entry (bare, case-insensitive; or double-quoted, exact, with "" as the escaped quote; commas inside quotes are respected) that has a non-catalog entry before it, and calls pg_catalog.set_config('search_path', …, false) only when something was removed. Paths without a shadowed entry cost one SHOW and no write. The creation schema changes only when every entry ahead of the removed pg_catalog names a nonexistent schema; it becomes the next existing entry, or NULL when there is none, in which case an unqualified CREATE fails and preflight reports ErrNoCreationSchema.
• dbconn.LocalSearchPath(schemas ...string) builds the SET LOCAL search_path statement for a transaction: identifiers sanitized, the same rewrite applied. schemadiff/introspect.go, schemadiff/desired.go, and executor/optimistic.go use it; a test walks the production sources under pkg/ and fails if any other file sets search_path.
• docs/invariants.md gains CO-9 and a Build-phase row; SAFETY.md lists it for pkg/dbconn and pkg/executor; the pg_catalog.-qualification comments in pkg/executor, pkg/progress, and pkg/schemadiff cite it. NewPool godoc and the README connection paragraph state the behavior.
• Tests: table-driven unit tests over the path rewriter (leading, sole, and repeated pg_catalog; quoted commas on a rewritten path; empty entries) and over LocalSearchPath; integration test on a throwaway database (ALTER DATABASE … SET search_path) that creates a decoy pg_class and a decoy set_config in a schema listed before pg_catalog, then asserts the pooled session's path has the entry removed, current_schema() is unchanged (or NULL when only a missing schema is ahead), and count(*) FROM pg_class reads the real catalog, plus unchanged-path cases for pg_catalog, decoy and pg_catalog alone; buildPoolConfig test asserts the hook is installed.
• internal/testutil.NewCatalogShadowingPool: a raw pgxpool with a shadowing search_path for the executor and schemadiff tests that prove catalog reads resist shadowing; those tests would otherwise be disarmed by the hook.

Scope: the guarantee is per session. Only an explicit pg_catalog entry is rewritten, not the implicit pg_temp search ahead of it — pg-sprite creates no temporary objects, so nothing it runs can populate a pg_temp that shadows its own session. The hook is a runtime set_config on the server connection, so behind a transaction-mode pooler (for example PgBouncer in transaction mode) the rewritten path may outlive the client that triggered it and be seen by the next one; the value it leaves behind is the stricter one, and that deployment already has the same limitation for lock_timeout and statement_timeout.

Before / after

search_path = app, pg_catalog is configured on the database, app.pg_class is a one-row decoy, and preflight runs SELECT count(*) FROM pg_class:

Before                                     After

search_path = app, pg_catalog              search_path = app, pg_catalog
             │                                          │
             ▼                                          ▼
┌─────────────────────────┐                ┌─────────────────────────┐
│ pooled session          │                │ AfterConnect            │
│ (path used as-is)       │                │ strips explicit entry   │
└────────────┬────────────┘                └────────────┬────────────┘
             │                                          │ search_path = app
             ▼                                          ▼
┌─────────────────────────┐                ┌─────────────────────────┐
│ lookup: pg_class        │                │ lookup: pg_class        │
│ app.pg_class  ◀── hit   │                │ pg_catalog (implicit)   │
│ pg_catalog.pg_class     │                │  └─ pg_class  ◀── hit   │
└─────────────────────────┘                │ app.pg_class (shadowed) │
                                           └─────────────────────────┘
count(*) = 1  (decoy)                      count(*) = 400+  (catalog)
current_schema() = app                     current_schema() = app

introspect tx:                             introspect tx:
SET LOCAL search_path = "app", public      dbconn.LocalSearchPath("app", "public")
(built at the call site)                   (same rewrite; the only builder under pkg/)

🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Reviewed 3f47c51f..0d868f18 (9 files, +245/−31) end to end, ran a mutation pass over pkg/dbconn/search_path.go (18 mutants), and probed each PostgreSQL claim against a live server. Comment 1 of 2 — correctness; comment 2 covers tests, docs, and the pooler caveat.

The core decision is right and the reasoning in the description is the part I'd keep verbatim. Stripping the explicit entry rather than prepending pg_catalog is the only option that leaves current_schema() alone, and I checked the consequence you don't spell out: because a user table can never live in pg_catalog, removing the entry cannot change how pkg/migrate's unqualified target resolution lands. It only changes which schema wins for names that also exist in the catalog — which is the whole point. Keeping a leading pg_catalog is likewise load-bearing rather than decorative, and the mutation pass confirms it: forcing the strip unconditionally is killed by leading_catalog_shadows_nothing and catalog_only. Failing the AfterConnect hook closed (pgx discards the connection) is the right disposition for a guarantee like this.

Eleven of eighteen mutants died. Three of the five survivors are worth acting on.

1. pg_catalog.set_config is load-bearing, and nothing pins it

search_path.go:42 qualifies the write:

conn.Exec(ctx, "SELECT pg_catalog.set_config('search_path', $1, false)", unshadowed)

Dropping the qualification survives the whole suite. It should not, because at the moment this line runs the path is still shadowed — that is the entire premise of the hook — so an unqualified set_config resolves through the very schema the hook exists to defuse. On a live server:

CREATE FUNCTION decoy.set_config(text, text, boolean) RETURNS text
  LANGUAGE sql AS $$ SELECT 'decoy-ran-and-changed-nothing' $$;
SET search_path = decoy, pg_catalog;

SELECT set_config('search_path', 'public', false);              -- decoy-ran-and-changed-nothing
SELECT pg_catalog.set_config('search_path', 'public', false);   -- public

The unqualified call returns the decoy's string and the path is unchanged, so an attacker who can create a table in the leading schema — the exact adversary in the description — can also create a three-argument set_config there and turn the hook into a silent no-op that reports success. SHOW is a utility statement and is immune, so the qualification on :42 is the only thing standing between this hook and being disarmed by the condition it detects.

That is a deliberate, correct detail, and it deserves the one test that keeps it: the integration case can create decoy.set_config(text, text, boolean) alongside decoy.pg_class and assert the path still comes back rewritten. Without it the qualification is a comment-free convention that the next refactor drops.

2. The quoted-comma case is tested on the one path where the split result is discarded

search_path.go:99 splits on commas outside quotes, and search_path_test.go:26 is clearly there to cover it:

{name: "comma inside a quoted entry is not a separator", path: `"a, pg_catalog", public`, want: `"a, pg_catalog", public`},

Removing && !inQuotes from :99 survives that case. It survives for two independent reasons, and both are worth seeing:

• wantChanged is false, so withoutShadowedCatalog takes the return path, false early exit at :75 and hands back the input string verbatim. Nothing the splitter produced is ever observed.
• Even if it were observed, the naive split yields "a / pg_catalog" / public, and pg_catalog" is neither a bare pg_catalog nor a well-formed quoted entry, so namesCatalog rejects it anyway. The pg_catalog text planted inside the quoted name cannot be mis-detected even by the splitter the case is written to catch.

The failure the case is reaching for is real and needs a path that actually gets rewritten, with no space after the embedded comma so the ", " join can't reconstruct the input:

{name: "comma inside a quoted entry is not a separator", path: `"a,pg_catalog", decoy, pg_catalog`, want: `"a,pg_catalog", decoy`, wantChanged: true},

Correct code returns "a,pg_catalog", decoy; the mutant returns "a, pg_catalog", decoy, which names a different schema. Same masking applies to dropping empty entries in splitSearchPath — it also survives, and also only shows up on a path that gets rewritten.

3. pg_temp still shadows the catalog, so the absolute claims overshoot

dbconn.go:85 says "catalog names always resolve to the catalog", and the README says the schema "can never shadow the catalog". PostgreSQL searches the temp schema ahead of the implicit pg_catalog, so a path naming neither is enough:

SET search_path = public;                    -- names neither pg_temp nor pg_catalog
CREATE TEMP TABLE pg_class (oid oid);
INSERT INTO pg_class VALUES (1);

SELECT count(*) FROM pg_class;               -- 1
SELECT count(*) FROM pg_catalog.pg_class;    -- 1187

SET search_path = public, pg_temp;           -- pin it last
SELECT count(*) FROM pg_class;               -- 1187

Calibrating this honestly: pg_temp is session-private and pg-sprite creates no temporary objects anywhere in the tree, so there is no live hole here — this is about what the guarantee is allowed to claim, not about a reachable attack. But "always" and "never" are the words this PR chose, and the hook is precisely the place where PostgreSQL's own CREATE FUNCTION guidance says to pin pg_temp last. Two ways out, and I'd take either: soften both sentences to name the explicit-entry case they actually cover, or append pg_temp to the rewritten path. The second is stronger but has a cost worth stating rather than absorbing — to make it a real invariant it would have to run on every connection, which turns the "one SHOW and no write" fast path into a write on every connection.

4. The session guarantee stops at the three transaction-local overrides

the connection layer is where the guarantee belongs, so a future unqualified read cannot reintroduce the hole

Three sites replace the session path wholesale for the duration of a transaction:

• pkg/schemadiff/introspect.go:58 — SET LOCAL search_path = <schema>, public
• pkg/schemadiff/desired.go:45 — SET LOCAL search_path = <scratch>, public
• pkg/executor/optimistic.go:263 — SET LOCAL search_path = <schema>, public

Inside those transactions the hook's rewrite is not in effect. They are safe today, but for their own reason — none names pg_catalog, so the implicit-first rule applies — not because of anything this PR adds. introspect.go:51-54 already leans on exactly that ("Its catalog queries can name pg_class and = unqualified because the search_path they run under is set by this function"), which is the same argument, made per site. So a future unqualified read can reintroduce the hole: it only takes one of these three constructing a path that names pg_catalog, and nothing at the connection layer would notice.

The cheap version of closing it is to route those three through the same rewriter — they build the string anyway — or, if that reads as overkill, to say in the description that the guarantee is per-session and the transaction-local paths uphold it separately. What I'd avoid is the current phrasing, which reads as though the connection layer now covers the whole surface.

---

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Comment 2 of 2 on 3f47c51f..0d868f18 — tests, docs, the pooler caveat, and the invariant this change earns. Comment 1 covers correctness.

5. The one documented behavior change is the one case the test cannot express

search_path.go:29-32 is careful to call out the single user-visible side effect:

The creation schema (current_schema()) changes only when every entry ahead of a removed pg_catalog names a schema that does not exist: it was pg_catalog, where unqualified CREATE is refused anyway, and becomes the next existing entry.

Two things about that sentence. First, when there is no next existing entry it does not become one — it becomes NULL:

SET search_path = no_such_schema;
SELECT current_schema() IS NULL;   -- t

So for search_path = no_such_schema, pg_catalog the creation schema goes from pg_catalog to NULL, and that lands in territory the codebase already models: pkg/preflight/absent.go:33's ErrNoCreationSchema, handled at absent.go:132 and create.go:73. Both the before and after refuse an unqualified CREATE, so nothing breaks — but the refusal changes from PostgreSQL's own "cannot create in pg_catalog" to pg-sprite's, and the sentence as written does not predict that.

Second, this is the only behavior change the PR documents and there is no case for it. It cannot be added as-is either: dbconn_integration_test.go:56 scans into var gotPath, creationSchema string, so a NULL creation schema fails the scan rather than asserting the documented outcome. Making the case expressible means a *string (or a wantNoCreationSchema bool), which is a small change and turns a prose claim into a checked one. The five existing cases all cover paths whose creation schema is unchanged, so the assertion at that line is currently proving the absence of the effect and never its presence.

6. The pooler caveat is right that the setting does not follow the client, and understates where it goes instead

Behind a transaction-mode pooler (for example PgBouncer in transaction mode) session settings do not follow the client across server connections, and this hook does not attempt to; that deployment already has the same limitation for lock_timeout and statement_timeout.

The comparison is not quite like for like, and the difference runs the other way from what the sentence implies.

lock_timeout and statement_timeout are set as startup parameters (buildPoolConfig, dbconn.go:181-182 writes them into ConnConfig.RuntimeParams), so they ride the startup packet. This hook is the first runtime session-scoped write in the pool: set_config(..., false) outside a transaction, after connect. Under transaction pooling that statement is its own transaction, so it lands on whichever server connection the pooler assigns and is then released — and PgBouncer does not run server_reset_query in transaction mode by default (server_reset_query_always = 0), so the rewritten path stays on that server connection and the next client to be assigned it inherits it.

The blast radius is small and worth saying so: the value that leaks is the unshadowed path, so the recipient is strictly better defended than before, and only a client deliberately relying on a user schema shadowing the catalog would notice. But "does not follow the client" and "is left behind for someone else" are different statements, and the caveat currently makes only the first. One clause covers it.

7. Two survivors that are fine as-is, and one test whose name promises more than it checks

• The strings.ReplaceAll unescape at search_path.go:112 is provably inert for this predicate: the comparison target pg_catalog contains no quote, so unescaping can never change whether unquoted == catalogSchema. Dropping it survives, which is correct rather than a gap. Worth knowing because search_path_test.go:27 — "escaped quote inside a quoted entry" — reads like the case that covers it and does not; it passes with the unescape removed. The name is the misleading part, not the code. If the general unquoting is meant as future-proofing against catalogSchema ever changing, a line saying so costs less than the next reader re-deriving it.
• The len(entry) >= 2 guard at :111 also survives, and I could not construct a reachable input that needs it: SHOW search_path re-quotes to a balanced form, so even a deliberately hostile setting comes back well formed — SET search_path = 'a,"' prints as "a,""", which the splitter handles correctly and whose unescape recovers a," exactly. The guard prevents a slice panic on a lone " that the server will not emit. Belt and braces, and I would keep it; just noting that the mutation gap is unreachability, not a missing test.

8. This change earns an invariant entry

docs/invariants.md has no entry governing catalog reads, and the reason it is worth adding one now is that this PR is what turns a per-site convention into a systemic property. The rule is currently asserted in four separate hand-written comments, each re-deriving it:

• pkg/executor/native.go:792 — operators carry explicit pg_catalog qualification because search_path may shadow
• pkg/progress/progress.go:229 — a schema ahead of pg_catalog could substitute a relation
• pkg/schemadiff/managed_tables.go:43 — qualified because a path may list a user schema ahead of pg_catalog
• pkg/schemadiff/introspect.go:51-54 — unqualified reads are permitted only because the function sets the path itself

Four statements of one rule, with no single place that says it and nothing that fails when a fifth site forgets. An entry in the CO family — every read pg-sprite uses to make a decision resolves to the real catalog, upheld at the connection layer by unshadowCatalog and at each transaction-local path by construction — would give the four comments something to cite and give finding 4 in comment 1 a home. A completeness test over the sites, rather than a hand-maintained list, is the version that keeps working.

9. What I'd keep unchanged

• internal/testutil.NewCatalogShadowingPool is the right shape for a helper that exists to keep other tests armed, and postgres.go:116's require.Equal(..., "the shadowing search_path must survive connect") is the part I'd point other people at: a helper whose whole purpose is to preserve a hazard should assert the hazard survived, or the tests it feeds pass for the wrong reason. postgres.go:105 saying outright that it carries none of pkg/dbconn's session defaults is the second half of that discipline.
• Writing only when something was removed, so the common path costs one SHOW and no round trip for the write.
• withoutShadowedCatalog returning the input string verbatim when nothing changed, rather than a re-joined equivalent — it means an untouched path is byte-identical to what the operator configured, which is what makes the wantChanged: false cases meaningful. (It is also what masks finding 2, so the fix there is a new case, not a change to this behavior.)
• Case handling split by quoting — bare entries case-folded, quoted entries exact — is right, and both halves are killed by their own cases (bare identifier is case-folded, quoted upper-case is a different schema).

None of the above blocks. Findings 1 and 2 are the two I would land before merge; the rest are precision.

---

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving. The core decision — stripping the explicit pg_catalog entry rather than prepending it — is the only variant that leaves current_schema() and pkg/migrate's target resolution alone, keeping a leading pg_catalog is load-bearing and tested, and the hook fails closed. Eleven of eighteen mutants died; the two survivors worth landing before merge are the untested pg_catalog.set_config qualification and the quoted-comma case that only exercises the unchanged-path return. Details in the two comments above.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/93, follow-up commit

All nine findings addressed in 4b30d16; the three SET LOCAL search_path sites now route through dbconn.LocalSearchPath, and CO-9 is registered.

#	Finding	Status	Explanation
1	pg_catalog.set_config qualification is unpinned	Fixed	The integration setup now also creates decoy.set_config(text, text, boolean) returning 'decoy'. With the hook un-qualified, "catalog after user schema" fails: the path stays decoy, pg_catalog and count(*) FROM pg_class reads 1. Confirmed by mutation.
2	Quoted-comma case only exercises the unchanged path	Fixed	Added "a,pg_catalog", decoy, pg_catalog → "a,pg_catalog", decoy (changed) and decoy, , pg_catalog → decoy (changed). Removing && !inQuotes now fails the first; the second pins the empty-entry drop on a rewritten path.
3	"always resolve to the catalog" overshoots — pg_temp	Fixed	NewPool godoc and the README now claim only that a schema listed ahead of an explicit pg_catalog no longer shadows it. The unshadowCatalog godoc states pg_temp is out of scope: pg-sprite creates no temporary objects, so nothing it runs can populate a pg_temp that shadows its own session. Decision: soften the claim rather than extend the rewrite to pg_temp.
4	Three SET LOCAL search_path sites bypass the rewriter	Fixed	New exported dbconn.LocalSearchPath(schemas ...string) sanitizes each identifier, applies withoutShadowedCatalog, and returns the SET LOCAL statement. schemadiff/introspect.go, schemadiff/desired.go, and executor/optimistic.go call it. TestLocalSearchPathIsTheOnlySearchPathWriter walks every non-test .go under pkg/ outside pkg/dbconn and fails on any SET [LOCAL] search_path, set_config('search_path', or RuntimeParams["search_path"], so a fourth site cannot appear unnoticed. The PR body now states the guarantee is per session.
5	Creation schema can become NULL, not "the next existing entry"	Fixed	Doc sentence now says "or NULL when there is none — an unqualified CREATE then fails with no schema has been selected, which preflight reports as ErrNoCreationSchema". Integration case no_such_schema, pg_catalog asserts path no_such_schema and current_schema() IS NULL (scanned into *string).
6	Pooler caveat is incomplete	Fixed	PR body and the hook comment now say the write is a runtime set_config on the server connection, so under transaction pooling the rewritten path can outlive the client that triggered it; the value left behind is the stricter one.
7	"escaped quote" case name and namesCatalog unquoting	Fixed	Case renamed to "quoted entry with an escaped quote is not the catalog". namesCatalog comment explains the "" unescape is general unquoting (inert for pg_catalog, correct if the target changes) and that the length guard covers a lone quote the server never emits.
8	No invariant entry for "decision reads resolve to the real catalog"	Fixed	Added CO-9 to docs/invariants.md (main already holds CO-8 for TOAST): two layers — connection (unshadowCatalog, LocalSearchPath) and per-query pg_catalog. qualification — plus the per-session scope, and a Build-phase row. SAFETY.md rows for pkg/dbconn and pkg/executor list it. The four qualification comments (native.go, progress.go, managed_tables.go, introspect.go) cite CO-9; optimistic.go and desired.go cite it at their LocalSearchPath calls.
9	Nothing else to change	—	No change.

Decisions to veto: CO-9 is registered as a Correctness invariant rather than a Refusals one, since it protects the answer of a read rather than a refusal path. LocalSearchPath quotes public as "public", which PostgreSQL treats identically to the bare name.

Source: #93, review comment 5610284689 and review comment 5610285291 at head 0d868f1; scratch review scratch/code-reviews/pg-sprite-pr93-review.md