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..bd4fe5a --- /dev/null +++ b/src/remote/publish-stats-on-sync.test.ts @@ -0,0 +1,50 @@ +import { expect, test } from "vitest"; +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, + * 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. + */ +test("a sync publishes the statistics it dumped from the source", async () => { + const [sourceDb, targetDb] = await Promise.all([ + // 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 { + 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) => 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); + } 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/remote.ts b/src/remote/remote.ts index 43ab968..8c1ae6f 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -239,6 +239,12 @@ export class Remote extends EventEmitter { fullSchema.status === "fulfilled" ? fullSchema.value : undefined, ); + // 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); + } + return { meta: { version: databaseInfo.status === "fulfilled" @@ -394,30 +400,47 @@ 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 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( `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 +538,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 +571,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 +588,50 @@ 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. + * + * 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 +748,11 @@ export type InferredStatsStrategy = "default" | "fromSource" | "imported"; type StatsResult = { mode: StatisticsMode; strategy: InferredStatsStrategy; + /** + * 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[]; }; const PgStatStatementsStatus = { 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)]); 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";