Skip to content

fix(sync): install pg_stat_statements into a named schema and verify through the resolver - #237

Closed
veksen wants to merge 3 commits into
mainfrom
fix-pg-stat-statements-schema
Closed

fix(sync): install pg_stat_statements into a named schema and verify through the resolver#237
veksen wants to merge 3 commits into
mainfrom
fix-pg-stat-statements-schema

Conversation

@veksen

@veksen veksen commented Aug 28, 2026

Copy link
Copy Markdown
Member

Goal

A user following the CI guide should be able to run their migrations afterwards. Today installing pg_stat_statements breaks any project whose migration tool reconciles public, and the failure surfaces as unrelated broken tests. Closes Query-Doctor/Site#4199 on the analyzer side; the docs half and the other producers of the same state are in Query-Doctor/Site#4202.

What

Before: POST /postgres/extensions/pg_stat_statements ran a bare CREATE EXTENSION pg_stat_statements, which lands the extension in public. The extension owns pg_stat_statements_info there and will not let a migration tool drop it, so drizzle-kit push --force aborts half-applied with SQLSTATE 2BP01. Nothing at the point of failure names the extension.

After: the endpoint creates a schema (query_doctor by default, or the schema field in the request body) and puts the extension in it. The response carries the schema back. An extension that was already installed stays where it is, and every read of a public-schema extension logs what is wrong and the ALTER EXTENSION ... SET SCHEMA that fixes it.

A missing extension also reaches the caller as ExtensionNotInstalledError again, so the app shows its install panel instead of raw SQL error text. A resolve now logs the extension and schema it read, so a run that found the statistics can be told from one that found nothing.

The two halves are coupled. The verify step used to probe an unqualified pg_stat_statements, which resolves through the search_path and returns 42P01 for exactly the placement the install now produces — shipping the install alone would report every successful install as a failure.

How

Read src/sync/pg-connector.ts first.

installPgStatStatements() takes { schema }, defaulting to PostgresConnector.EXTENSION_SCHEMA. The name goes through PgIdentifier, so a caller-supplied schema is quoted rather than interpolated.

The verify step resolves the schema from pg_extension JOIN pg_namespace through the new getExtensionSchema(), then probes the view qualified. That helper asks about one named extension rather than reusing getQuerySource(), which matches pg_stat_statements or pg_stat_monitor with no ordering and could otherwise verify an extension this call never installed.

warnIfExtensionIsInPublic() fires from the resolver, so a public install is named at capture time rather than discovered through broken migrations. Its guard is a WeakMap keyed by the Postgres connection: ConnectionManager.getConnectorFor builds a new connector on every poll, so an instance flag would warn every ten seconds, and a process-wide flag would silence every source database after the first.

getRecentQueries() and resetPgStatStatements() classify errors by SQLSTATE through the new isMissingExtensionObject(). They matched the literal relation "pg_stat_statements" does not exist, but every statement they send is schema-qualified, so Postgres names the schema in the message and the match never fired. The codes are 42P01 for a missing relation, 42883 for a missing function, and 3F000 when the schema itself is gone, which is what a qualified function call reports rather than 42883.

src/remote/remote.dto.ts adds InstallPgStatStatementsRequest, which the HTTP route and the websocket controller both decode. An empty schema is a 400 rather than a 500.

The default is query_doctor because a migration tool reconciles the schemas it declares, and no tool declares that one. Any non-public name works.

Databases that already have the extension in public are not migrated. ALTER EXTENSION ... SET SCHEMA needs an ownership the tool may not hold, and relocating a schema object someone else installed is not ours to decide; the warning tells them instead.

Moving the extension trips the schema-drift gate once. The dump records the extension's schemaName and the schemaName of the two views it owns, so a run that installs into query_doctor differs from a baseline captured against public and the gate reports "This PR changes the database schema" on a pull request containing no migration. Measured on a live instance, and reproduced here by diffing the dump against two servers: extensions and views differ by schemaName, and nothing else does (the oid differences the diff also shows are what schemaDigest already ignores). It clears as soon as a run on the default branch writes a new baseline. Whether the drift comparison should ignore objects an extension owns is a separate question, not settled here.

Tests

src/sync/pg-connector.test.ts adds four integration tests against postgres:17: a fresh install lands in query_doctor and leaves nothing named pg_stat_statements* in public; an explicit { schema: "ext" } is honoured; an extension pre-installed in monitoring verifies and reports that schema (this one fails with 42P01 without the resolver change); and a public extension warns once across two connectors over the same connection.

An integration test pins those SQLSTATEs against a real server and asserts the messages do not contain the text the old check looked for. A unit test covers the classifier, including the privilege and syntax codes it must not claim.

Full suite: 44 files, 451 tests, all passing. tsc --noEmit clean.

veksen and others added 2 commits August 27, 2026 21:16
`installPgStatStatements()` issued a bare `CREATE EXTENSION`, which lands in
`public`. The extension owns views there that a migration tool reconciling
`public` cannot drop, so the tool aborts half-applied with SQLSTATE 2BP01 and
the failure surfaces far from the extension.

The install now takes a schema, defaulting to `query_doctor`, and the verify
probe resolves the schema through `getQuerySource()` instead of reading an
unqualified name through the `search_path` — which returns 42P01 for exactly
the placement the install now produces.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The verify step read `getQuerySource()`, which resolves either
`pg_stat_statements` or `pg_stat_monitor` with no ordering, so with both
installed it could verify an extension the install never touched. It now
resolves the one extension it installed.

The public-schema warning was guarded by an instance flag, but
`ConnectionManager.getConnectorFor` builds a new connector on every poll, so
it fired every ten seconds. The guard is now keyed by connection: once per
database, and still once per database when several are attached.

`POST /postgres/extensions/pg_stat_statements` takes the schema through the
websocket controller as well, and rejects an empty one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Query Doctor — 6 successful checks

 Cost regression — No query went up more than 5%
 Untested data access — No changed data-access file without a test
 New query — No new queries
 New query with index recommendation — No new query ships an index recommendation
 Schema drift — No schema changes
 High-value nudge — No index or rewrite past the threshold


More details via MCP → get_ci_run({ runId: "01a045d7-c1ca-7717-abbf-2d072097848f" }) · view run · docs
3 queries read against main on assumed statistics of 10,000,000 rows per table. Sync production stats for costs measured against your real data.

getRecentQueries and resetPgStatStatements matched the literal text `relation
"pg_stat_statements" does not exist`. Every statement they send is
schema-qualified, so Postgres names the schema in the message and the match
never fired: a missing extension reached the caller as a generic PostgresError
instead of ExtensionNotInstalledError, and the app showed raw SQL text rather
than its install panel.

Both now read the SQLSTATE — 42P01 for a missing relation, 42883 for a missing
function, 3F000 when the schema itself is gone, which is what a qualified
function call reports.

getQuerySource also logs the extension and schema it resolved. A run that read
the statistics and a run that found nothing were indistinguishable in the logs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@veksen

veksen commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

Folded into Query-Doctor/Site#4205. The collector image builds from Site's apps/analyzer, so the fix belongs there; this PR was opened unasked.

@veksen veksen closed this Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant