fix: gauntlet cycle 2 must-fixes for PR-5.1.5 (operator docs + deterministic fallback)

- requires-superuser operator docs un-drifted: migrations/README.md's bootstrap
  ordering section and migrate-schema.py's comment/docstring/error text no
  longer enumerate a stale 002/004/005 list (006/007 carry the marker too);
  the in-file marker is named as authoritative. README also documents 006/007
  and the drift-repairing commit_timestamp re-stamp SQL.
- The query-summary all-NULL fallback arm now joins commits and orders by
  c.timestamp DESC NULLS LAST, so a series ingested entirely inside the
  transient unstamped window deterministically returns its newest value
  (matching the replaced join-ordered query); LEFT join keeps orphan rows
  fail-visible. Regression test seeds two unstamped rows and asserts the newer
  commit's value wins.
- The migrate loader's denormalization UPDATE is now drift-repairing
  (IS DISTINCT FROM instead of IS NULL), matching the documented repair form.
- summary.ts module header reworded (IS NOT DISTINCT FROM is no longer the
  query-summary mechanism); 006/007 historical-rationale note added to the
  skip-scan docblock; orphaned no-Docker comment in groups.test.ts re-attached
  to its describe; conn type annotation; _UPDATE_VALUE_MUTATIONS header note.

Deferred (nits, cycle-2 triage): mapWithConcurrency unit test, poolMax default
pin, summary-path coverage of the remaining groupPred pin combinations.

Signed-off-by: "Connor Tsui" <connor@spiraldb.com>

Author: connortsui20

benchmarks-website/migrate/src/postgres.rs

@@ -257,14 +257,17 @@ pub fn load(duckdb_path: &Path, dsn: &str, ca_cert: Option<&Path>) -> Result<Loa
     // 006, the read path's latest-per-series sort key). The v3 DuckDB source has
     // no such column, so it cannot ride the COPY's 1:1 column mapping; it is
     // derived here from the already-loaded `commits` dim (`TABLE_SPECS` loads
-    // `commits` first), inside the same all-or-nothing transaction.
+    // `commits` first), inside the same all-or-nothing transaction. `IS DISTINCT
+    // FROM` (rather than `IS NULL`) makes the stamp drift-repairing as well as
+    // NULL-filling, the same repair form migrations/README.md documents for
+    // operational re-stamping.
     let stamped = tx
         .execute(
             "UPDATE query_measurements q
                 SET commit_timestamp = c.timestamp
                FROM commits c
               WHERE c.commit_sha = q.commit_sha
-                AND q.commit_timestamp IS NULL",
+                AND q.commit_timestamp IS DISTINCT FROM c.timestamp",
             &[],
         )
         .context("denormalizing commit_timestamp onto query_measurements")?;

benchmarks-website/web/lib/groups.test.ts

@@ -462,8 +462,6 @@ describe.skipIf(!dockerAvailable())('summary math fidelity (testcontainers Postg
   });
 });
 
-// Slug-decode rejection short-circuits to a 400 before any DB call, so this
-// runs without Docker, matching the chart route's input-validation contract.
 // PR-5.1.5 read-path skip-scan fidelity. The canonical fixture above cannot
 // exercise these paths: it seeds a single query group with one format per
 // engine and stamps every commit_timestamp, so the summary skip scan's
@@ -506,6 +504,10 @@ describe.skipIf(!dockerAvailable())(
         [4, COMMIT_A, 1, 'e2', 'f1', 555, true],
         [5, COMMIT_A, 2, 'e1', 'f1', 666, true],
         [6, COMMIT_B, 1, 'e9', 'f9', 333, false],
+        // A second, OLDER unstamped row for e9:f9: the all-NULL fallback must
+        // deterministically pick COMMIT_B's 333 (newest via the commits join),
+        // not an arbitrary row.
+        [7, COMMIT_A, 1, 'e9', 'f9', 999, false],
       ];
       for (const [mid, sha, queryIdx, engine, format, valueNs, stamped] of rows) {
         await pool.query(
@@ -571,8 +573,11 @@ describe.skipIf(!dockerAvailable())(
       const byName = new Map(summary.rankings.map((r) => [r.name, r.totalRuntime]));
       // e1:f1's latest for Q1 is the STAMPED 111, not the newer NULL-stamped 222
       // (plus its Q2 value 666); e9:f9 has only NULL-stamped rows and must still
-      // appear via the fallback arm. Flipping the probe to a plain
-      // `commit_timestamp DESC` (NULLS FIRST) order fails this test.
+      // appear via the fallback arm, which must pick the NEWEST commit's 333
+      // over the older 999 (the fallback orders by the joined
+      // commits.timestamp). Flipping the probe to a plain `commit_timestamp
+      // DESC` (NULLS FIRST) order, or dropping the fallback's ORDER BY, fails
+      // this test.
       expect(byName.get('e1:f1')).toBeCloseTo(111 + 666, 6);
       expect(byName.get('e9:f9')).toBeCloseTo(333, 6);
     });
@@ -647,6 +652,8 @@ describe.skipIf(!dockerAvailable())(
   },
 );
 
+// Slug-decode rejection short-circuits to a 400 before any DB call, so this
+// runs without Docker, matching the chart route's input-validation contract.
 describe('GET /api/group/[slug] input validation (no DB)', () => {
   it('returns 400 for a structurally malformed slug', async () => {
     const slug = 'not-a-slug';

benchmarks-website/web/lib/summary.ts

@@ -10,8 +10,13 @@
  * gated on a v2 dataset allowlist via [`queryGroupHasV2Summary`].
  *
  * Behaviour-preservation notes (substrate migration, DuckDB -> Postgres):
- *  - `IS NOT DISTINCT FROM` gives `NULL == NULL` equality on the nullable
- *    `dataset_variant` / `scale_factor` dims, exactly as the DuckDB query did.
+ *  - Nullable-dim equality (`dataset_variant` / `scale_factor`) in the
+ *    query-group summary is rendered as `col IS NULL` / `col = $n` per the
+ *    concrete key's build-time value, the index-sargable form of the DuckDB
+ *    `NULL == NULL` semantics (the same rule as `sargableDimEq` in
+ *    `queries.ts`; PR-5.1.5). The compression summaries' self-joins below are
+ *    the only remaining `IS NOT DISTINCT FROM` users -- there the dim is a
+ *    join column, not a build-time constant, and the tables are small.
  *  - value columns are read `::float8` so node-postgres returns a JS `number`
  *    matching the Rust `CAST(... AS DOUBLE)`, rather than the bigint-as-string
  *    default.
@@ -355,7 +360,11 @@ async function collectQuerySummary(
   // "Latest per series" is a recursive-CTE skip scan (loose index scan) over the
   // covering index `idx_query_measurements_summary` (dataset, dataset_variant,
   // scale_factor, storage, query_idx, engine, format, commit_timestamp DESC)
-  // INCLUDE (value_ns) from migrations 006/007 (PR-5.1.5 fix c). The previous
+  // INCLUDE (value_ns) from migrations 006/007 (PR-5.1.5 fix c). Those
+  // migrations' in-file rationale describes the interim `DISTINCT ON` consumer
+  // and predates the cold-render decision that shipped this skip scan; the
+  // files are frozen post-apply, so this comment (and migrations/README.md) is
+  // the current record. The previous
   // `DISTINCT ON (query_idx, engine, format) ... ORDER BY commit_timestamp DESC
   // NULLS LAST` form scanned the group's entire history (~1.8M index entries,
   // ~2.4s warm for tpcds at the prod seed); the skip scan instead does one
@@ -398,17 +407,18 @@ async function collectQuerySummary(
   //    index is `commit_timestamp DESC`, i.e. NULLS FIRST, so that order is not
   //    index-provided. The per-series latest probe therefore takes the newest
   //    `commit_timestamp IS NOT NULL` row first (index-ordered descent past the
-  //    NULL block, ordinal 1) and falls back to an arbitrary NULL-timestamp row
-  //    (ordinal 2) only when the series has no timestamped rows, which is
-  //    exactly the NULLS LAST semantics; the two branches are selected via the
-  //    same `ORDER BY br LIMIT 1` guarantee as the successor probe. (Which row
-  //    wins among all-NULL ties is unspecified, as it already was under
-  //    `DISTINCT ON`. One deliberate divergence from the replaced query: the
-  //    old form's `JOIN commits` silently excluded an orphan row whose
-  //    commit_sha has no commits row, while the skip scan never joins commits,
-  //    so such an orphan -- impossible unless a writer violates the
-  //    commits-upsert-first invariant -- would now surface via the NULL
-  //    fallback instead of vanishing. Fail-visible is preferred.)
+  //    NULL block, ordinal 1) and falls back to the NULL-timestamp rows
+  //    (ordinal 2) only when the series has no timestamped rows; the two
+  //    branches are selected via the same `ORDER BY br LIMIT 1` guarantee as
+  //    the successor probe. The fallback branch joins `commits` and orders by
+  //    `c.timestamp DESC NULLS LAST` so an all-unstamped series (one ingested
+  //    entirely inside the transient window) still returns its NEWEST value,
+  //    matching the replaced join-ordered query, rather than an arbitrary row.
+  //    The join is LEFT so an orphan row whose commit_sha has no commits row
+  //    -- impossible unless a writer violates the commits-upsert-first
+  //    invariant -- still surfaces (last) instead of vanishing as it did under
+  //    the old INNER JOIN; fail-visible is preferred. The fallback's join cost
+  //    is irrelevant: it executes only for series with zero stamped rows.
   //
   // The `value_ns > 0` filter rides inside every probe: it is read from the
   // index leaf (INCLUDE), so the enumeration lands directly on series that have
@@ -491,12 +501,14 @@ async function collectQuerySummary(
         UNION ALL
         (SELECT 2 AS br, q.value_ns::float8 AS value_ns
            FROM query_measurements q
+           LEFT JOIN commits c ON c.commit_sha = q.commit_sha
           WHERE ${groupPred}
             AND q.query_idx = s.query_idx
             AND q.engine = s.engine
             AND q.format = s.format
             AND q.value_ns > 0
             AND q.commit_timestamp IS NULL
+          ORDER BY c.timestamp DESC NULLS LAST
           LIMIT 1)
         ORDER BY br
         LIMIT 1

migrations/README.md

@@ -54,29 +54,65 @@ lexicographic order, which equals numeric order under this convention.
   `rds_iam` membership: the read service authenticates with a static password
   (Vercel has no AWS credentials to mint IAM tokens), and on RDS `rds_iam`
   membership forces IAM-only auth. Added during the Phase-4 operator-gate work.
+- `006_read_path_perf.sql` — denormalizes `commits.timestamp` onto
+  `query_measurements.commit_timestamp` (with a one-time backfill) and adds the
+  read-path-perf indexes (PR-5.1.5). Applied by the RDS master (it ALTERs a
+  master-owned table), hence its `requires-superuser` marker.
+- `007_summary_covering_index.sql` — rebuilds the summary index with
+  `INCLUDE (value_ns)` so the latest-per-series read is index-only (PR-5.1.5).
+  Same `requires-superuser` marker as 006. Note: 006/007's in-file rationale
+  describes the `DISTINCT ON` read path that shipped first; the read service
+  later moved to recursive-CTE skip scans (the PR-5.1.5 cold-render fix) that
+  use the same indexes, and the files are frozen post-apply (see Authoring
+  rules), so their prose is historical. The current consumer is documented in
+  `benchmarks-website/web/lib/summary.ts` / `queries.ts`.
 
 This README + the runner ship in PR-1.2; `001`/`002` land in PR-1.3, `003` in
-PR-1.4, `004` in PR-2.1, and `005` in the Phase-4 operator-gate work (the v4
-read-service identity).
+PR-1.4, `004` in PR-2.1, `005` in the Phase-4 operator-gate work (the v4
+read-service identity), and `006`/`007` in PR-5.1.5 (read-path perf).
 
-## Bootstrap ordering — `requires-superuser` migrations (002 / 004 / 005)
+## Re-stamping `commit_timestamp` (operational note)
 
-Migrations `002_iam_db_user.sql`, `004_ingest_role.sql`, and `005_read_role.sql`
-carry a `-- migrate-schema: requires-superuser` marker comment. Before applying a marked
-file, the runner ([`scripts/migrate-schema.py`](../scripts/migrate-schema.py))
-asserts the connected role is **master-capable** — the capability proxy is
-`rolsuper OR rolcreaterole` (a true superuser locally, or the RDS master, which
-has `CREATEROLE`). If the connected role has neither, the runner raises
+`query_measurements.commit_timestamp` is a denormalized copy of
+`commits.timestamp` (006). Writers stamp it at insert; 006 backfilled
+pre-existing rows. If rows are ever observed unstamped or drifted (a writer
+predating the stamp, or a commits re-ingest that changed a timestamp), the
+repair is the drift-tolerant form of the backfill, run as any role with UPDATE
+on the table:
+
+```sql
+UPDATE query_measurements q
+   SET commit_timestamp = c.timestamp
+  FROM commits c
+ WHERE c.commit_sha = q.commit_sha
+   AND q.commit_timestamp IS DISTINCT FROM c.timestamp;
+```
+
+(`IS DISTINCT FROM` also fills NULLs, so this strictly supersedes the
+NULL-only 006 form. Follow a large repair with `VACUUM ANALYZE
+query_measurements` — the 006 prod backfill showed the planner needs it.)
+
+## Bootstrap ordering — `requires-superuser` migrations
+
+Migrations whose header carries a `-- migrate-schema: requires-superuser`
+marker comment (currently `002`, `004`, `005`, `006`, and `007`; the marker in
+the file is authoritative, not this list) must be applied by a master-capable
+role. Before applying a marked file, the runner
+([`scripts/migrate-schema.py`](../scripts/migrate-schema.py)) asserts the
+connected role is **master-capable** — the capability proxy is `rolsuper OR
+rolcreaterole` (a true superuser locally, or the RDS master, which has
+`CREATEROLE`). If the connected role has neither, the runner raises
 `PermissionError` and refuses to apply the file, rather than failing partway
-through its privileged `CREATE ROLE` / `GRANT` / `ALTER DEFAULT PRIVILEGES`
-statements.
-
-The ordering contract this enforces: **apply the bootstrap migrations (002/004/005)
-as the RDS master before any `migrator`-role deploy.** The `migrator` IAM user
-that `schema-deploy.yml` assumes into is itself created by `002` and is not
-master-capable, so it cannot apply `002`/`004`/`005`; those must land first under the
-master. `001` (plain DDL) and `003` (a ledger grant) carry no marker and apply
-under either role. A future migration that needs a master-capable role (another
-`CREATE ROLE`, `ALTER DEFAULT PRIVILEGES`, or other superuser-only DDL) should
-carry `-- migrate-schema: requires-superuser` on a comment line so the same
+through its privileged `CREATE ROLE` / `GRANT` / `ALTER DEFAULT PRIVILEGES` /
+master-owned-table DDL statements.
+
+The ordering contract this enforces: **apply every marked migration as the RDS
+master before any `migrator`-role deploy.** The `migrator` IAM user that
+`schema-deploy.yml` assumes into is itself created by `002` and is not
+master-capable, so it cannot apply the marked files; those must land first
+under the master. `001` (plain DDL) and `003` (a ledger grant) carry no marker
+and apply under either role. A future migration that needs a master-capable
+role (another `CREATE ROLE`, `ALTER DEFAULT PRIVILEGES`, DDL on master-owned
+tables, or other superuser-only DDL) should carry
+`-- migrate-schema: requires-superuser` on a comment line so the same
 preflight guards it.

scripts/migrate-schema.py

@@ -60,11 +60,11 @@
 
 # A migration may require privileges a least-privilege deploy role (the
 # `migrator` IAM role) does not hold: creating login roles, self-granting role
-# membership, or `ALTER DEFAULT PRIVILEGES FOR ROLE`. The bootstrap migrations
-# `002_iam_db_user.sql`, `004_ingest_role.sql`, and `005_read_role.sql` are exactly
-# this class and MUST be applied by a master-capable role (the RDS master, or a
-# superuser locally) in the one-time bootstrap, BEFORE any `migrator`-connected
-# deploy. That ordering is
+# membership, `ALTER DEFAULT PRIVILEGES FOR ROLE`, or DDL on master-owned
+# tables. The role-bootstrap migrations (002/004/005) and the master-owned-table
+# migrations (006/007) are exactly this class and MUST be applied by a
+# master-capable role (the RDS master, or a superuser locally) BEFORE any
+# `migrator`-connected deploy reaches them. That ordering is
 # documented in each migration's header but was previously enforced nowhere: a
 # `migrator`-connected `apply` reaching `004` would fail deep inside a `DO` block
 # with an opaque `InsufficientPrivilege` and roll the migration back mid-statement.
@@ -113,10 +113,11 @@ def _role_is_master_capable(conn: psycopg.Connection) -> bool:
 def _assert_master_capable(conn: psycopg.Connection, filename: str) -> None:
     """Raise `PermissionError` if the connected role cannot apply a marked migration.
 
-    Enforces the documented bootstrap ordering (the marked `requires-superuser`
-    migrations -- currently `002`/`004`/`005` -- are applied by the master before
-    any `migrator` run) with a clear, actionable message instead of
-    an opaque mid-`DO`-block `InsufficientPrivilege` rollback.
+    Enforces the documented bootstrap ordering (every marked `requires-superuser`
+    migration is applied by the master before any `migrator` run; the marker in
+    each file is authoritative -- see migrations/README.md) with a clear,
+    actionable message instead of an opaque mid-`DO`-block
+    `InsufficientPrivilege` rollback.
     """
     if _role_is_master_capable(conn):
         return
@@ -126,9 +127,9 @@ def _assert_master_capable(conn: psycopg.Connection, filename: str) -> None:
         f"migration {filename} is marked `{_REQUIRES_SUPERUSER_DIRECTIVE}` and must be "
         f"applied by a master-capable role (a superuser, or the RDS master with "
         f"CREATEROLE), but the connected role `{current_user}` has neither rolsuper nor "
-        f"rolcreaterole. Apply all marked `requires-superuser` bootstrap migrations "
-        f"(currently 002/004/005) as the RDS master before any migrator deploy; see "
-        f"migrations/README.md and the migration header."
+        f"rolcreaterole. Apply all marked `requires-superuser` migrations as the RDS "
+        f"master before any migrator deploy; see migrations/README.md and the "
+        f"migration header."
     )
 
 
@@ -221,7 +222,7 @@ def apply(conn: psycopg.Connection, migrations_dir: Path) -> int:
                 "DDL."
             )
         # Bootstrap ordering guard: a migration marked `requires-superuser`
-        # (002/004/005) must be applied by a master-capable role. Check BEFORE
+        # must be applied by a master-capable role. Check BEFORE
         # opening the migration's transaction so a least-privilege `migrator`
         # connection fails loud + early with no partial DDL, rather than rolling
         # back mid-`DO`-block with an opaque InsufficientPrivilege.

scripts/test_migrate_schema.py

@@ -832,7 +832,9 @@ def test_real_migrations_index_columns(conn: psycopg.Connection) -> None:
     )
 
 
-def test_006_backfills_preexisting_query_measurements(conn, tmp_path: Path) -> None:
+def test_006_backfills_preexisting_query_measurements(
+    conn: psycopg.Connection, tmp_path: Path
+) -> None:
     """006's one-time backfill UPDATE fills `commit_timestamp` on PRE-EXISTING rows.
 
     This is the statement that stamped the 4.85M prod rows at apply time, and it is

scripts/test_post_ingest_postgres.py

@@ -330,6 +330,9 @@ def test_query_measurement_insert_populates_commit_timestamp(schema_conn: psycop
 # CONFLICT DO UPDATE SET list owns (dim columns deliberately excluded). Mirrors
 # the SET lists in benchmarks-website/server/src/ingest.rs; a stale/incorrect SET
 # list in any table is caught by the parametrized update test below.
+# `commit_timestamp` is also in the query_measurements SET list but is DERIVED
+# from `commits` (not a record field), so it is pinned by
+# `test_query_measurement_insert_populates_commit_timestamp` instead of here.
 _UPDATE_VALUE_MUTATIONS = {
     "query_measurement": {
         "value_ns": 4242,