diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 43ab968..44ad40b 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -422,7 +422,7 @@ export class Remote extends EventEmitter { */ private async refreshStatsIfStale( source: Connectable, - schema?: FullSchema, + schema: FullSchema, ): Promise { if (!this.statsBaseline) { this.noteSkippedRefresh( @@ -635,7 +635,7 @@ export class Remote extends EventEmitter { // The schema poll is also the drift tick: it already runs every 60s, so // checking here costs one `pg_class` read and no extra schedule. The schema // it just dumped comes along for the same reason — Shape Drift reads the - // columns and indexes off it, which no other signal can see. + // columns off it, which no other signal can see. this.schemaLoader.on("polled", (schema) => { this.refreshStatsIfStale(source, schema).catch((error) => { log.error("Failed to check statistics drift", "remote"); diff --git a/src/remote/stats-drift.test.ts b/src/remote/stats-drift.test.ts index 9bc7441..c4e4507 100644 --- a/src/remote/stats-drift.test.ts +++ b/src/remote/stats-drift.test.ts @@ -11,7 +11,7 @@ import { function table( name: string, reltuples: number, - covers?: { columns?: string[] }, + columns?: string[], ): ExportedStats { return { schemaName: "public", @@ -20,50 +20,42 @@ function table( relpages: Math.max(1, Math.round(reltuples / 100)), relallvisible: 0, // `DUMP_STATS_SQL` reads these from `pg_attribute`, unquoted. - columns: (covers?.columns ?? []).map((columnName) => ({ columnName })), + columns: (columns ?? []).map((columnName) => ({ columnName })), indexes: [], } as unknown as ExportedStats; } /** - * A live schema as the 60s poll delivers it: names go through the same - * `quote_ident` → `PgIdentifier` path the real dump uses, so a test written + * A live schema as the 60s poll delivers it. Names take the same + * `quote_ident` → `PgIdentifier` path the real dump does, so a case written * with a raw name exercises whatever escaping that path applies. */ function schema( - tables: { - name: string; - columns?: (string | { name: string; dropped: boolean })[]; - }[], + tables: { name: string; columns?: string[]; dropped?: string[] }[], ): FullSchema { + // `PgIdentifier.toString()` is `quote_ident`, per its own docstring; the zod + // codec then wraps that in a second `fromString`. const identifier = (raw: string) => - PgIdentifier.fromString(quoteIdent(raw)) as unknown as string; + PgIdentifier.fromString(PgIdentifier.fromString(raw).toString()); + const column = (raw: string, dropped: boolean) => ({ + type: "column", + name: identifier(raw), + dropped, + }); return { tables: tables.map((t) => ({ type: "table", schemaName: identifier("public"), tableName: identifier(t.name), - columns: (t.columns ?? []).map((column) => - typeof column === "string" - ? { type: "column", name: identifier(column), dropped: false } - : { - type: "column", - name: identifier(column.name), - dropped: column.dropped, - } - ), + columns: [ + ...(t.columns ?? []).map((c) => column(c, false)), + ...(t.dropped ?? []).map((c) => column(c, true)), + ], })), indexes: [], } as unknown as FullSchema; } -/** `quote_ident`, as the schema dump applies it before we ever see the name. */ -function quoteIdent(raw: string): string { - return /^[a-z_][a-z0-9_]*$/.test(raw) - ? raw - : `"${raw.replaceAll('"', '""')}"`; -} - function reltuples(entries: Record): Map { return new Map( Object.entries(entries).map(([name, rows]) => [`public.${name}`, rows]), @@ -136,7 +128,7 @@ describe("detectDrift — Shape Drift", () => { describe("detectDrift — Shape Drift on columns", () => { it("fires when the live schema has a column the snapshot doesn't cover", () => { const baseline = baselineFromDump([ - table("users", BIG, { columns: ["id", "email"] }), + table("users", BIG, ["id", "email"]), ]); const verdict = detectDrift(baseline, { @@ -154,7 +146,7 @@ describe("detectDrift — Shape Drift on columns", () => { // The steady state, and the one that has to hold: a signal that stays lit // after the dump it asked for would re-dump on every 60s poll forever. const baseline = baselineFromDump([ - table("users", BIG, { columns: ["id", "email"] }), + table("users", BIG, ["id", "email"]), ]); const verdict = detectDrift(baseline, { @@ -172,11 +164,9 @@ describe("detectDrift — Shape Drift on columns", () => { ])( "converges on a name needing quotes: %s", (_case, columnName) => { - // The snapshot reads `pg_attribute` raw; the schema runs the name through - // `quote_ident` and then `PgIdentifier`, which escapes it again. Any name - // the two sides can't agree on is a full dump every 60 seconds, forever. + // Pins `rawName` against every escaping level the two sides apply. const baseline = baselineFromDump([ - table("users", BIG, { columns: ["id", columnName] }), + table("users", BIG, ["id", columnName]), ]); const verdict = detectDrift(baseline, { @@ -188,30 +178,36 @@ describe("detectDrift — Shape Drift on columns", () => { }, ); - it("ignores a column flagged dropped, whichever side filters it", () => { - // Both dumps filter `attisdropped` today, so this state does not reach us - // in practice. The schema carries the flag, and a column missing by - // construction would ask for a dump it can never be satisfied by. - const baseline = baselineFromDump([ - table("users", BIG, { columns: ["id"] }), - ]); + it("keeps names apart that differ only in how many quotes they hold", () => { + // `we""ird` and `we"ird` are different columns. Undoing exactly one level + // of escaping per side keeps them so; collapsing until the name stops + // changing folds them together and reports the second as covered. + const baseline = baselineFromDump([table("users", BIG, ['we"ird'])]); const verdict = detectDrift(baseline, { reltuples: reltuples({ users: BIG }), - schema: schema([ - { name: "users", columns: ["id", { name: "legacy", dropped: true }] }, - ]), + schema: schema([{ name: "users", columns: ['we"ird', 'we""ird'] }]), + }); + + expect(verdict.drifted).toBe(true); + if (!verdict.drifted) return; + expect(verdict.reason).toContain('public.users.we""ird'); + }); + + it("ignores a column flagged dropped", () => { + const baseline = baselineFromDump([table("users", BIG, ["id"])]); + + const verdict = detectDrift(baseline, { + reltuples: reltuples({ users: BIG }), + schema: schema([{ name: "users", columns: ["id"], dropped: ["legacy"] }]), }); expect(verdict.drifted).toBe(false); }); it("ignores columns on a relation the snapshot never covered", () => { - // The table checks own an uncovered table, and they read the `pg_class` - // probe rather than the schema. The two disagree on materialized views: - // the schema carries them, the probe and the statistics dump don't. Firing - // here would ask for a dump that can never satisfy it. - const baseline = baselineFromDump([table("users", BIG, { columns: ["id"] })]); + // A materialized view: in the schema, in neither the probe nor the dump. + const baseline = baselineFromDump([table("users", BIG, ["id"])]); const verdict = detectDrift(baseline, { reltuples: reltuples({ users: BIG }), @@ -225,12 +221,9 @@ describe("detectDrift — Shape Drift on columns", () => { }); it("does not fire when a column is dropped from a covered table", () => { - // Deliberate. Restore matches the snapshot against the live relation by - // name, so a column that no longer exists matches nothing and costs - // nothing. Spending a full dump to delete unread rows is the noise this - // signal exists to avoid. + // Deliberate, and the asymmetry with a dropped table is the point. const baseline = baselineFromDump([ - table("users", BIG, { columns: ["id", "email", "legacy_flag"] }), + table("users", BIG, ["id", "email", "legacy_flag"]), ]); const verdict = detectDrift(baseline, { @@ -243,7 +236,7 @@ describe("detectDrift — Shape Drift on columns", () => { it("takes precedence over a simultaneous size change", () => { const baseline = baselineFromDump([ - table("users", BIG, { columns: ["id"] }), + table("users", BIG, ["id"]), ]); const verdict = detectDrift(baseline, { @@ -259,7 +252,7 @@ describe("detectDrift — Shape Drift on columns", () => { // Both are true at once: `teams` is new, and `users` gained a column. Only // an order that checks tables first can report the table, so this pins the // order rather than restating it. - const baseline = baselineFromDump([table("users", BIG, { columns: ["id"] })]); + const baseline = baselineFromDump([table("users", BIG, ["id"])]); const verdict = detectDrift(baseline, { reltuples: reltuples({ users: BIG, teams: 0 }), @@ -277,7 +270,7 @@ describe("detectDrift — Shape Drift on columns", () => { it("checks nothing when the poll has produced no schema yet", () => { const baseline = baselineFromDump([ - table("users", BIG, { columns: ["id", "email"] }), + table("users", BIG, ["id", "email"]), ]); const verdict = detectDrift(baseline, { reltuples: reltuples({ users: BIG }) }); diff --git a/src/remote/stats-drift.ts b/src/remote/stats-drift.ts index 2e49695..f207e02 100644 --- a/src/remote/stats-drift.ts +++ b/src/remote/stats-drift.ts @@ -1,4 +1,5 @@ import type { ExportedStats, FullSchema } from "@query-doctor/core"; +import { PgIdentifier } from "@query-doctor/core"; /** * Decides when the production-statistics snapshot the server holds has fallen @@ -20,41 +21,37 @@ import type { ExportedStats, FullSchema } from "@query-doctor/core"; * planner's no-statistics defaults, which nothing synthesizes and nothing * reports. * - * Every comparison here is presence-only, and in one direction: the source has - * something the snapshot lacks. + * Columns are compared on presence, never on type. A column's type reaches the + * schema through `format_type` (`character varying(255)`) and the snapshot + * through `pg_type.typname` (`varchar`), so comparing types would call every + * column changed on every poll. Catching a type change needs the dump to export + * `format_type` first. * - * Presence-only because the two sides don't speak the same dialect anywhere - * else. A column's type reaches the schema through `format_type` (`character - * varying(255)`) and the snapshot through `pg_type.typname` (`varchar`), so - * comparing types would call every column changed on every poll. Catching a - * type change needs the dump to export `format_type` first. + * A dropped column is not drift, where a dropped table is. Restore matches the + * snapshot to the live database by name, so a column the source no longer has + * matches nothing and costs nothing, and spending a full dump to delete rows no + * query reads is the noise this signal exists to avoid. A dropped table is + * cheap to notice and rare enough to be worth a dump. * - * One direction because a relation the snapshot covers and the source has - * dropped costs nothing: the restore matches the snapshot to the live database - * by name, so the extra entry matches nothing. Spending a full dump to delete - * rows no query reads is the noise this signal exists to avoid. - * - * Indexes are deliberately not compared, though the snapshot carries their - * names. `DUMP_STATS_SQL` collects them in a CTE filtered by - * `relname NOT LIKE 'pg_%'` — where `_` is a wildcard, so a table called - * `pgmigrations` or `pgbench_accounts` is exported with its columns and an - * empty index list. That table's indexes would read as uncovered after the very - * dump that was meant to cover them, which is a full dump every 60 seconds - * forever. The same CTE groups by `relname` alone, so same-named tables in - * different schemas share one index list and a genuinely new index is masked - * anyway. Both are fixable only in the dump. + * Indexes are not compared, though the snapshot carries their names, because + * `DUMP_STATS_SQL`'s index list is not a sound baseline: its CTE filters + * `relname NOT LIKE 'pg_%'`, where `_` is a wildcard, and groups by `relname` + * alone. See Query-Doctor/Site#4005 and Query-Doctor/Site#3959. */ /** A table's identity in a dump, as `"schema.table"`. */ export type TableKey = string; export interface StatsBaseline { - /** Tables the last pushed dump covered. */ - tables: Set; - /** Their `reltuples` at that moment, for the Size Drift comparison. */ - reltuples: Map; - /** The column names it covered on each of them, unquoted. */ - columns: Map>; + /** What the last pushed dump covered, per table it covered. */ + tables: Map; +} + +interface CoveredTable { + /** Its `reltuples` at that moment, for the Size Drift comparison. */ + reltuples: number; + /** Its column names, as Postgres reports them. */ + columns: Set; } /** The current cheap reading from the source, for comparison against a baseline. */ @@ -62,9 +59,8 @@ export interface SourceReading { reltuples: Map; /** * The live schema from the same poll tick, which carries the columns - * `reltuples` alone can't see. Optional: a caller that has no schema yet gets - * the table and size checks and skips the rest, rather than reading an absent - * schema as an empty database. + * `reltuples` alone can't see. Omit it to run only the table and size checks; + * an absent schema is never read as an empty database. */ schema?: FullSchema; } @@ -104,13 +100,8 @@ export const DEFAULT_SIZE_DRIFT_RATIO = 0.2; export const SIZE_DRIFT_MIN_ROWS = 1_000; export function baselineFromDump(stats: ExportedStats[]): StatsBaseline { - const tables = new Set(); - const reltuples = new Map(); - const columns = new Map>(); + const tables = new Map(); for (const table of stats) { - const key = tableKey(table.schemaName, table.tableName); - tables.add(key); - reltuples.set(key, table.reltuples); // Every live column, whether or not Postgres has analyzed it — the dump // reads `pg_attribute` and left-joins `pg_statistic`. So a column that has // never been analyzed still lands here, and asking for a dump on its @@ -120,49 +111,38 @@ export function baselineFromDump(stats: ExportedStats[]): StatsBaseline { // stored, which for an old enough capture may carry no columns key at all. // Reading that as "covers no columns" earns one re-dump and then converges, // where trusting the type throws inside the poll and no refresh ever runs. - columns.set( - key, - new Set((table.columns ?? []).map((c) => unquote(c.columnName))), - ); + tables.set(tableKey(table.schemaName, table.tableName), { + reltuples: table.reltuples, + columns: new Set((table.columns ?? []).map((c) => c.columnName)), + }); } - return { tables, reltuples, columns }; + return { tables }; } -export function tableKey( - schemaName: string | { toString(): string }, - tableName: string | { toString(): string }, +function tableKey( + schemaName: string | PgIdentifier, + tableName: string | PgIdentifier, ): TableKey { - return `${unquote(String(schemaName))}.${unquote(String(tableName))}`; + return `${rawName(schemaName)}.${rawName(tableName)}`; } /** - * Undo `quote_ident`, collapsing escaped quotes until the name stops changing. + * The raw name Postgres reports, from either side of the comparison. * - * The two sides of a comparison reach us in different dialects. The statistics - * dump reads `pg_attribute` raw; the schema poll runs `quote_ident` and then - * `PgIdentifier` escapes that result a second time, so a column named `we"ird` - * arrives as `"we""""ird"` against a raw `we"ird`. Undoing one level leaves - * them unequal, and a name that never matches asks for a full dump on every - * poll, forever. + * The capture reads `pg_attribute` and `pg_class` directly, so its identifiers + * are already raw strings. The schema poll's went through `quote_ident` before + * `PgIdentifier` wrapped them, and `unquoted()` returns the value as + * `fromString` recorded it, which leaves that one level of doubling in place. + * A name compared in the wrong dialect never matches, and a relation that never + * matches asks for a full dump on every poll, forever. * - * Collapsing to a fixed point lands both dialects on the raw name. It also - * makes two names that differ only in how many quotes they contain compare - * equal, so a deliberately hostile identifier can read as covered when it - * isn't. That costs one missed refresh; the loop it replaces costs a full dump - * every 60 seconds. + * Same pairing, for the same reason, as `parentTableKey` in core's + * `sql/foreign-keys.ts`. */ -function unquote(identifier: string): string { - if ( - identifier.length < 2 || !identifier.startsWith('"') || - !identifier.endsWith('"') - ) { - return identifier; - } - let value = identifier.slice(1, -1); - while (value.includes('""')) { - value = value.replaceAll('""', '"'); - } - return value; +function rawName(identifier: string | PgIdentifier): string { + return identifier instanceof PgIdentifier + ? identifier.unquoted().replaceAll('""', '"') + : identifier; } /** @@ -184,38 +164,23 @@ export function detectDrift( for (const key of current.reltuples.keys()) { if (!baseline.tables.has(key)) added.push(key); } - if (added.length > 0) { - return { - drifted: true, - kind: "shape", - reason: `${added.length} table(s) not covered by the snapshot: ${ - summarize(added) - }`, - }; - } const dropped: TableKey[] = []; - for (const key of baseline.tables) { + for (const key of baseline.tables.keys()) { if (!current.reltuples.has(key)) dropped.push(key); } - if (dropped.length > 0) { - return { - drifted: true, - kind: "shape", - reason: `${dropped.length} table(s) in the snapshot no longer exist: ${ - summarize(dropped) - }`, - }; - } - const uncovered = uncoveredColumns(baseline, current.schema); - if (uncovered) { - return { drifted: true, kind: "shape", reason: uncovered }; - } + const shapeDrift = shape(added, "table(s) not covered by the snapshot") ?? + shape(dropped, "table(s) in the snapshot no longer exist") ?? + shape( + uncoveredColumns(baseline, current.schema), + "column(s) not covered by the snapshot", + ); + if (shapeDrift) return shapeDrift; let closest: { table: TableKey; ratio: number } | undefined; for (const [key, now] of current.reltuples) { - const before = baseline.reltuples.get(key); + const before = baseline.tables.get(key)?.reltuples; if (before === undefined) continue; // Exempt tables that are small on both sides. A table that grew past the // floor is a real change even if it started tiny. @@ -239,26 +204,37 @@ export function detectDrift( return { drifted: false, closest }; } +/** A Shape Drift verdict over whatever was found missing, or undefined if none was. */ +function shape(items: string[], phrase: string): DriftVerdict | undefined { + if (items.length === 0) return undefined; + return { + drifted: true, + kind: "shape", + reason: `${items.length} ${phrase}: ${summarize(items)}`, + }; +} + /** * Columns the live schema has on a table the snapshot covers, and the snapshot - * doesn't. Returns the reason to re-dump, or undefined for none. + * doesn't, as `schema.table.column`. * * Tables the snapshot doesn't cover at all are skipped: the checks above own * them, and they read the `pg_class` probe rather than the schema. The two - * disagree on purpose — the probe is `relkind = 'r'`, while the schema also - * carries materialized views, which no statistics dump will ever cover. - * Reporting those here would ask for a dump that cannot satisfy it. + * disagree on purpose. The probe is `relkind = 'r'`; the schema is + * `relkind in ('r','m') AND relispartition = false`, so it adds materialized + * views, which no statistics dump will ever cover, and omits every partition of + * a partitioned table, which get no column check at all today. */ function uncoveredColumns( baseline: StatsBaseline, schema: FullSchema | undefined, -): string | undefined { - if (!schema) return undefined; +): string[] { + if (!schema) return []; const columns: string[] = []; for (const table of schema.tables) { const key = tableKey(table.schemaName, table.tableName); - const covered = baseline.columns.get(key); + const covered = baseline.tables.get(key)?.columns; if (!covered) continue; for (const column of table.columns) { // Belt and braces: both dumps filter `attisdropped` today, so a dropped @@ -266,20 +242,16 @@ function uncoveredColumns( // flag, and a column that is missing by construction would never stop // asking for a dump. if (column.dropped) continue; - const name = unquote(String(column.name)); + const name = rawName(column.name); if (!covered.has(name)) columns.push(`${key}.${name}`); } } - if (columns.length === 0) return undefined; - - return `${columns.length} column(s) not covered by the snapshot: ${ - summarize(columns) - }`; + return columns; } -function summarize(keys: TableKey[]): string { - const shown = keys.slice(0, 5); - const rest = keys.length - shown.length; +function summarize(items: string[]): string { + const shown = items.slice(0, 5); + const rest = items.length - shown.length; return rest > 0 ? `${shown.join(", ")}, and ${rest} more` : shown.join(", "); }