fix(postgres): introspect the session search_path, not a hardcoded 'public' - #40
fix(postgres): introspect the session search_path, not a hardcoded 'public'#40geekypunk wants to merge 5 commits into
Conversation
…ublic'
Every catalog query in PostgresIntrospectionProvider filtered on the literal
'public', and the Java side tagged every discovered object with a
DEFAULT_SCHEMA constant of the same value. Any database that keeps its tables
elsewhere was therefore completely invisible to DeepSQL.
The failure was silent and looked like success. On a dbt warehouse whose 37
tables live in `marts` and whose `public` holds nothing but extension views,
connection init ran all nine stages in 1.1 seconds and wrote
"All set! Brain is ready." at 100% — having produced an empty schema snapshot
("tables":[], totalTables 0), zero rag_documents, zero column profiles and zero
semantic models. Nothing errored, because nothing was found to process.
All 16 predicates now filter on current_schema(), and the schema tags come from
a resolveSchema(Connection) helper reading the same value, so the target schema
becomes a property of the connection — set it on the role, or with the JDBC
currentSchema parameter — instead of a compile-time constant. It falls back to
'public' when current_schema() cannot be read, so existing connections behave
exactly as before.
getDefaultSchema() deliberately keeps returning 'public'. Its four callers
(SchemaIntrospectionService, PgVectorSearchService, AzureSearchService,
TrainingService) use it to decide whether a name needs qualifying; returning
the session schema there would strip the qualifier off precisely the names that
need it, turning marts.dim_person into a bare dim_person.
Verified against a live 25GB warehouse: the same connection that previously
snapshotted 0 tables now snapshots 37 tables and 837 columns, and init proceeds
through real data sampling instead of completing instantly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Postgres ships `search_path = "$user", public` everywhere, RDS and Aurora included, and the "$user" entry is inert only while no schema matches the connecting role's name. Create one — per-tenant layouts, or the per-user pattern the Postgres docs recommend and which spread after PG15 hardened `public` — and current_schema() silently becomes that schema. Verified on a stock instance: SHOW search_path; -> "$user", public SELECT current_schema(); -> public CREATE SCHEMA postgres; -- schema named after the connecting role SELECT current_schema(); -> postgres So a connection that had been reading `public` can start reading an empty user schema and report a healthy, empty brain — the exact failure the search_path change was written to fix, inverted. resolveSchema()'s fallback does not catch it, because current_schema() returned a perfectly valid schema. Two guards, both diagnostic only: - announceSchema() logs the schema at INFO when it is not `public`, and DEBUG when it is, so the unchanged historical case stays quiet while a switch is always on the record. Called from getDatabaseObjects and scanSchema rather than from resolveSchema, which runs once per table via getTableColumns — a log there would emit a line per table. - warnIfEmptyWhilePublicHasTables() fires on the fingerprint of an accidental "$user" match: nothing found in the resolved schema while `public`, where this provider used to look unconditionally, still holds tables. It names the ALTER ROLE ... SET search_path remedy. Best-effort; a failed count never breaks introspection. Default RDS/Aurora installs are unaffected either way: with no role-named schema current_schema() is `public`, exactly as before.
…gain
resolveSchema() issues `SELECT current_schema()` on its own Statement before
each method's real query. Seven tests predated that call and broke: those
stubbing only prepareStatement got the unstubbed-mock default null from
createStatement()
NullPointerException: Cannot invoke "java.sql.Statement.executeQuery(String)"
because "stmt" is null at resolveSchema(...:797)
and those stubbing createStatement handed resolveSchema the shared ResultSet,
so its getString(1) tripped strict stubbing against getString("tablename").
resolveSchema() is the first createStatement() caller in every method that
reaches the database, so returning a dedicated schemaStatement first and the
shared statement afterwards routes each to the right place. It answers "public",
keeping these fixtures on the historical schema so they go on asserting the
behaviour they were written for rather than the search_path change itself.
setUp() uses lenient() because the pure-Java tests (getDatabaseType,
getDefaultSchema) never touch the Connection and strict stubbing would fail them
over an unused stub. Three tests re-stub createStatement locally and had to
prepend schemaStatement themselves; getForeignKeys_returnsRelationships is left
alone because getForeignKeys does not call resolveSchema.
11/11 pass.
✅ Tests green + Aurora/RDS safety guards added
Does this break existing Aurora / RDS connections?Tested against a real Postgres. No, not for a default install — with one specific exception:
Guards added (
|
…umns
The PK subquery joined tc.constraint_name = ku.constraint_name and filtered on
ku.table_name alone, with no schema predicate anywhere. Postgres auto-names
primary keys "<table>_pkey", so two schemas holding a same-named table collide
by construction — the join cross-matches them.
Measured against two schemas each with an `orders` table (s_a's PK is `other`,
s_b's is `name`), asking for s_b.orders:
column_name | is_primary_key
name | t
name | t <- duplicated row
other | t <- false positive: `other` is s_a's PK, not s_b's
other | t <- duplicated row
Ground truth from pg_index: s_b.orders has exactly one PK column, `name`. So the
provider reported every column as a primary key and returned each one twice —
duplicate ColumnInfo entries, and a table with no real single PK. Adding
tc.table_schema = ku.table_schema and ku.table_schema = ? returns the correct
`name | t`, `other | f`.
This predates the search_path change and was mostly latent while the provider
only ever read `public`: a collision needed some other schema to hold a
same-named table. Now that it targets whatever the session resolves to, that is
no longer the exception — staging/marts/public each holding `orders` or
`customers` is the normal shape of the dbt warehouse this change was made for.
So the earlier commit does not introduce this bug, but it does move it from
unlikely to expected, which is why it is fixed on the same branch.
Also hoists resolveSchema() out of the argument list: this method runs once per
table, and each call costs a round-trip.
Tests: 12/12. A mocked ResultSet cannot exercise SQL semantics, so the new test
asserts the two schema predicates are present and that every placeholder is
bound — enough to stop the qualification being dropped again. The correctness
evidence is the measurement above, run against Postgres 18.
Second review — found a third defect, fixed on this branch (
|
Honouring current_schema() unconditionally changed behaviour for every Postgres connection in order to fix the subset that keeps tables outside `public`. That is the wrong trade, because the risk is not hypothetical: Postgres ships `search_path = "$user", public` everywhere, RDS and Aurora included, and the leading "$user" is inert only while no schema matches the connecting role's name. SHOW search_path; -> "$user", public SELECT current_schema(); -> public CREATE SCHEMA postgres; -- named after the connecting role SELECT current_schema(); -> postgres Per-tenant layouts, and the per-user pattern the Postgres docs recommend and which spread after PG15 hardened `public`, make that live. An untouched connection would then read an empty user schema and report a healthy, empty brain — the failure this feature exists to fix, inverted. The warning added earlier makes that visible after the fact; it does not prevent it. resolveSchema() now reads search_path and current_user alongside current_schema(), and ignores a schema that was selected by the implicit "$user" entry: current_schema() equal to current_user while search_path is still exactly what Postgres ships. Such a connection introspects `public`, bit for bit as before. The blast radius is now the connections that asked for this. Nothing changes unless an operator sets a search_path — `ALTER ROLE … SET search_path`, or the JDBC currentSchema parameter — which is how the reported warehouse enabled it, so that case is unaffected. The check is deliberately narrow: it matches the shipped default exactly. A path of `"$user", marts` states an intent and is honoured, verified by test. No new configuration: DatabaseConnection carries no schema field, so a config route would mean entity, DTO, API and UI. The database-side search_path is already the opt-in. Tests: 16/16, covering the implicit "$user" match, a configured path, "$user" inside a non-default path, and a NULL current_schema().
Opt-in guard added (
|
search_path |
current_schema() |
Result |
|---|---|---|
"$user", public (untouched default) |
= current_user |
public — implicit, ignored |
"$user", public (untouched default) |
public |
public — unchanged |
marts, public (configured) |
marts |
marts — honoured |
"$user", marts (configured) |
= current_user |
honoured — a stated intent |
| names only missing schemas | NULL |
public |
An operator who never set a search_path gets the historical behaviour bit for bit. Not "probably fine" — structurally. The blast radius is now exactly the connections that asked for this, and the reported warehouse enabled it with ALTER ROLE … SET search_path = marts, public, so it is unaffected.
The check matches the shipped default exactly, so "$user", marts is treated as deliberate and honoured (covered by test).
No new configuration: DatabaseConnection has no schema field, so a config route would mean entity + DTO + API + UI. The database-side search_path already is the opt-in.
Branch state
16/16 tests pass. Four commits on top of the original, which is untouched:
cff1e73 |
the original patch — authorship and message preserved |
f5196eb |
INFO log on a non-public schema; WARN when it is empty while public is not |
2a23bc4 |
fixture repair for SELECT current_schema() |
f63eea0 |
schema-qualify the PK subquery (fixed false primary keys + duplicate columns) |
05ddf85 |
this guard |
## Problem
Pull requests were sitting on a required check that could never arrive:
```
analyze (${{ matrix.language }}) Expected — Waiting for status to be reported [Required]
```
Note the name: the **raw, un-interpolated template**. That is not a
check that failed — it is a check nothing will ever report.
## Root cause
`codeql.yml` guarded the `analyze` job with:
```yaml
if: github.event.repository.visibility == 'public'
```
A job-level `if:` is evaluated **before the matrix expands**. So when
the job is skipped, GitHub emits a *single* check run under the literal
`name:` template rather than the two expanded names. While this
repository was private, every PR reported exactly one CodeQL check,
named `analyze (${{ matrix.language }})`, conclusion `skipped` — still
visible on the older open PRs (#40, #36).
That phantom name was the only CodeQL check anyone had seen, so it was
pinned as a required status check in the `Protect main branch` ruleset —
GitHub's suggestion list offers whatever was last reported.
When the repository went public, the job started running for real and
reporting `analyze (java-kotlin)` and `analyze (javascript-typescript)`.
The required phantom was left with nothing to satisfy it, and every PR
became unmergeable with no failing job to point at.
## Fix
Remove the guard. Its own comment named the condition for deleting it —
*"DELETE THIS LINE once the repository is public"* — and that condition
is now met: the repository is public and code scanning is free.
The replacement comment records the failure mode so the `if:` is not
reintroduced, and directs a future private-repository scenario to the
ruleset instead of a job condition.
## Test plan
- [ ] CodeQL runs on this PR and reports **`analyze (java-kotlin)`** and
**`analyze (javascript-typescript)`** — not the template name
- [ ] Both legs pass, satisfying the two required contexts already
configured in the ruleset
- [ ] No new `if:` remains on the job (the two matches in the file are
inside comments)
- [ ] Watch the Monday `27 4 * * 1` cron: scheduled runs on Aug 3 and
Aug 10 both reported `skipped`, correctly, since the repository was
private then. Whether `github.event.repository.visibility` is even
populated on `schedule` events was never tested — removing the guard
makes it moot, and the next cron should now produce a real scan.
## Notes
- The `Protect main branch` ruleset has already been corrected
separately; it now lists only the two expanded contexts. This PR removes
the thing that generated the bad name in the first place.
- Older PRs created while the repository was private still carry the
stale skipped check and report `UNKNOWN` mergeability. A rebase or any
push forces GitHub to recompute them against the corrected ruleset.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`getTablesAndViews()` passes the schema each table was found in, but
`getTableColumns` bound `DEFAULT_SCHEMA` (`'public'`) and discarded the
argument. Every table on a database whose objects live outside `public`
came back with **zero columns**.
Measured against a real dbt warehouse (195 tables across 17 schemas,
nothing in `public`):
```
marts.dim_person, schema bound to 'public' -> 0 columns
'marts' -> 91 columns
```
The PK subquery also had no schema predicate at all. Postgres auto-names
primary keys `<table>_pkey`, so two schemas holding a same-named table
cross-match by construction — producing duplicated `ColumnInfo` rows and
false-positive PK flags. This adds `tc.table_schema = ku.table_schema`
and `ku.table_schema = ?`.
Both problems were latent while the provider only ever read `public`.
#55 made them live: now that introspection walks every non-system
schema, a `staging`/`marts`/`public` collision on `orders` or
`customers` is the normal shape of a dbt warehouse, not the exception.
## Verification
Deployed and measured on a live 195-table warehouse:
| | before | after |
|---|---:|---:|
| schema snapshot | 37 tables / 837 cols | **195 / 4,070** |
| tables returning zero columns | 195 of 195 | **0 of 195** |
| tables with correctly-detected PKs | — | 67 |
| tables with duplicated column rows | — | 0 |
| tables with every column flagged PK | — | 0 |
A full brain re-init on that connection went from a 37-table snapshot to
195 tables / 4,070 columns, lifting `table_classification` 25 → 161 and
`inferred_table_relationship` 17 → 280. The agent went from being blind
to 14 of 18 schemas to correctly describing them.
Unit tests: `PostgresIntrospectionProviderTest` passes. A mocked
`ResultSet` can't exercise SQL semantics, so the correctness evidence is
the measurement above, against Postgres 18.
## Relationship to #40
#40 is open and `CONFLICTING`. It fixed the same class of problem with a
`current_schema()` approach that #55 superseded by scanning all
non-system schemas. This PR sits on top of #55 instead and is
independent of #40 — #40 can likely be closed once this lands.
Co-authored-by: deepsql-deploy <venkatesh.sakamuri@stayflexi.com>
1 participant
Every catalog query in PostgresIntrospectionProvider filtered on the literal
'public', and the Java side tagged every discovered object with a
DEFAULT_SCHEMA constant of the same value. Any database that keeps its tables
elsewhere was therefore completely invisible to DeepSQL.
The failure was silent and looked like success. On a dbt warehouse whose 37
tables live in
martsand whosepublicholds nothing but extension views,connection init ran all nine stages in 1.1 seconds and wrote
"All set! Brain is ready." at 100% — having produced an empty schema snapshot
("tables":[], totalTables 0), zero rag_documents, zero column profiles and zero
semantic models. Nothing errored, because nothing was found to process.
All 16 predicates now filter on current_schema(), and the schema tags come from
a resolveSchema(Connection) helper reading the same value, so the target schema
becomes a property of the connection — set it on the role, or with the JDBC
currentSchema parameter — instead of a compile-time constant. It falls back to
'public' when current_schema() cannot be read, so existing connections behave
exactly as before.
getDefaultSchema() deliberately keeps returning 'public'. Its four callers
(SchemaIntrospectionService, PgVectorSearchService, AzureSearchService,
TrainingService) use it to decide whether a name needs qualifying; returning
the session schema there would strip the qualifier off precisely the names that
need it, turning marts.dim_person into a bare dim_person.
Verified against a live 25GB warehouse: the same connection that previously
snapshotted 0 tables now snapshots 37 tables and 837 columns, and init proceeds
through real data sampling instead of completing instantly.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com