From ca54187f7247a5c0494a36f745d4ffc5be8d1997 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Mon, 27 Jul 2026 17:53:56 -0300 Subject: [PATCH] fix(remote): push the source dump instead of the optimizer's own metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `applyStatistics` emitted `optimizer.ownMetadata`, which is a dump of the *optimizing* database, not the source. That copy is restored with `--exclude-table-data-and-children` and its statistics are only ever written inside a transaction that rolls back, so every table reports reltuples = -1 and every column stats: null. The relay stores what it receives verbatim, so a push replaces a project's real production statistics with an empty snapshot. Nothing triggered it before: applyStatistics is only reached from updateStatistics, resetStats, and the HTTP import, and none run on their own. Seeding the drift baseline on connect made the drift path reachable, which turns this into a live fault — a drifted project would push the empty dump, baseline itself against reltuples = -1, then see every real table as size drift and re-dump the source every 60 seconds. Push `statsMode.stats` instead, and push nothing for fromAssumption: synthetic defaults are not a project's production statistics. Co-Authored-By: Claude --- src/remote/apply-statistics.test.ts | 70 +++++++++++++++++++++++++++++ src/remote/remote.ts | 22 ++++----- 2 files changed, 82 insertions(+), 10 deletions(-) create mode 100644 src/remote/apply-statistics.test.ts diff --git a/src/remote/apply-statistics.test.ts b/src/remote/apply-statistics.test.ts new file mode 100644 index 0000000..0c36175 --- /dev/null +++ b/src/remote/apply-statistics.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it, vi } from "vitest"; +import { Statistics, type ExportedStats } from "@query-doctor/core"; +import { Connectable } from "../sync/connectable.ts"; +import { ConnectionManager } from "../sync/connection-manager.ts"; +import { Remote } from "./remote.ts"; + +function makeRemote(): Remote { + return new Remote( + Connectable.fromString("postgresql://postgres@localhost:5432/postgres"), + ConnectionManager.forRemoteDatabase(), + ); +} + +function table(name: string, reltuples: number): ExportedStats { + return { + schemaName: "public", + tableName: name, + reltuples, + relpages: 1, + relallvisible: 0, + columns: [], + indexes: [], + } as unknown as ExportedStats; +} + +/** + * `applyStatistics` decides what reaches the server's stored snapshot, so what + * it emits matters more than what it applies locally. The optimizer runs + * against a copy restored without table data, so its own metadata reports + * `reltuples = -1` everywhere — pushing that would replace a project's real + * production statistics with an empty snapshot. + */ +describe("Remote.applyStatistics", () => { + it("pushes the source dump, not the optimizer's own metadata", async () => { + const remote = makeRemote(); + const sourceDump = [table("users", 5_000_000)]; + // The optimizing database is empty; its dump is what must NOT be pushed. + vi.spyOn(remote.optimizer, "setStatistics").mockResolvedValue(undefined); + vi.spyOn(remote.optimizer, "restart").mockResolvedValue(undefined); + vi.spyOn(remote.optimizer, "ownMetadata", "get").mockReturnValue([ + table("users", -1), + ]); + const pushed: ExportedStats[][] = []; + remote.on("statsApplied", (stats) => pushed.push(stats)); + + await remote.applyStatistics(Statistics.statsModeFromExport(sourceDump)); + + expect(pushed).toEqual([sourceDump]); + expect(pushed[0][0].reltuples).toBe(5_000_000); + }); + + it("pushes nothing for synthetic statistics", async () => { + // resetStats applies fromAssumption. Those numbers are invented, so + // publishing them as production statistics would be a lie. + const remote = makeRemote(); + vi.spyOn(remote.optimizer, "setStatistics").mockResolvedValue(undefined); + vi.spyOn(remote.optimizer, "restart").mockResolvedValue(undefined); + vi.spyOn(remote.optimizer, "ownMetadata", "get").mockReturnValue([ + table("users", -1), + ]); + let pushed = false; + remote.on("statsApplied", () => { + pushed = true; + }); + + await remote.applyStatistics(Statistics.defaultStatsMode); + + expect(pushed).toBe(false); + }); +}); diff --git a/src/remote/remote.ts b/src/remote/remote.ts index d72e920..4ced906 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -490,16 +490,18 @@ export class Remote extends EventEmitter { async applyStatistics(statsMode: StatisticsMode): Promise { await this.optimizer.setStatistics(statsMode); - const stats = this.optimizer.ownMetadata; - if (stats) { - // Record what we're about to push as the drift baseline. Only meaningful - // for source-derived stats; an imported snapshot describes someone else's - // database, so drifting against it would be nonsense. - if (statsMode.kind === "fromStatisticsExport") { - this.statsBaseline = baselineFromDump(stats); - this.lastStatsPushAt = Date.now(); - } - this.emit("statsApplied", stats); + // 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 — + // 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); } // don't block the reply by awaiting all optimizations this.optimizer.restart();