From cffc60fcd371d52e4db348a1a07eae43a1073495 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Mon, 27 Jul 2026 17:41:12 -0300 Subject: [PATCH 1/2] feat(remote): re-dump production statistics on a daily floor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #3671. Shape Drift and Size Drift watch structure and row counts. Neither sees a database whose tables and sizes hold steady while the distribution of its data moves: histograms, most-common-value lists and correlation all age with the values rather than the count. A floor bounds how wrong those can get. The check rides the same schema poll as drift, so no new schedule. A push from either trigger updates the timestamp, so drift and the floor can't dump twice in quick succession. Never fires before a first push. With no baseline there is nothing to compare against, and dumping on a timer for an analyzer that has never pushed would publish statistics for a database the server may not expect. ADR 0007 §2 places this backstop as a cron in Site. It sits in the analyzer instead: the server has no way to ask an analyzer to refresh, so the Site-side version needs a new ClientApi method, an implementation, and a cron, where this needs a timestamp. Worth amending the ADR if the Site placement is preferred. Co-Authored-By: Claude --- src/remote/remote.ts | 15 +++++++++++++-- src/remote/stats-drift.test.ts | 29 +++++++++++++++++++++++++++++ src/remote/stats-drift.ts | 28 ++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/remote/remote.ts b/src/remote/remote.ts index f86483f..c8f4553 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -25,6 +25,7 @@ import { SchemaLoader } from "./schema-loader.ts"; import { baselineFromDump, detectDrift, + isPastRefreshFloor, type StatsBaseline, } from "./stats-drift.ts"; @@ -92,6 +93,12 @@ export class Remote extends EventEmitter { private statsBaseline?: StatsBaseline; /** Guards against a second drift dump starting while one is in flight. */ private refreshingStats = false; + /** + * When this analyzer last pushed a dump, for the daily floor. A drift- + * triggered push updates it too, so the two triggers can't dump twice in + * quick succession. + */ + private lastStatsPushAt?: number; private pgStatStatementsStatus: PgStatStatementsStatus = PgStatStatementsStatus.Unknown; @@ -390,14 +397,17 @@ export class Remote extends EventEmitter { const connector = this.sourceManager.getConnectorFor(source); const reltuples = await connector.getReltuplesByTable(); const verdict = detectDrift(this.statsBaseline, { reltuples }); - if (!verdict.drifted) { + const pastFloor = isPastRefreshFloor(this.lastStatsPushAt, Date.now()); + if (!verdict.drifted && !pastFloor) { return; } this.refreshingStats = true; try { log.info( - `${verdict.kind === "shape" ? "Shape" : "Size"} Drift — ${verdict.reason}. Re-dumping production statistics`, + verdict.drifted + ? `${verdict.kind === "shape" ? "Shape" : "Size"} Drift — ${verdict.reason}. Re-dumping production statistics` + : "Production statistics are past the daily floor. Re-dumping", "remote", ); // applyStatistics records the new baseline and emits `statsApplied`, @@ -467,6 +477,7 @@ export class Remote extends EventEmitter { // database, so drifting against it would be nonsense. if (statsMode.kind === "fromStatisticsExport") { this.statsBaseline = baselineFromDump(stats); + this.lastStatsPushAt = Date.now(); } this.emit("statsApplied", stats); } diff --git a/src/remote/stats-drift.test.ts b/src/remote/stats-drift.test.ts index 392b286..b04aed2 100644 --- a/src/remote/stats-drift.test.ts +++ b/src/remote/stats-drift.test.ts @@ -3,6 +3,7 @@ import type { ExportedStats } from "@query-doctor/core"; import { baselineFromDump, detectDrift, + isPastRefreshFloor, SIZE_DRIFT_MIN_ROWS, } from "./stats-drift.ts"; @@ -149,3 +150,31 @@ describe("detectDrift — Size Drift", () => { expect(verdict.drifted).toBe(false); }); }); + +describe("isPastRefreshFloor", () => { + const NOW = 1_800_000_000_000; + const DAY = 24 * 60 * 60 * 1000; + + it("is false before the analyzer has ever pushed", () => { + // Nothing to compare against, and dumping on a timer for an analyzer with + // no baseline would publish statistics the server never asked for. + expect(isPastRefreshFloor(undefined, NOW)).toBe(false); + }); + + it("is false while the last push is recent", () => { + expect(isPastRefreshFloor(NOW - DAY / 2, NOW)).toBe(false); + }); + + it("is true once a full day has passed", () => { + expect(isPastRefreshFloor(NOW - DAY, NOW)).toBe(true); + }); + + it("is true well past the floor", () => { + expect(isPastRefreshFloor(NOW - DAY * 40, NOW)).toBe(true); + }); + + it("honours a custom floor", () => { + expect(isPastRefreshFloor(NOW - 5_000, NOW, 1_000)).toBe(true); + expect(isPastRefreshFloor(NOW - 500, NOW, 1_000)).toBe(false); + }); +}); diff --git a/src/remote/stats-drift.ts b/src/remote/stats-drift.ts index 2db156a..4b95dd3 100644 --- a/src/remote/stats-drift.ts +++ b/src/remote/stats-drift.ts @@ -143,3 +143,31 @@ function summarize(keys: TableKey[]): string { const rest = keys.length - shown.length; return rest > 0 ? `${shown.join(", ")}, and ${rest} more` : shown.join(", "); } + +/** + * How long a snapshot may stand without a re-dump, regardless of drift. + * + * Shape Drift and Size Drift both watch structure and row counts. Neither sees + * a database whose tables and sizes hold steady while the *distribution* of its + * data moves: histograms, most-common-value lists and correlation all age with + * the values, not the row count. A floor bounds how wrong those can get. + */ +export const DEFAULT_REFRESH_FLOOR_MS = 24 * 60 * 60 * 1000; + +/** + * Whether the last push is old enough to earn a re-dump on its own. + * + * Never true before a first push: with no baseline there is nothing to compare + * against, and dumping on a timer for an analyzer that has never pushed would + * publish statistics for a database the server may not expect. + */ +export function isPastRefreshFloor( + lastPushedAt: number | undefined, + now: number, + floorMs: number = DEFAULT_REFRESH_FLOOR_MS, +): boolean { + if (lastPushedAt === undefined) { + return false; + } + return now - lastPushedAt >= floorMs; +} From 072ebe8f178fdbf42829bf8f1f256340bea1c448 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Mon, 27 Jul 2026 17:49:53 -0300 Subject: [PATCH 2/2] fix(remote): arm the daily floor on seed and back off failed refreshes Review of the daily floor found it never fires for the analyzers it targets. `lastStatsPushAt` is written only by `applyStatistics`, and the sync path reaches the optimizer directly instead, so a seeded analyzer leaves it undefined and `isPastRefreshFloor` short-circuits forever. Only a drift-triggered dump ever armed it, which is the case the floor exists to cover when drift does not fire. It also re-armed nothing across restarts. Seeding now sets the timestamp. It is dated from the seed rather than the snapshot's capture time, which the RPC doesn't carry, so the floor fires 24h after connect rather than immediately. A failing dump also retried on every 60s poll, because the timestamp only advances on success. A statement_timeout on a large pg_statistic read would run DUMP_STATS_SQL ~1440 times a day. Failures now back off for 15 minutes. Renamed `refreshStatsIfDrifted` to `refreshStatsIfStale`, since it has had two triggers since the floor landed. Co-Authored-By: Claude --- src/remote/remote.ts | 24 ++++++++++++++++++++++-- src/remote/seed-stats-baseline.test.ts | 26 ++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/remote/remote.ts b/src/remote/remote.ts index c8f4553..d72e920 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -99,6 +99,9 @@ export class Remote extends EventEmitter { * quick succession. */ private lastStatsPushAt?: number; + /** Earliest time a failed refresh may be retried. See {@link Remote.STATS_RETRY_BACKOFF_MS}. */ + private retryStatsAfter?: number; + private static readonly STATS_RETRY_BACKOFF_MS = 15 * 60 * 1000; private pgStatStatementsStatus: PgStatStatementsStatus = PgStatStatementsStatus.Unknown; @@ -390,10 +393,16 @@ export class Remote extends EventEmitter { * baseline for it would push someone else's numbers as this project's * production statistics. */ - private async refreshStatsIfDrifted(source: Connectable): Promise { + private async refreshStatsIfStale(source: Connectable): Promise { if (!this.statsBaseline || this.refreshingStats) { return; } + // Back off after a failure. `lastStatsPushAt` only advances on success, so + // without this a dump that keeps throwing (a statement_timeout on a large + // pg_statistic read, say) would be retried on every 60s poll. + if (this.retryStatsAfter !== undefined && Date.now() < this.retryStatsAfter) { + return; + } const connector = this.sourceManager.getConnectorFor(source); const reltuples = await connector.getReltuplesByTable(); const verdict = detectDrift(this.statsBaseline, { reltuples }); @@ -413,6 +422,10 @@ export class Remote extends EventEmitter { // applyStatistics records the new baseline and emits `statsApplied`, // which is what carries the dump back to the server. await this.applyStatistics(await this.dumpSourceStats(source)); + this.retryStatsAfter = undefined; + } catch (error) { + this.retryStatsAfter = Date.now() + Remote.STATS_RETRY_BACKOFF_MS; + throw error; } finally { this.refreshingStats = false; } @@ -466,6 +479,13 @@ 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. + this.lastStatsPushAt = Date.now(); } async applyStatistics(statsMode: StatisticsMode): Promise { @@ -529,7 +549,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. this.schemaLoader.on("polled", () => { - this.refreshStatsIfDrifted(source).catch((error) => { + this.refreshStatsIfStale(source).catch((error) => { log.error("Failed to check statistics drift", "remote"); console.error(error); }); diff --git a/src/remote/seed-stats-baseline.test.ts b/src/remote/seed-stats-baseline.test.ts index c73ac2b..2d4ee74 100644 --- a/src/remote/seed-stats-baseline.test.ts +++ b/src/remote/seed-stats-baseline.test.ts @@ -84,3 +84,29 @@ describe("Remote.seedStatsBaseline", () => { expect(pushed).toBe(false); }); }); + +describe("Remote.seedStatsBaseline — daily floor interaction", () => { + function floorArmedAt(remote: Remote): number | undefined { + return (remote as unknown as { lastStatsPushAt?: number }).lastStatsPushAt; + } + + 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. + const remote = makeRemote(); + + remote.seedStatsBaseline([table("users", 10_000)]); + + expect(floorArmedAt(remote)).toBeTypeOf("number"); + }); + + it("leaves the floor unarmed when there is no snapshot to seed from", () => { + const remote = makeRemote(); + + remote.seedStatsBaseline([]); + + expect(floorArmedAt(remote)).toBeUndefined(); + }); +});