From 59c1ce8794ffe546a1f2236cae2d5194421e7c51 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Thu, 13 Aug 2026 23:11:16 -0300 Subject: [PATCH] refact(remote): normalize identifiers through PgIdentifier, fold the baseline The column check hand-rolled its identifier unquoting: String(name), then a fixed-point loop undoing however many levels of quote escaping that had applied. PgIdentifier.unquoted() is the accessor for reading a name rather than emitting one, and its doc says so. Core already pairs it with a single un-doubling in sql/foreign-keys.ts, for this exact asymmetry. Undoing exactly one level per side is also more correct than the fixed point, which folded `we""ird` and `we"ird` into the same key and reported the second as covered. Test added for the distinction. StatsBaseline held three structures keyed by the same table key, whose key sets were identical by construction but not by type. One Map of a record deletes a field rather than adding a third. The three Shape Drift checks built the same verdict literal three times, differing in a noun phrase. They now share one builder, and the check order reads as one chain. The module doc claimed every comparison runs in one direction, which the dropped-table check contradicts. Scoped to columns, with the asymmetry named. The call-site comment in remote.ts claimed Shape Drift reads indexes, which it does not. Co-Authored-By: Claude --- src/remote/remote.ts | 4 +- src/remote/stats-drift.test.ts | 101 ++++++++---------- src/remote/stats-drift.ts | 190 ++++++++++++++------------------- 3 files changed, 130 insertions(+), 165 deletions(-) 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(", "); }