From e808ffa33a2e483da38ab759a485c0d91dd61e84 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Thu, 13 Aug 2026 23:22:16 -0300 Subject: [PATCH 1/3] fix(remote): publish production statistics from the sync that dumped them Statistics reached the server only from refreshStatsIfStale, which returns immediately when there is no drift baseline. Only a push set that baseline, and only refreshStatsIfStale pushed. A project with no snapshot on the server had nothing to seed a baseline from, so it never dumped, so it never got a snapshot. Every project stayed on synthetic statistics unless someone pushed a dump by hand. The sync already dumps those statistics and hands them to the optimizer. Record them there as well. The first sync at boot now publishes, and arms drift to keep the snapshot current. Below STATS_ROWS_THRESHOLD the sync skipped the dump. That threshold decides what the optimizer costs against, where a sequential scan on 200 rows is the right plan. It should not decide whether production's numbers get published, so the sync now dumps either way and publishes what it dumped. The optimizer still plans against the mode the threshold picked. Republishing on every boot costs nothing. The server hashes the tables, and a match only moves confirmed_at, so the timeline stays one row per change. Co-Authored-By: Claude --- src/remote/publish-stats-on-sync.test.ts | 57 ++++++++++++ src/remote/remote.ts | 106 +++++++++++++++++------ 2 files changed, 135 insertions(+), 28 deletions(-) create mode 100644 src/remote/publish-stats-on-sync.test.ts diff --git a/src/remote/publish-stats-on-sync.test.ts b/src/remote/publish-stats-on-sync.test.ts new file mode 100644 index 0000000..8a571ef --- /dev/null +++ b/src/remote/publish-stats-on-sync.test.ts @@ -0,0 +1,57 @@ +import { expect, test } from "vitest"; +import { PostgreSqlContainer } from "@testcontainers/postgresql"; +import type { ExportedStats } from "@query-doctor/core"; +import { Connectable } from "../sync/connectable.ts"; +import { ConnectionManager } from "../sync/connection-manager.ts"; +import { Remote } from "./remote.ts"; + +/** + * A project publishes its production statistics because its analyzer synced, + * not because someone went and asked it to. + * + * The drift check is what pushes a re-dump, and it cannot run without a + * baseline. The baseline was set only by a push, so an analyzer that had never + * pushed never would: no snapshot on the server, nothing to seed from, no + * baseline, no dump, no snapshot. The sync already holds the dump — it just has + * to say so. + */ +test("a sync publishes the statistics it dumped from the source", async () => { + const [sourceDb, targetDb] = await Promise.all([ + new PostgreSqlContainer("postgres:17") + .withCopyContentToContainer([ + { + // 200 rows: far under STATS_ROWS_THRESHOLD, which is the case that + // used to publish nothing at all. + content: ` + create table items(id int primary key, sockets int); + insert into items select g, g % 6 from generate_series(1, 200) g; + analyze items; + `, + target: "/docker-entrypoint-initdb.d/init.sql", + }, + ]) + .start(), + new PostgreSqlContainer("postgres:17").start(), + ]); + + try { + await using remote = new Remote( + Connectable.fromString(targetDb.getConnectionUri()), + ConnectionManager.forLocalDatabase(), + ); + const published: ExportedStats[][] = []; + remote.on("statsApplied", (stats) => published.push(stats)); + + await remote.syncFrom(Connectable.fromString(sourceDb.getConnectionUri())); + + expect(published).toHaveLength(1); + const items = published[0].find( + (t) => String(t.tableName) === "items", + ); + // Production's own number, not the 10M-row assumption the optimizer costs + // a database this small against. What we publish is what we measured. + expect(items?.reltuples).toBe(200); + } finally { + await Promise.all([sourceDb.stop(), targetDb.stop()]); + } +}); diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 43ab968..e7e773e 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -239,6 +239,13 @@ export class Remote extends EventEmitter { fullSchema.status === "fulfilled" ? fullSchema.value : undefined, ); + // After the sync, not before: a sync that failed half way through has not + // established what production looks like, and publishing from it would set + // a baseline the next drift check measures against. + if (statsResult.dump) { + this.recordSourceStatistics(statsResult.dump); + } + return { meta: { version: databaseInfo.status === "fulfilled" @@ -396,28 +403,41 @@ export class Remote extends EventEmitter { const connector = this.sourceManager.getConnectorFor(source); const totalRows = await connector.getTotalRowCount(tables); + // Dump either way. The threshold decides what the optimizer plans against, + // which is a costing decision — on two hundred rows a sequential scan is + // genuinely the right plan, and real statistics would hide every index the + // table will need once it grows. It is not a reason to withhold what + // production measures: the published snapshot is a record of this database, + // and a small database has one just as much as a large one does. + const dumped = await this.dumpSourceStats(source); + if (totalRows < Remote.STATS_ROWS_THRESHOLD) { log.info( `Total rows (${totalRows}) below threshold, using default stats`, "remote", ); - return { mode: Statistics.defaultStatsMode, strategy: "default" }; + return { + mode: Statistics.defaultStatsMode, + strategy: "default", + dump: dumped.stats, + }; } log.info( `Total rows (${totalRows}) above threshold, pulling source stats`, "remote", ); - return { mode: await this.dumpSourceStats(source), strategy: "fromSource" }; + return { mode: dumped, strategy: "fromSource", dump: dumped.stats }; } /** * Re-dump and push the source's statistics when they've drifted far enough * from what we last pushed (ADR 0007 §2). Runs on the schema poll tick. * - * No-ops until a `fromSource` dump has established a baseline: a synthetic or - * imported snapshot has nothing meaningful to drift against, and inventing a - * baseline for it would push someone else's numbers as this project's + * No-ops until a dump from the source has established a baseline. The sync at + * boot establishes one, so in practice this is armed from the first sync + * onwards; the guard is what stops an imported or synthetic snapshot standing + * in for a baseline and pushing someone else's numbers as this project's * production statistics. */ private async refreshStatsIfStale( @@ -515,7 +535,9 @@ export class Remote extends EventEmitter { log.info(`Statistics refresh skipped: ${reason}`, "remote"); } - private async dumpSourceStats(source: Connectable): Promise { + private async dumpSourceStats( + source: Connectable, + ): Promise> { const pg = this.sourceManager.getOrCreateConnection( source, ); @@ -546,11 +568,11 @@ export class Remote extends EventEmitter { /** * Adopt the server's stored snapshot as the drift baseline, without pushing. * - * Drift is measured against the last dump this process pushed, so an analyzer - * that has never pushed has no baseline and never drifts — which is every - * project whose snapshot was seeded by hand. Seeding on connect closes that - * gap: the first schema poll can then compare the live schema against what - * the server holds and re-dump if it has fallen behind. + * Drift is measured against the last dump this process recorded. The sync at + * boot records one, so this is not the only way a baseline appears — but it + * arrives first, and it covers a connection that reconnects without syncing + * again. Either way the first schema poll can compare the live schema against + * a baseline and re-dump if it has fallen behind. * * Deliberately does not push. These numbers came from the server; echoing * them back would republish a stale snapshot as if it were fresh. @@ -563,34 +585,52 @@ export class Remote extends EventEmitter { return; } this.statsBaseline = baselineFromDump(stats); - // Arm the daily floor too. The sync path reaches the optimizer directly - // rather than through `applyStatistics`, so without this `lastStatsPushAt` - // stays undefined and the floor never fires for a seeded analyzer — which - // is every analyzer that hasn't happened to drift. Dated from now rather - // than the snapshot's capture time, which the RPC doesn't carry: the floor - // then fires 24h after connect instead of immediately. + // Arm the daily floor too, so a connection that seeds and then never syncs + // still refreshes eventually. Dated from now rather than the snapshot's + // capture time, which the RPC doesn't carry: the floor then fires 24h after + // connect instead of immediately. this.lastStatsPushAt = Date.now(); } async applyStatistics(statsMode: StatisticsMode): Promise { await this.optimizer.setStatistics(statsMode); - // Push the statistics we were handed, not `optimizer.ownMetadata`. That is - // a dump of the *optimizing* database, which is restored with - // `--exclude-table-data-and-children` and never durably analyzed, so every - // table in it reports `reltuples = -1` and every column `stats: null`. - // Sending it would overwrite the project's real snapshot with an empty one. - // - // `fromAssumption` carries no real numbers at all, so it pushes nothing — + // Record the statistics we were handed, not `optimizer.ownMetadata` — see + // recordSourceStatistics for why that would publish an empty snapshot. + // `fromAssumption` carries no real numbers at all, so it records nothing: // synthetic defaults are not this project's production statistics. - if (statsMode.kind === "fromStatisticsExport" && statsMode.stats.length > 0) { - this.statsBaseline = baselineFromDump(statsMode.stats); - this.lastStatsPushAt = Date.now(); - this.emit("statsApplied", statsMode.stats); + if (statsMode.kind === "fromStatisticsExport") { + this.recordSourceStatistics(statsMode.stats); } // don't block the reply by awaiting all optimizations this.optimizer.restart(); } + /** + * Adopt a dump the analyzer took from the source: it becomes the drift + * baseline, and it reaches the server. + * + * Both halves matter. Publishing is how a project gets a snapshot at all, and + * the baseline is what lets the drift check re-dump later — without it, + * `refreshStatsIfStale` returns on its first line forever. Since the only + * thing that used to call this was a push, and a push only happened on drift, + * an analyzer that had never pushed never could. + * + * Callers pass what they measured from production, never the optimizer's own + * metadata: the optimizing database is restored with + * `--exclude-table-data-and-children` and never durably analyzed, so every + * table in it reports `reltuples = -1` and every column `stats: null`. + * Publishing that would overwrite the project's real snapshot with an empty + * one. + */ + private recordSourceStatistics(stats: ExportedStats[]): void { + if (stats.length === 0) { + return; + } + this.statsBaseline = baselineFromDump(stats); + this.lastStatsPushAt = Date.now(); + this.emit("statsApplied", stats); + } + async resetPgStatStatements(source: Connectable): Promise { const connector = this.sourceManager.getConnectorFor(source); await connector.resetPgStatStatements(); @@ -707,6 +747,16 @@ export type InferredStatsStrategy = "default" | "fromSource" | "imported"; type StatsResult = { mode: StatisticsMode; strategy: InferredStatsStrategy; + /** + * The statistics measured from the source on this sync, when there are any. + * + * Separate from `mode` because the two answer different questions: `mode` is + * what the optimizer costs against, `dump` is what production actually + * reports. They diverge below the row threshold, and only a dump is honest + * enough to publish — a `static` mode came from a file someone handed us, not + * from this database. + */ + dump?: ExportedStats[]; }; const PgStatStatementsStatus = { From 483c97f7baa62577af9d1ae64d3d0141b88e80d2 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Thu, 13 Aug 2026 23:36:02 -0300 Subject: [PATCH 2/3] test(remote): move testSpawnTarget into the shared helpers runner.ci.test.ts imported it from remote.test.ts, which registers that file's 11 container tests into runner.ci's suite as well. They ran twice: the full run reported 455 tests where 444 exist. test-utils.ts holds no top-level test(), so importing from there costs nothing. Co-Authored-By: Claude --- src/remote/publish-stats-on-sync.test.ts | 37 ++++++++++-------------- src/remote/remote.test.ts | 22 +------------- src/remote/test-utils.ts | 30 +++++++++++++++++++ src/runner.ci.test.ts | 2 +- 4 files changed, 47 insertions(+), 44 deletions(-) diff --git a/src/remote/publish-stats-on-sync.test.ts b/src/remote/publish-stats-on-sync.test.ts index 8a571ef..bd4fe5a 100644 --- a/src/remote/publish-stats-on-sync.test.ts +++ b/src/remote/publish-stats-on-sync.test.ts @@ -1,9 +1,9 @@ import { expect, test } from "vitest"; -import { PostgreSqlContainer } from "@testcontainers/postgresql"; import type { ExportedStats } from "@query-doctor/core"; import { Connectable } from "../sync/connectable.ts"; import { ConnectionManager } from "../sync/connection-manager.ts"; import { Remote } from "./remote.ts"; +import { assertDefined, testSpawnTarget } from "./test-utils.ts"; /** * A project publishes its production statistics because its analyzer synced, @@ -12,26 +12,20 @@ import { Remote } from "./remote.ts"; * The drift check is what pushes a re-dump, and it cannot run without a * baseline. The baseline was set only by a push, so an analyzer that had never * pushed never would: no snapshot on the server, nothing to seed from, no - * baseline, no dump, no snapshot. The sync already holds the dump — it just has - * to say so. + * baseline, no dump, no snapshot. */ test("a sync publishes the statistics it dumped from the source", async () => { const [sourceDb, targetDb] = await Promise.all([ - new PostgreSqlContainer("postgres:17") - .withCopyContentToContainer([ - { - // 200 rows: far under STATS_ROWS_THRESHOLD, which is the case that - // used to publish nothing at all. - content: ` - create table items(id int primary key, sockets int); - insert into items select g, g % 6 from generate_series(1, 200) g; - analyze items; - `, - target: "/docker-entrypoint-initdb.d/init.sql", - }, - ]) - .start(), - new PostgreSqlContainer("postgres:17").start(), + // 200 rows: far under STATS_ROWS_THRESHOLD, which is the case that used to + // publish nothing at all. + testSpawnTarget({ + content: ` + create table items(id int primary key, sockets int); + insert into items select g, g % 6 from generate_series(1, 200) g; + analyze items; + `, + }), + testSpawnTarget(), ]); try { @@ -45,12 +39,11 @@ test("a sync publishes the statistics it dumped from the source", async () => { await remote.syncFrom(Connectable.fromString(sourceDb.getConnectionUri())); expect(published).toHaveLength(1); - const items = published[0].find( - (t) => String(t.tableName) === "items", - ); + const items = published[0].find((t) => t.tableName === "items"); + assertDefined(items, "expected the published dump to cover items"); // Production's own number, not the 10M-row assumption the optimizer costs // a database this small against. What we publish is what we measured. - expect(items?.reltuples).toBe(200); + expect(items.reltuples).toBe(200); } finally { await Promise.all([sourceDb.stop(), targetDb.stop()]); } diff --git a/src/remote/remote.test.ts b/src/remote/remote.test.ts index eb40399..77e59a6 100644 --- a/src/remote/remote.test.ts +++ b/src/remote/remote.test.ts @@ -6,33 +6,13 @@ import { RemoteController } from "./remote-controller.ts"; import { Pool } from "pg"; import { ConnectionManager } from "../sync/connection-manager.ts"; -import { normalizeQuery } from "./test-utils.ts"; +import { normalizeQuery, testSpawnTarget } from "./test-utils.ts"; import { type Op } from "jsondiffpatch/formatters/jsonpatch"; -const TEST_TARGET_CONTAINER_NAME = "postgres:17"; const TEST_TARGET_CONTAINER_TIMESCALEDB_NAME = "timescale/timescaledb:latest-pg17"; -export function testSpawnTarget( - options: { content?: string; containerName?: string } = { - containerName: TEST_TARGET_CONTAINER_NAME, - }, -) { - let pg = new PostgreSqlContainer( - options.containerName ?? TEST_TARGET_CONTAINER_NAME, - ); - if (options.content) { - pg = pg.withCopyContentToContainer([ - { - content: options.content, - target: "/docker-entrypoint-initdb.d/init.sql", - }, - ]); - } - return pg.start(); -} - function assertOk( result: { type: string; value?: T }, ): asserts result is { type: "ok"; value: T } { diff --git a/src/remote/test-utils.ts b/src/remote/test-utils.ts index b911152..58a9323 100644 --- a/src/remote/test-utils.ts +++ b/src/remote/test-utils.ts @@ -1,4 +1,5 @@ import { expect } from "vitest"; +import { PostgreSqlContainer } from "@testcontainers/postgresql"; export function assert( condition: unknown, @@ -17,3 +18,32 @@ export function assertDefined( export function normalizeQuery(query: string): string { return query.replace(/\s+/g, " "); } + +const TEST_TARGET_CONTAINER_NAME = "postgres:17"; + +/** + * Start a Postgres container, seeded from `content` as an initdb script when + * one is given. + * + * Lives here rather than beside the tests that use it: importing it from a + * `*.test.ts` registers that file's tests into the importer's suite too, so + * every borrower pays for the lender's containers. + */ +export function testSpawnTarget( + options: { content?: string; containerName?: string } = { + containerName: TEST_TARGET_CONTAINER_NAME, + }, +) { + let pg = new PostgreSqlContainer( + options.containerName ?? TEST_TARGET_CONTAINER_NAME, + ); + if (options.content) { + pg = pg.withCopyContentToContainer([ + { + content: options.content, + target: "/docker-entrypoint-initdb.d/init.sql", + }, + ]); + } + return pg.start(); +} diff --git a/src/runner.ci.test.ts b/src/runner.ci.test.ts index a0b2788..f163602 100644 --- a/src/runner.ci.test.ts +++ b/src/runner.ci.test.ts @@ -1,6 +1,6 @@ import { test, expect } from "vitest"; import { PostgreSqlContainer } from "@testcontainers/postgresql"; -import { testSpawnTarget } from "./remote/remote.test.ts"; +import { testSpawnTarget } from "./remote/test-utils.ts"; import { Connectable } from "./sync/connectable.ts"; import { ConnectionManager } from "./sync/connection-manager.ts"; import { Remote } from "./remote/remote.ts"; From 4e1835714cc487b22e82480ba3ef117d65a57e66 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Thu, 13 Aug 2026 23:36:02 -0300 Subject: [PATCH 3/3] refactor(remote): overlap the row count with the dump, and cut repeated comments decideStatsStrategy no longer needs the row count before it can dump, so the two queries run together. The dump reads pg_class, pg_attribute and pg_statistic; the count aggregates pg_class. Neither waits on the other. The comments said the same thing in three places. StatsResult.dump keeps the part the code cannot show, which is why a static mode publishes nothing. The placement comment in syncFrom claimed an ordering constraint the position does not buy, since the dump is already taken by then; it now says what is true. A comment in seed-stats-baseline.test.ts described the old sync path and this branch made it false. Co-Authored-By: Claude --- src/remote/remote.ts | 38 ++++++++++++-------------- src/remote/seed-stats-baseline.test.ts | 6 ++-- 2 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/remote/remote.ts b/src/remote/remote.ts index e7e773e..8c1ae6f 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -239,9 +239,8 @@ export class Remote extends EventEmitter { fullSchema.status === "fulfilled" ? fullSchema.value : undefined, ); - // After the sync, not before: a sync that failed half way through has not - // established what production looks like, and publishing from it would set - // a baseline the next drift check measures against. + // After `optimizer.start`, so a failed start doesn't leave a baseline armed + // for a drift check that will never run. if (statsResult.dump) { this.recordSourceStatistics(statsResult.dump); } @@ -401,15 +400,19 @@ export class Remote extends EventEmitter { tables: FullSchemaTable[], ): Promise { const connector = this.sourceManager.getConnectorFor(source); - const totalRows = await connector.getTotalRowCount(tables); // Dump either way. The threshold decides what the optimizer plans against, - // which is a costing decision — on two hundred rows a sequential scan is - // genuinely the right plan, and real statistics would hide every index the - // table will need once it grows. It is not a reason to withhold what - // production measures: the published snapshot is a record of this database, - // and a small database has one just as much as a large one does. - const dumped = await this.dumpSourceStats(source); + // which is a costing decision: on two hundred rows a sequential scan is the + // right plan, and real statistics would hide every index the table needs + // once it grows. It does not decide whether production's numbers reach the + // server. A small database has a snapshot worth publishing too. + // + // Concurrent because the row count no longer gates the dump, and the two + // read different catalogs. + const [totalRows, dumped] = await Promise.all([ + connector.getTotalRowCount(tables), + this.dumpSourceStats(source), + ]); if (totalRows < Remote.STATS_ROWS_THRESHOLD) { log.info( @@ -610,10 +613,8 @@ export class Remote extends EventEmitter { * baseline, and it reaches the server. * * Both halves matter. Publishing is how a project gets a snapshot at all, and - * the baseline is what lets the drift check re-dump later — without it, - * `refreshStatsIfStale` returns on its first line forever. Since the only - * thing that used to call this was a push, and a push only happened on drift, - * an analyzer that had never pushed never could. + * the baseline is what lets the drift check re-dump later: without it, + * `refreshStatsIfStale` returns on its first line forever. * * Callers pass what they measured from production, never the optimizer's own * metadata: the optimizing database is restored with @@ -748,13 +749,8 @@ type StatsResult = { mode: StatisticsMode; strategy: InferredStatsStrategy; /** - * The statistics measured from the source on this sync, when there are any. - * - * Separate from `mode` because the two answer different questions: `mode` is - * what the optimizer costs against, `dump` is what production actually - * reports. They diverge below the row threshold, and only a dump is honest - * enough to publish — a `static` mode came from a file someone handed us, not - * from this database. + * What production measured, when this sync measured it. Absent for a static + * mode: those numbers came from a file, not from this database. */ dump?: ExportedStats[]; }; diff --git a/src/remote/seed-stats-baseline.test.ts b/src/remote/seed-stats-baseline.test.ts index 2d4ee74..898476f 100644 --- a/src/remote/seed-stats-baseline.test.ts +++ b/src/remote/seed-stats-baseline.test.ts @@ -91,10 +91,8 @@ describe("Remote.seedStatsBaseline — daily floor interaction", () => { } it("arms the daily floor, so a seeded analyzer still refreshes eventually", () => { - // The sync path reaches the optimizer directly rather than through - // applyStatistics, so seeding is the only chance to set this. Left unset, - // isPastRefreshFloor short-circuits on undefined and the floor never fires - // for any analyzer that hasn't happened to drift. + // Left unset, isPastRefreshFloor short-circuits on undefined and the floor + // never fires for a connection that seeds and then never syncs. const remote = makeRemote(); remote.seedStatsBaseline([table("users", 10_000)]);