diff --git a/src/remote/remote-controller.ts b/src/remote/remote-controller.ts index 1981a83..57fef7f 100644 --- a/src/remote/remote-controller.ts +++ b/src/remote/remote-controller.ts @@ -1,7 +1,10 @@ import { WebSocket } from "ws"; import { env } from "../env.ts"; import { log } from "../log.ts"; -import { RemoteSyncRequest } from "./remote.dto.ts"; +import { + InstallPgStatStatementsRequest, + RemoteSyncRequest, +} from "./remote.dto.ts"; import { Remote } from "./remote.ts"; import * as errors from "../sync/errors.ts"; import type { OptimizedQuery } from "../sql/recent-query.ts"; @@ -251,14 +254,24 @@ export class RemoteController { } async onInstallPgStatStatements(rawBody: string): Promise { - const body = RemoteSyncRequest.safeDecode(rawBody); + const body = InstallPgStatStatementsRequest.safeDecode(rawBody); if (!body.success) { return { status: 400, body: body.error }; } try { - const result = await this.remote.installPgStatStatements(body.data.db); - return { status: 200, body: { success: true, preloadUpdated: result.preloadUpdated } }; + const result = await this.remote.installPgStatStatements( + body.data.db, + body.data.schema, + ); + return { + status: 200, + body: { + success: true, + preloadUpdated: result.preloadUpdated, + schema: result.schema, + }, + }; } catch (error) { console.error(error); if (error instanceof errors.PostgresError) { diff --git a/src/remote/remote.dto.ts b/src/remote/remote.dto.ts index f87675f..caf9b82 100644 --- a/src/remote/remote.dto.ts +++ b/src/remote/remote.dto.ts @@ -17,6 +17,29 @@ export const RemoteSyncRequest = z.codec( }, ); +/** + * `schema` is where the extension gets created. Omitting it takes the + * connector's default, which is deliberately not `public`. + */ +export const InstallPgStatStatementsRequest = z.codec( + z.string(), + z.object({ + db: z.custom(), + schema: z.string().min(1).optional(), + }), + { + encode: (value) => + JSON.stringify({ db: value.db.toString(), schema: value.schema }), + decode: (value) => { + const parsed = JSON.parse(value); + return { + db: Connectable.fromString(parsed.db), + schema: typeof parsed.schema === "string" ? parsed.schema : undefined, + }; + }, + }, +); + export const RemoteSyncFullSchemaResponse = z.discriminatedUnion("type", [ z.object({ type: z.literal("ok"), value: z.custom() }), z.object({ type: z.literal("error"), error: z.string() }), diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 5c5e7fd..e281c4e 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -638,9 +638,12 @@ export class Remote extends EventEmitter { this.optimizer.restart({ clearQueries: true }); } - async installPgStatStatements(source: Connectable): Promise<{ preloadUpdated: boolean }> { + async installPgStatStatements( + source: Connectable, + schema?: string, + ): Promise<{ preloadUpdated: boolean; schema: string }> { const connector = this.sourceManager.getConnectorFor(source); - return connector.installPgStatStatements(); + return connector.installPgStatStatements({ schema }); } /** diff --git a/src/server/http.ts b/src/server/http.ts index 3aa36e3..9d08a8e 100644 --- a/src/server/http.ts +++ b/src/server/http.ts @@ -12,7 +12,10 @@ import { SyncResult } from "../sync/syncer.ts"; import * as errors from "../sync/errors.ts"; import { RemoteController } from "../remote/remote-controller.ts"; import { Connectable } from "../sync/connectable.ts"; -import { RemoteSyncRequest } from "../remote/remote.dto.ts"; +import { + InstallPgStatStatementsRequest, + RemoteSyncRequest, +} from "../remote/remote.dto.ts"; import { ConnectionManager } from "../sync/connection-manager.ts"; import { Remote } from "../remote/remote.ts"; @@ -170,14 +173,18 @@ export async function createServer( fastify.post("/postgres/extensions/pg_stat_statements", async (request, reply) => { log.info(`[POST] /postgres/extensions/pg_stat_statements`, "http"); - const body = RemoteSyncRequest.safeDecode(JSON.stringify(request.body)); + const body = InstallPgStatStatementsRequest.safeDecode( + JSON.stringify(request.body), + ); if (!body.success) { return reply.status(400).send(body.error); } try { const db = await body.data.db.resolveDockerHost(); const connector = sourceConnectionManager.getConnectorFor(db); - const result = await connector.installPgStatStatements(); + const result = await connector.installPgStatStatements({ + schema: body.data.schema, + }); return reply.status(200).send({ success: true, ...result }); } catch (error) { return reply.status(500).send(makeUnexpectedErrorResult(error).body); diff --git a/src/sync/pg-connector.test.ts b/src/sync/pg-connector.test.ts index d5ed21b..485c58d 100644 --- a/src/sync/pg-connector.test.ts +++ b/src/sync/pg-connector.test.ts @@ -1,8 +1,9 @@ -import { test, expect } from "vitest"; +import { test, expect, vi } from "vitest"; import { PostgreSqlContainer } from "@testcontainers/postgresql"; import { ConnectionManager } from "./connection-manager.ts"; import { Connectable } from "./connectable.ts"; import { PgIdentifier } from "@query-doctor/core"; +import { isMissingExtensionObject } from "./pg-connector.ts"; test("getRecentQueries resolves pg_stat_statements in a non-default schema", async () => { const pg = await new PostgreSqlContainer("postgres:17") @@ -312,3 +313,225 @@ test("getTotalRowCount does not double-count when a table appears twice in the i await pg.stop(); } }); + +// Installing into `public` breaks any project whose migration tool reconciles +// that schema: the extension owns `pg_stat_statements_info`, and a reconciler +// that tries to drop it aborts half-applied with SQLSTATE 2BP01. The one path +// where we choose the placement must not choose `public`. +test("installPgStatStatements creates the extension outside public", async () => { + const pg = await new PostgreSqlContainer("postgres:17") + .withCommand(["-c", "shared_preload_libraries=pg_stat_statements"]) + .start(); + + const manager = ConnectionManager.forLocalDatabase(); + const conn = Connectable.fromString(pg.getConnectionUri()); + const db = manager.getOrCreateConnection(conn); + const connector = manager.getConnectorFor(db); + + try { + const result = await connector.installPgStatStatements(); + + expect(result.schema).toBe("query_doctor"); + + const [placement] = await db.exec<{ schema: string }>(` + SELECT n.nspname AS schema + FROM pg_extension e + JOIN pg_namespace n ON n.oid = e.extnamespace + WHERE e.extname = 'pg_stat_statements' + `); + expect(placement?.schema).toBe("query_doctor"); + + const [leftInPublic] = await db.exec<{ count: string }>(` + SELECT count(*) AS count + FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' AND c.relname LIKE 'pg_stat_statements%' + `); + expect(leftInPublic?.count).toBe("0"); + } finally { + await manager.closeAll(); + await pg.stop(); + } +}); + +test("installPgStatStatements installs into a caller-supplied schema", async () => { + const pg = await new PostgreSqlContainer("postgres:17") + .withCommand(["-c", "shared_preload_libraries=pg_stat_statements"]) + .start(); + + const manager = ConnectionManager.forLocalDatabase(); + const conn = Connectable.fromString(pg.getConnectionUri()); + const db = manager.getOrCreateConnection(conn); + const connector = manager.getConnectorFor(db); + + try { + const result = await connector.installPgStatStatements({ schema: "ext" }); + + expect(result.schema).toBe("ext"); + + const [placement] = await db.exec<{ schema: string }>(` + SELECT n.nspname AS schema + FROM pg_extension e + JOIN pg_namespace n ON n.oid = e.extnamespace + WHERE e.extname = 'pg_stat_statements' + `); + expect(placement?.schema).toBe("ext"); + } finally { + await manager.closeAll(); + await pg.stop(); + } +}); + +// The verify step used to probe an unqualified `pg_stat_statements`, which +// resolves through the search_path and so returns 42P01 for exactly the +// placement the install step now produces: a working install reported as a +// failure. +test("installPgStatStatements verifies an extension that sits off the search_path", async () => { + const pg = await new PostgreSqlContainer("postgres:17") + .withCopyContentToContainer([ + { + content: ` + CREATE SCHEMA monitoring; + CREATE EXTENSION pg_stat_statements SCHEMA monitoring; + `, + target: "/docker-entrypoint-initdb.d/init.sql", + }, + ]) + .withCommand(["-c", "shared_preload_libraries=pg_stat_statements"]) + .start(); + + const manager = ConnectionManager.forLocalDatabase(); + const conn = Connectable.fromString(pg.getConnectionUri()); + const connector = manager.getConnectorFor(conn); + + try { + const result = await connector.installPgStatStatements(); + + expect(result.schema).toBe("monitoring"); + } finally { + await manager.closeAll(); + await pg.stop(); + } +}); + +// Today the user finds out about a `public` install through unrelated migration +// failures. The resolver already knows, so say it where we read the extension. +test("getRecentQueries warns when the extension sits in public", async () => { + const pg = await new PostgreSqlContainer("postgres:17") + .withCopyContentToContainer([ + { + content: ` + CREATE EXTENSION pg_stat_statements; + CREATE TABLE users(id int, name text); + SELECT * FROM users WHERE id = 1; + `, + target: "/docker-entrypoint-initdb.d/init.sql", + }, + ]) + .withCommand(["-c", "shared_preload_libraries=pg_stat_statements"]) + .start(); + + const manager = ConnectionManager.forLocalDatabase(); + const conn = Connectable.fromString(pg.getConnectionUri()); + const connector = manager.getConnectorFor(conn); + const logged = vi.spyOn(console, "log").mockImplementation(() => {}); + + try { + await connector.getRecentQueries(); + // A fresh connector over the same connection, which is what every poll + // does. The warning dedupes per connection, so this must stay silent. + await manager.getConnectorFor(conn).getRecentQueries(); + + const messages = logged.mock.calls.map((call) => String(call[0])); + const warnings = messages.filter((message) => + message.includes("public schema") + ); + expect(warnings).toHaveLength(1); + expect( + messages.some((message) => + message.includes("pg_stat_statements") && + message.includes("public") && + message.includes("SET SCHEMA") + ), + `Expected a warning naming the fix. Logged:\n${messages.join("\n")}`, + ).toBe(true); + } finally { + logged.mockRestore(); + await manager.closeAll(); + await pg.stop(); + } +}); + +// The catch blocks in getRecentQueries/resetPgStatStatements used to match the +// literal `relation "pg_stat_statements" does not exist`. Every query that +// reaches them is schema-qualified, so Postgres names the schema too and the +// match never fired: a missing extension surfaced as a generic PostgresError +// rather than ExtensionNotInstalledError, and the UI showed raw SQL text +// instead of its install panel. +test("a qualified read of a missing extension reports 42P01, not the bare relation name", async () => { + const pg = await new PostgreSqlContainer("postgres:17") + .withCopyContentToContainer([ + { + content: `CREATE SCHEMA monitoring;`, + target: "/docker-entrypoint-initdb.d/init.sql", + }, + ]) + .start(); + + const manager = ConnectionManager.forLocalDatabase(); + const conn = Connectable.fromString(pg.getConnectionUri()); + const db = manager.getOrCreateConnection(conn); + + try { + const err = await db + .exec("SELECT 1 FROM monitoring.pg_stat_statements LIMIT 1") + .then(() => null, (e: unknown) => e as { code?: string; message: string }); + + expect(err).toBeTruthy(); + expect(err!.code).toBe("42P01"); + expect(err!.message).not.toContain( + 'relation "pg_stat_statements" does not exist', + ); + + const fnErr = await db + .exec("SELECT monitoring.pg_stat_statements_reset()") + .then(() => null, (e: unknown) => e as { code?: string; message: string }); + + expect(fnErr).toBeTruthy(); + expect(fnErr!.code).toBe("42883"); + expect(fnErr!.message).not.toContain( + "function pg_stat_statements_reset() does not exist", + ); + + // A function call through a schema that is gone entirely reports the + // schema rather than the function, so that code has to be recognised too. + const schemaErr = await db + .exec("SELECT absent_schema.pg_stat_statements_reset()") + .then(() => null, (e: unknown) => e as { code?: string }); + + expect(schemaErr?.code).toBe("3F000"); + } finally { + await manager.closeAll(); + await pg.stop(); + } +}); + +test("isMissingExtensionObject classifies the codes Postgres uses, and nothing else", () => { + expect(isMissingExtensionObject({ code: "42P01" })).toBe(true); + expect(isMissingExtensionObject({ code: "42883" })).toBe(true); + expect(isMissingExtensionObject({ code: "3F000" })).toBe(true); + + // Insufficient privilege and a syntax error are real failures the caller + // must see, not "the extension isn't installed". + expect(isMissingExtensionObject({ code: "42501" })).toBe(false); + expect(isMissingExtensionObject({ code: "42601" })).toBe(false); + + // The message text the old check matched carries no code of its own. + expect( + isMissingExtensionObject( + new Error('relation "pg_stat_statements" does not exist'), + ), + ).toBe(false); + expect(isMissingExtensionObject(null)).toBe(false); + expect(isMissingExtensionObject(undefined)).toBe(false); +}); diff --git a/src/sync/pg-connector.ts b/src/sync/pg-connector.ts index b38da95..a73854e 100644 --- a/src/sync/pg-connector.ts +++ b/src/sync/pg-connector.ts @@ -24,6 +24,20 @@ import { RawRecentQuery, RecentQuery } from "../sql/recent-query.ts"; import type { RecentQuerySource } from "../sql/recent-query.ts"; +/** + * Whether an error means the extension's table or function isn't there. + * + * Matching the message text does not work: every statement we send is + * schema-qualified, so Postgres names the schema in the message and a check for + * the bare `pg_stat_statements` never fires. The SQLSTATE is stable — 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. + */ +export function isMissingExtensionObject(err: unknown): boolean { + const code = (err as { code?: unknown } | null)?.code; + return code === "42P01" || code === "42883" || code === "3F000"; +} + const ctidSymbol = Symbol("ctid"); type Row = NonNullable & { [ctidSymbol]: string; @@ -83,8 +97,25 @@ export type ResetPgStatStatementsResult = */ export class PostgresConnector implements DatabaseConnector, RecentQuerySource { private static readonly QUERY_DOCTOR_USER = "query_doctor_db_link"; + /** + * Where {@link installPgStatStatements} puts the extension when the caller + * doesn't say. Never `public`: the extension owns views a migration tool + * reconciling `public` will try to drop, and fail on. + */ + public static readonly EXTENSION_SCHEMA = "query_doctor"; private readonly tupleEstimates = new Map(); private querySource: QuerySourceExtension | null = null; + /** + * Keyed by connection, because the connector is rebuilt on every poll — an + * instance flag would re-warn every ten seconds for the whole run — while a + * process-wide flag would silence every source database after the first. + * ConnectionManager caches one Postgres per database, so it is the identity + * that matches "warn once about this database". + */ + private static readonly warnedAboutPublicSchema = new WeakMap< + Postgres, + Set + >(); private static extensionNotInstalledError = new ExtensionNotInstalledError([ "pg_stat_statements", "pg_stat_monitor" @@ -503,9 +534,63 @@ ORDER BY extensionName: PgIdentifier.fromString(firstResult.extension), schema: PgIdentifier.fromString(firstResult.schema) }; + // A run that reads the statistics and a run that silently found nothing + // used to look identical in the logs. Name what was resolved. + log.debug( + `query source: ${firstResult.extension} in schema ${firstResult.schema}`, + "postgres", + ); + this.warnIfExtensionIsInPublic(this.querySource); return this.querySource; } + /** + * An extension in `public` is a landmine for any migration tool that + * reconciles that schema, and nothing at the point of failure names it. We + * resolve the schema on every read, so this is the one place that knows. + */ + private warnIfExtensionIsInPublic(source: QuerySourceExtension): void { + const extension = source.extensionName.unquoted(); + if (source.schema.unquoted() !== "public") { + return; + } + let warned = PostgresConnector.warnedAboutPublicSchema.get(this.db); + if (!warned) { + warned = new Set(); + PostgresConnector.warnedAboutPublicSchema.set(this.db, warned); + } + if (warned.has(extension)) { + return; + } + warned.add(extension); + log.warn( + `${extension} is installed in the public schema. A migration tool that reconciles public cannot drop its extension-owned views and will abort mid-run (SQLSTATE 2BP01). Move it with: CREATE SCHEMA IF NOT EXISTS ${PostgresConnector.EXTENSION_SCHEMA}; ALTER EXTENSION ${extension} SET SCHEMA ${PostgresConnector.EXTENSION_SCHEMA};`, + "postgres", + ); + } + + /** + * The schema one named extension lives in. {@link getQuerySource} answers the + * broader "where do the queries come from", which can resolve to + * `pg_stat_monitor`; a caller that installed a specific extension needs to + * probe that one. + * + * @throws {ExtensionNotInstalledError} + */ + private async getExtensionSchema(extension: string): Promise { + const [row] = await this.db.exec<{ schema: string }>( + `SELECT n.nspname AS schema + FROM pg_extension e + JOIN pg_namespace n ON n.oid = e.extnamespace + WHERE e.extname = $1 -- @qd_introspection`, + [extension], + ); + if (!row) { + throw new ExtensionNotInstalledError([extension]); + } + return PgIdentifier.fromString(row.schema); + } + /** * Get the latest queries using pg_stat_statements * @throws {ExtensionNotInstalledError} - pg_stat_statements is not installed @@ -526,10 +611,7 @@ ORDER BY return await syncQueries(results); } } catch (err) { - if ( - err instanceof Error && - err.message.includes('relation "pg_stat_statements" does not exist') - ) { + if (isMissingExtensionObject(err)) { throw PostgresConnector.extensionNotInstalledError; } console.error(err); @@ -556,11 +638,7 @@ ORDER BY `); } } catch (err) { - if ( - err instanceof Error && - (err.message.includes("function pg_stat_statements_reset() does not exist") || - err.message.includes("function pg_stat_monitor_reset() does not exist")) - ) { + if (isMissingExtensionObject(err)) { throw PostgresConnector.extensionNotInstalledError; } throw new PostgresError(err instanceof Error ? err.message : String(err)); @@ -578,8 +656,25 @@ ORDER BY } } - public async installPgStatStatements(): Promise<{ preloadUpdated: boolean }> { + /** + * Installs `pg_stat_statements` into {@link options.schema}, defaulting to + * {@link PostgresConnector.EXTENSION_SCHEMA}. + * + * An extension already installed elsewhere is left where it is — relocating + * it needs an ownership we may not hold, and moving a schema object out from + * under whoever put it there is not ours to decide. The resolver warns about + * a `public` placement on every read instead. + * + * @throws {PostgresError} + * @throws {ExtensionNotInstalledError} - the install ran but left nothing behind + */ + public async installPgStatStatements( + options: { schema?: string } = {}, + ): Promise<{ preloadUpdated: boolean; schema: string }> { let preloadUpdated = false; + const targetSchema = PgIdentifier.fromString( + options.schema ?? PostgresConnector.EXTENSION_SCHEMA, + ); const [preload] = await this.db.exec<{ setting: string }>(` SELECT setting FROM pg_settings WHERE name = 'shared_preload_libraries'; -- @qd_introspection @@ -603,19 +698,32 @@ ORDER BY `); if (!result?.exists) { try { - await this.db.exec(`CREATE EXTENSION pg_stat_statements;`); + await this.db.exec(`CREATE SCHEMA IF NOT EXISTS ${targetSchema};`); + await this.db.exec( + `CREATE EXTENSION pg_stat_statements SCHEMA ${targetSchema};`, + ); } catch (err) { throw new PostgresError(err instanceof Error ? err.message : String(err)); } } + // Resolve the schema rather than assume it. The extension may predate this + // call, and an unqualified probe reads through the search_path — which + // reports 42P01 for the very placement the branch above just produced. + const schema = await this.getExtensionSchema("pg_stat_statements"); + this.warnIfExtensionIsInPublic({ + extensionName: PgIdentifier.fromString("pg_stat_statements"), + schema, + }); try { - await this.db.exec(`SELECT 1 FROM pg_stat_statements LIMIT 1; -- @qd_introspection`); + await this.db.exec( + `SELECT 1 FROM ${schema}.pg_stat_statements LIMIT 1; -- @qd_introspection`, + ); } catch (err) { throw new PostgresError(err instanceof Error ? err.message : String(err)); } - return { preloadUpdated }; + return { preloadUpdated, schema: schema.unquoted() }; } public async checkPrivilege(): Promise<{