diff --git a/.gitignore b/.gitignore index 94b7f06..10d04f6 100644 --- a/.gitignore +++ b/.gitignore @@ -55,3 +55,6 @@ public/static /playwright-report/ /playwright/.cache/ /test-results/ + +# Python bytecode +__pycache__/ diff --git a/bench/CONTRACT.md b/bench/CONTRACT.md index 8bca548..3cebf9b 100644 --- a/bench/CONTRACT.md +++ b/bench/CONTRACT.md @@ -78,6 +78,10 @@ best-effort — a failure fails the bench job and alerts): 1. **Commit upsert:** the writer builds the commit row from `git show` and upserts it (`ON CONFLICT (commit_sha) DO UPDATE`) before applying any record. + The row's `branch` column (migration 012) defaults to `develop`, the only branch the + ingest role may write from today, so the writer does not send it. The read service charts + only `develop` commits (`web/lib/branch.ts`); a writer that ingests another branch must set + `branch` on the commit row. 2. **Direct write:** `INSERT … ON CONFLICT (measurement_id) DO UPDATE` into RDS as the least-privilege `bench_ingest` IAM role (IAM auth, `sslmode=verify-full`), against the schema in this repo's `migrations/`. Each JSONL file applies in one transaction. diff --git a/bench/infra/README.md b/bench/infra/README.md index cbeca4b..67faa7c 100644 --- a/bench/infra/README.md +++ b/bench/infra/README.md @@ -22,6 +22,7 @@ Single one-shot script. Provisions, in order: 7. The GitHub OIDC provider `token.actions.githubusercontent.com` (account-scoped — created once if not present). 8. An IAM role `GitHubBenchmarkSchemaRole` trusted to GitHub Actions OIDC for `vortex-data/vortex.dev` on `main` with `sts:AssumeRoleWithWebIdentity`. Permission policy: `rds-db:connect` scoped to the `migrator` Postgres user on the **instance** resource only (CI schema deploys connect to the public instance endpoint). 9. An IAM role `GitHubBenchmarkIngestRole` (PR-2.1) trusted to the same OIDC provider for `vortex-data/vortex` on `develop`. Permission policy: `rds-db:connect` scoped to the `bench_ingest` Postgres user on the instance resource only. This is the dedicated least-privilege identity for the Phase-2 CI dual-write ingest path, deliberately separate from the schema-deploy `migrator` identity so the high-frequency ingest path can write data but never run DDL or migrations. +10. An IAM role `GitHubBenchmarkReadRole` trusted to the same OIDC provider for `vortex-data/vortex` on any branch and on the repository's own pull requests. Permission policy: `rds-db:connect` scoped to the `bench_ci_read` Postgres user on the instance resource only. CI uses it to read the published numbers and diff them against a run's local numbers. `bench_ci_read` (migration `011`) can only `SELECT` the six data tables, and every one of its transactions is read-only. The `migrator` Postgres user is created by `migrations/002_iam_db_user.sql` (PR-1.3) and the `bench_ingest` user by `migrations/004_ingest_role.sql` (PR-2.1); each OIDC role's permission ARN is pre-scoped to its user. `002` is applied as the RDS master because it creates a role (it needs `CREATEROLE` and grants the `rds_iam` / schema privileges the master holds), while `003` and `004` are applied as the master because they additionally grant on master-owned objects (the ledger and the six data tables, respectively). All three run during the one-time bootstrap; the migrator-run `schema-deploy` path then records them as already-applied. @@ -84,6 +85,7 @@ The script prints the exact `gh variable set` commands; copy them from its outpu | `RDS_BENCH_DB_NAME` | `vortex_bench` | All Postgres connections | | `GH_BENCH_SCHEMA_ROLE_ARN` | the schema-deploy OIDC role ARN | `.github/workflows/bench-schema-deploy.yml` | | `GH_BENCH_INGEST_ROLE_ARN` | the ingest OIDC role ARN | PR-2.4 dual-write ingest workflows (`bench.yml` / `sql-benchmarks.yml` / `v3-commit-metadata.yml`) | +| `GH_BENCH_READ_ROLE_ARN` | the read-only OIDC role ARN | `vortex-data/vortex` CI jobs that diff against the published numbers, as Postgres user `bench_ci_read` | The Vercel reader also connects to the public instance endpoint. It is configured through `BENCH_DB_*` Vercel env vars (see `bench/web/README.md`), @@ -319,6 +321,8 @@ aws iam delete-role-policy --role-name GitHubBenchmarkSchemaRole --policy-name r aws iam delete-role --role-name GitHubBenchmarkSchemaRole aws iam delete-role-policy --role-name GitHubBenchmarkIngestRole --policy-name rds-db-connect-ingest aws iam delete-role --role-name GitHubBenchmarkIngestRole +aws iam delete-role-policy --role-name GitHubBenchmarkReadRole --policy-name rds-db-connect-ci-read +aws iam delete-role --role-name GitHubBenchmarkReadRole aws ec2 delete-security-group --group-name vortex-bench-sg aws rds delete-db-subnet-group --db-subnet-group-name vortex-bench-subnet-group # OIDC provider (account-scoped): only delete if no other workflow uses it @@ -368,14 +372,21 @@ it had no client connections from 2026-05-31 to 2026-10-28. `vortex-data/benchmarks-website` repository are excluded. Manual schema deploys must select `main`. The old shared `GITHUB_REPO` input is rejected to prevent an ambiguous role assignment. +The read role is the exception. `READ_GITHUB_REPO` defaults to `vortex-data/vortex`, and the role +trusts `repo::ref:refs/heads/*` and `repo::pull_request`, so a benchmark run on a PR +or a feature branch can read the published numbers. That is safe because its database user cannot +write. GitHub does not issue an OIDC token to a `pull_request` run from a fork, so fork PRs cannot +assume it. + Reprovisioning replaces each role's trust policy with these inputs. It does not preserve manual subjects. Review both generated policies offline before applying infrastructure changes: ```bash bash infra/provision.sh --print-trust schema bash infra/provision.sh --print-trust ingest +bash infra/provision.sh --print-trust read python3 -m unittest discover -s infra/tests -v ``` The print commands require only `jq` and make no AWS calls. They do not apply the policy. -The schema and ingest permission policies remain scoped to their respective database users. +The schema, ingest and read permission policies remain scoped to their respective database users. diff --git a/bench/infra/provision.sh b/bench/infra/provision.sh index ffb1bd8..3882351 100755 --- a/bench/infra/provision.sh +++ b/bench/infra/provision.sh @@ -3,8 +3,8 @@ # SPDX-FileCopyrightText: Copyright the Vortex contributors # # One-time bootstrap of the bench AWS infrastructure: RDS Postgres, its -# backups and alarms, and the GitHub Actions OIDC roles for schema deploys -# and ingest. Idempotent: safe to re-run; each step checks for an existing +# backups and alarms, and the GitHub Actions OIDC roles for schema deploys, +# ingest and CI reads. Idempotent: safe to re-run; each step checks for an existing # resource and skips creation if found. See infra/README.md for the # full operator runbook and infra/RESTORE.md for restores. # @@ -41,15 +41,18 @@ # (free storage, CPU, connections) and an RDS event subscription # (failure, low storage). Subscribes `ALARM_HTTPS_ENDPOINT` (the # incident.io CloudWatch alert source) when it is set. -# 9. Creates two GitHub Actions OIDC IAM roles trusted to -# `token.actions.githubusercontent.com`, each scoped to one branch of its -# repository and one instance dbuser: +# 9. Creates three GitHub Actions OIDC IAM roles trusted to +# `token.actions.githubusercontent.com`, each scoped to one instance dbuser: # - `GitHubBenchmarkSchemaRole`: `rds-db:connect` on the `migrator` -# Postgres user (schema deploys; migration 002 in PR-1.3). +# Postgres user (schema deploys; migration 002 in PR-1.3). One branch. # - `GitHubBenchmarkIngestRole`: `rds-db:connect` on the `bench_ingest` # Postgres user (Phase-2 CI dual-write ingest; migration 004 in PR-2.1). +# One branch. +# - `GitHubBenchmarkReadRole`: `rds-db:connect` on the read-only +# `bench_ci_read` Postgres user (CI diffs against published numbers; +# migration 011). Any branch, and same-repository pull requests. # 10. Prints a summary: the GitHub Actions vars to set (instance endpoint, -# region, DB name, both role ARNs). +# region, DB name, the role ARNs). # # Re-run safety: every aws-cli mutation goes through `ensure_*` # functions that check for an existing resource first. A partially @@ -93,10 +96,12 @@ readonly DB_LOG_RETENTION_DAYS="${DB_LOG_RETENTION_DAYS:-30}" readonly ALARM_HTTPS_ENDPOINT="${ALARM_HTTPS_ENDPOINT:-}" readonly SCHEMA_ROLE_NAME="${SCHEMA_ROLE_NAME:-GitHubBenchmarkSchemaRole}" readonly INGEST_ROLE_NAME="${INGEST_ROLE_NAME:-GitHubBenchmarkIngestRole}" +readonly READ_ROLE_NAME="${READ_ROLE_NAME:-GitHubBenchmarkReadRole}" readonly SCHEMA_GITHUB_REPO="${SCHEMA_GITHUB_REPO:-vortex-data/vortex.dev}" readonly SCHEMA_GITHUB_BRANCH="${SCHEMA_GITHUB_BRANCH:-main}" readonly INGEST_GITHUB_REPO="${INGEST_GITHUB_REPO:-vortex-data/vortex}" readonly INGEST_GITHUB_BRANCH="${INGEST_GITHUB_BRANCH:-develop}" +readonly READ_GITHUB_REPO="${READ_GITHUB_REPO:-vortex-data/vortex}" # Postgres role created by migrations/002 in PR-1.3; the OIDC role's # rds-db:connect permission is scoped to this user. NOT overridable (unlike the # other names above): the role name is hardcoded in migrations/002 @@ -111,6 +116,9 @@ readonly PG_MIGRATOR_ROLE="migrator" # authenticate as `bench_ingest`, so an env override would scope the IAM grant to # a user the migration never creates and silently break ingest auth. readonly PG_INGEST_ROLE="bench_ingest" +# Postgres role created by migrations/011: read-only and IAM-only. NOT +# overridable, same rationale as PG_MIGRATOR_ROLE. +readonly PG_CI_READ_ROLE="bench_ci_read" readonly TAG_KEY_PROJECT="Project" readonly TAG_VAL_PROJECT="vortex-benchmarks" @@ -532,7 +540,7 @@ ensure_alarms() { } # ----------------------------------------------------------------------------- -# Step 7: GitHub Actions OIDC IAM roles for schema deploys and ingest. +# Step 7: GitHub Actions OIDC IAM roles for schema deploys, ingest and CI reads. # ----------------------------------------------------------------------------- ensure_oidc_provider() { @@ -571,6 +579,27 @@ github_trust_policy() { }' } +# The read role can only SELECT, so it trusts every branch and the repository's +# own pull requests. GitHub issues no OIDC token to a `pull_request` run from a +# fork, so the `pull_request` subject admits only branches in the repository. +github_read_trust_policy() { + jq -n --arg account "$TARGET_ACCOUNT" --arg repo "$1" '{ + Version: "2012-10-17", + Statement: [{ + Effect: "Allow", + Principal: {Federated: ("arn:aws:iam::" + $account + ":oidc-provider/token.actions.githubusercontent.com")}, + Action: "sts:AssumeRoleWithWebIdentity", + Condition: { + StringEquals: {"token.actions.githubusercontent.com:aud": "sts.amazonaws.com"}, + StringLike: {"token.actions.githubusercontent.com:sub": [ + ("repo:" + $repo + ":ref:refs/heads/*"), + ("repo:" + $repo + ":pull_request") + ]} + } + }] + }' +} + ensure_schema_role() { log "Step 7: GitHub Actions OIDC role ${SCHEMA_ROLE_NAME}." ensure_oidc_provider @@ -676,6 +705,57 @@ EOF log " inline permissions policy applied (rds-db:connect for ${PG_INGEST_ROLE} on instance)." } +ensure_read_role() { + log "Step 7c: GitHub Actions OIDC role ${READ_ROLE_NAME}." + ensure_oidc_provider + + local trust_policy + trust_policy=$(github_read_trust_policy "$READ_GITHUB_REPO") + + if READ_ROLE_ARN=$(aws iam get-role --role-name "$READ_ROLE_NAME" \ + --query 'Role.Arn' --output text 2>/dev/null); then + log " exists: ${READ_ROLE_ARN}; updating trust policy." + aws iam update-assume-role-policy \ + --role-name "$READ_ROLE_NAME" \ + --policy-document "$trust_policy" + else + READ_ROLE_ARN=$(aws iam create-role \ + --role-name "$READ_ROLE_NAME" \ + --assume-role-policy-document "$trust_policy" \ + --description "GitHub Actions OIDC role for read-only benchmarks-website Postgres access" \ + --tags "Key=${TAG_KEY_PROJECT},Value=${TAG_VAL_PROJECT}" \ + "Key=${TAG_KEY_OWNER},Value=${TAG_VAL_OWNER}" \ + --query 'Role.Arn' --output text) + log " created: ${READ_ROLE_ARN}" + fi + + # rds-db:connect for the bench_ci_read Postgres user on the INSTANCE resource + # only. That user's grants (migrations/011) are what make it read-only. + local rds_arn_account="$TARGET_ACCOUNT" + + local permissions_policy + permissions_policy=$(cat </dev/null + log " inline permissions policy applied (rds-db:connect for ${PG_CI_READ_ROLE} on instance)." +} + # ----------------------------------------------------------------------------- # Step 8: Summary. # ----------------------------------------------------------------------------- @@ -694,6 +774,7 @@ Backup replica : ${BACKUP_REPLICA_REGION}, ${BACKUP_REPLICA_RETENTION_DAYS} da Alarm topic : ${ALARM_TOPIC_ARN} Schema role ARN : ${SCHEMA_ROLE_ARN} Ingest role ARN : ${INGEST_ROLE_ARN} +Read role ARN : ${READ_ROLE_ARN} NEXT STEPS: @@ -706,6 +787,7 @@ NEXT STEPS: gh variable set RDS_BENCH_DB_NAME --body "${DB_NAME}" gh variable set GH_BENCH_SCHEMA_ROLE_ARN --body "${SCHEMA_ROLE_ARN}" gh variable set GH_BENCH_INGEST_ROLE_ARN --body "${INGEST_ROLE_ARN}" + gh variable set GH_BENCH_READ_ROLE_ARN --body "${READ_ROLE_ARN}" 2. If ALARM_HTTPS_ENDPOINT was unset, subscribe the incident.io CloudWatch alert source to the alarm topic: @@ -717,11 +799,11 @@ NEXT STEPS: --query 'DBInstances[0].[DBInstanceStatus,IAMDatabaseAuthenticationEnabled,DeletionProtection,MaxAllocatedStorage]' # expected: available True True ${DB_MAX_ALLOCATED_STORAGE_GB} -4. Migrations create the Postgres '${PG_MIGRATOR_ROLE}' (002) and - '${PG_INGEST_ROLE}' (004) users; both OIDC roles' permission policies are +4. Migrations create the Postgres '${PG_MIGRATOR_ROLE}' (002), + '${PG_INGEST_ROLE}' (004) and '${PG_CI_READ_ROLE}' (011) users; the OIDC roles' permission policies are already scoped to those users. During the one-time bootstrap, apply EVERY migration carrying the '-- migrate-schema: requires-superuser' marker as the - RDS master (currently 002/004/005, which create roles + grants, and 006/007, + RDS master (currently 002/004/005/011, which create roles + grants, and 006/007, which run DDL on the master-owned query_measurements table). The marker in each file is authoritative; see bench/migrations/README.md 'Bootstrap ordering' for the contract. After the bootstrap, unmarked schema @@ -736,17 +818,18 @@ EOF # ----------------------------------------------------------------------------- main() { - [ -z "${GITHUB_REPO+x}" ] || die "GITHUB_REPO was replaced by SCHEMA_GITHUB_REPO and INGEST_GITHUB_REPO." + [ -z "${GITHUB_REPO+x}" ] || die "GITHUB_REPO was replaced by SCHEMA_GITHUB_REPO, INGEST_GITHUB_REPO and READ_GITHUB_REPO." if [ "$#" -gt 0 ]; then if [ "$#" -eq 2 ] && [ "$1" = "--print-trust" ]; then case "$2" in schema) github_trust_policy "$SCHEMA_GITHUB_REPO" "$SCHEMA_GITHUB_BRANCH" ;; ingest) github_trust_policy "$INGEST_GITHUB_REPO" "$INGEST_GITHUB_BRANCH" ;; - *) die "Expected schema or ingest." ;; + read) github_read_trust_policy "$READ_GITHUB_REPO" ;; + *) die "Expected schema, ingest or read." ;; esac return fi - die "Usage: provision.sh [--print-trust schema|ingest]" + die "Usage: provision.sh [--print-trust schema|ingest|read]" fi verify_prereqs discover_default_vpc @@ -759,6 +842,7 @@ main() { ensure_alarms ensure_schema_role ensure_ingest_role + ensure_read_role print_summary } diff --git a/bench/infra/tests/test_oidc_trust.py b/bench/infra/tests/test_oidc_trust.py index fee34a5..812afa6 100644 --- a/bench/infra/tests/test_oidc_trust.py +++ b/bench/infra/tests/test_oidc_trust.py @@ -13,7 +13,7 @@ class TrustTests(unittest.TestCase): def render(self, role, **overrides): env = {k: v for k, v in os.environ.items() - if k not in {"GITHUB_REPO", "SCHEMA_GITHUB_REPO", "INGEST_GITHUB_REPO", + if k not in {"GITHUB_REPO", "SCHEMA_GITHUB_REPO", "INGEST_GITHUB_REPO", "READ_GITHUB_REPO", "SCHEMA_GITHUB_BRANCH", "INGEST_GITHUB_BRANCH", "TARGET_ACCOUNT"}} env.update(overrides) return subprocess.run(["bash", str(SCRIPT), "--print-trust", role], @@ -43,6 +43,19 @@ def test_repository_inputs_are_independent(self): self.assertEqual(condition["StringEquals"]["token.actions.githubusercontent.com:sub"], f"repo:example/{repo}:ref:refs/heads/{branch}") + def test_read_role_admits_branches_and_repository_pull_requests(self): + result = self.render("read") + self.assertEqual(result.returncode, 0, result.stderr) + statement, = json.loads(result.stdout)["Statement"] + self.assertEqual(statement["Action"], "sts:AssumeRoleWithWebIdentity") + self.assertEqual(statement["Condition"], { + "StringEquals": {"token.actions.githubusercontent.com:aud": "sts.amazonaws.com"}, + "StringLike": {"token.actions.githubusercontent.com:sub": [ + "repo:vortex-data/vortex:ref:refs/heads/*", + "repo:vortex-data/vortex:pull_request", + ]}, + }) + def test_shared_legacy_input_is_rejected(self): result = self.render("schema", GITHUB_REPO="example/old") self.assertNotEqual(result.returncode, 0) diff --git a/bench/migrations/011_ci_read_role.sql b/bench/migrations/011_ci_read_role.sql new file mode 100644 index 0000000..c3512dd --- /dev/null +++ b/bench/migrations/011_ci_read_role.sql @@ -0,0 +1,73 @@ +-- SPDX-License-Identifier: Apache-2.0 +-- SPDX-FileCopyrightText: Copyright the Vortex contributors + +-- migrate-schema: requires-superuser +-- This migration creates a login role (`CREATE ROLE bench_ci_read`), sets its +-- connection limit, and runs `ALTER DEFAULT PRIVILEGES FOR ROLE migrator` -- all +-- requiring a master-capable executing role, exactly like 004/005. The marker +-- makes `migrate-schema.py` reject a non-master `apply` loudly and early. + +-- Create the `bench_ci_read` login role that `vortex-data/vortex` CI uses to read +-- the published benchmark numbers and diff them against the numbers a run just +-- measured. It is read-only by construction, like `bench_read` (005): USAGE on +-- the schema plus SELECT on the six data tables, nothing else. +-- +-- It is deliberately SEPARATE from `bench_read`. `bench_read` authenticates with +-- a static password because Vercel has no AWS credentials, and on RDS a member of +-- `rds_iam` cannot use a password. GitHub Actions can mint RDS IAM tokens through +-- OIDC, so this role is IAM-only: it is granted `rds_iam`, has no password, and +-- the OIDC role `GitHubBenchmarkReadRole` (`infra/provision.sh`) scopes +-- `rds-db:connect` to it. That role trusts pull-request runs, which is why this +-- identity can never write. +-- +-- Idempotent and substrate-portable, matching 002/004: `CREATE ROLE` is guarded +-- (roles are cluster-global), the `rds_iam` grant is guarded behind an existence +-- check so this also applies on vanilla Postgres, and re-granting an existing +-- privilege is a Postgres no-op. +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'bench_ci_read') THEN + CREATE ROLE bench_ci_read WITH LOGIN; + END IF; + IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'rds_iam') THEN + GRANT rds_iam TO bench_ci_read; + END IF; +END$$; + +-- CI runs share the instance with the website's read path, so cap what a +-- runaway diff query or a burst of parallel jobs can take from it. Every +-- transaction is read-only on top of the grants below. +ALTER ROLE bench_ci_read CONNECTION LIMIT 16; +ALTER ROLE bench_ci_read SET statement_timeout = '60s'; +ALTER ROLE bench_ci_read SET idle_in_transaction_session_timeout = '60s'; +ALTER ROLE bench_ci_read SET default_transaction_read_only = on; + +GRANT USAGE ON SCHEMA public TO bench_ci_read; + +-- SELECT-only on the six data tables. The `_applied_migrations` ledger is +-- excluded, as for `bench_read`. +GRANT SELECT ON commits TO bench_ci_read; +GRANT SELECT ON query_measurements TO bench_ci_read; +GRANT SELECT ON compression_times TO bench_ci_read; +GRANT SELECT ON compression_sizes TO bench_ci_read; +GRANT SELECT ON random_access_times TO bench_ci_read; +GRANT SELECT ON vector_search_runs TO bench_ci_read; + +-- Future `migrator`-created tables grant SELECT automatically, as in 004/005 +-- (see 004 for why the executing role may need to self-grant `migrator`). +DO $$ +DECLARE + self_granted boolean := false; +BEGIN + IF NOT pg_has_role(current_user, 'migrator', 'USAGE') THEN + GRANT migrator TO CURRENT_USER WITH INHERIT TRUE; + self_granted := true; + END IF; + + ALTER DEFAULT PRIVILEGES FOR ROLE migrator IN SCHEMA public + GRANT SELECT ON TABLES TO bench_ci_read; + + IF self_granted THEN + REVOKE migrator FROM CURRENT_USER; + END IF; +END$$; diff --git a/bench/migrations/012_commit_branch.sql b/bench/migrations/012_commit_branch.sql new file mode 100644 index 0000000..e038f45 --- /dev/null +++ b/bench/migrations/012_commit_branch.sql @@ -0,0 +1,20 @@ +-- SPDX-License-Identifier: Apache-2.0 +-- SPDX-FileCopyrightText: Copyright the Vortex contributors + +-- Record which branch each commit's numbers were measured on. Measurements key +-- to a commit through `commit_sha`, so the branch lives on `commits`, and every +-- fact row inherits it through that join. +-- +-- Every row so far was written by the ingest role, whose OIDC trust admits only +-- `develop` pushes, so the default fills the existing rows correctly. It stays +-- the default so the current writer, which does not send a branch, keeps +-- writing `develop` rows. Since PostgreSQL 11 a constant default is stored in the +-- catalog, so this does not rewrite the table. +ALTER TABLE commits + ADD COLUMN IF NOT EXISTS branch TEXT NOT NULL DEFAULT 'develop'; + +-- The read service resolves every chart's x-axis from one branch's commits, +-- newest first; this serves that window the way `idx_commits_timestamp` served +-- the unfiltered one. +CREATE INDEX IF NOT EXISTS idx_commits_branch_timestamp + ON commits (branch, timestamp DESC, commit_sha DESC); diff --git a/bench/migrations/README.md b/bench/migrations/README.md index c9c102a..d78cd11 100644 --- a/bench/migrations/README.md +++ b/bench/migrations/README.md @@ -73,6 +73,13 @@ lexicographic order, which equals numeric order under this convention. used for compression-ratio and logical-throughput estimates. Historical rows remain unchanged. - `010_random_access_open_mode.sql` — distinguishes cached-accessor and per-take reopen timings. Historical rows receive the `cached` mode. +- `011_ci_read_role.sql` — `CREATE ROLE` for the read-only `bench_ci_read` IAM-auth user that + `vortex-data/vortex` CI assumes through `GitHubBenchmarkReadRole` to diff its numbers against + the published ones. `SELECT` on the six data tables, read-only transactions, a 60 s statement + timeout and 16 connections at most. Apply it as the RDS master. +- `012_commit_branch.sql` — adds `commits.branch` (`NOT NULL DEFAULT 'develop'`; every earlier + row came from `develop`) and the `(branch, timestamp DESC, commit_sha DESC)` index the read + service's branch-filtered commit window uses. `migrator` applies it. 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, `005` in the Phase-4 operator-gate work (the v4 @@ -104,7 +111,7 @@ 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`, `007`, and `008`; the marker in +marker comment (currently `002`, `004`, `005`, `006`, `007`, `008`, and `011`; 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 diff --git a/bench/scripts/tests/test_migrate_schema.py b/bench/scripts/tests/test_migrate_schema.py index 71ca5b1..0338d4a 100644 --- a/bench/scripts/tests/test_migrate_schema.py +++ b/bench/scripts/tests/test_migrate_schema.py @@ -107,7 +107,7 @@ def test_real_bootstrap_and_steady_state_permissions(self): self.run_cli("apply", 0, target=bootstrap) self.run_cli("status", 0, target=bootstrap) with psycopg.connect(self.target, autocommit=True) as conn: - for role in ("migrator", "bench_ingest", "bench_read"): + for role in ("migrator", "bench_ingest", "bench_read", "bench_ci_read"): conn.execute(f"ALTER ROLE {role} PASSWORD 'test'") migrator = make_conninfo(self.target, user="migrator") for migration in migrations[bootstrap_end + 1:]: @@ -117,18 +117,27 @@ def test_real_bootstrap_and_steady_state_permissions(self): self.assertIn("0 migration(s) applied", self.run_cli("apply", 0, target=migrator).stderr) (self.migrations / "999_future.sql").write_text("CREATE TABLE future (id int);") self.run_cli("apply", 0, target=migrator) - for role, allowed in (("bench_read", "SELECT"), ("bench_ingest", "SELECT,INSERT,UPDATE")): + for role, allowed in (("bench_read", "SELECT"), ("bench_ci_read", "SELECT"), + ("bench_ingest", "SELECT,INSERT,UPDATE")): with psycopg.connect(make_conninfo(self.target, user=role)) as conn: for privilege in allowed.split(","): self.assertTrue(conn.execute("SELECT has_table_privilege('future', %s)", (privilege,)).fetchone()[0]) self.assertFalse(conn.execute("SELECT has_table_privilege('future', 'DELETE')").fetchone()[0]) self.assertFalse(conn.execute("SELECT has_schema_privilege('public', 'CREATE')").fetchone()[0]) self.assertFalse(conn.execute("SELECT has_table_privilege('_applied_migrations', 'SELECT')").fetchone()[0]) - if role == "bench_read": + if allowed == "SELECT": self.assertFalse(conn.execute("SELECT has_table_privilege('commits', 'INSERT')").fetchone()[0]) else: conn.execute("INSERT INTO future VALUES (1)") conn.execute("UPDATE future SET id = 2") + with psycopg.connect(make_conninfo(self.target, user="bench_ci_read")) as conn: + self.assertEqual(conn.execute("SHOW default_transaction_read_only").fetchone()[0], "on") + self.assertEqual(conn.execute("SHOW statement_timeout").fetchone()[0], "1min") + with psycopg.connect(self.target) as conn: + conn.execute("INSERT INTO commits (commit_sha, timestamp, tree_sha, url) " + "VALUES ('a', now(), 't', 'u')") + self.assertEqual(conn.execute("SELECT branch FROM commits").fetchone()[0], "develop") + conn.rollback() (self.migrations / "999_marked.sql").write_text( "-- migrate-schema: requires-superuser\nCREATE TABLE forbidden (id int);" ) diff --git a/bench/web/lib/branch.ts b/bench/web/lib/branch.ts new file mode 100644 index 0000000..5d0cf90 --- /dev/null +++ b/bench/web/lib/branch.ts @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +/** + * The branch the site charts. Migration 012 records each commit's branch on + * `commits.branch`; reads see only this branch's commits, so numbers measured on + * any other branch never reach a chart, a summary, or a commit window. + */ +export const READ_BRANCH = 'develop'; + +if (!/^[A-Za-z0-9._/-]+$/.test(READ_BRANCH)) { + throw new Error(`READ_BRANCH is spliced into SQL and must be a plain branch name`); +} + +/** `alias.branch = ''`, for a `commits` row named `alias`. */ +export function onReadBranch(alias: string): string { + return `${alias}.branch = '${READ_BRANCH}'`; +} + +/** + * `commits` restricted to [`READ_BRANCH`], usable wherever the table name is. + * Postgres flattens the subquery, so the planner sees a plain filtered scan. + */ +export const BRANCH_COMMITS = `(SELECT * FROM commits WHERE branch = '${READ_BRANCH}')`; + +/** `alias.commit_sha` is one of [`READ_BRANCH`]'s commits, for a fact row that skips the join. */ +export function factOnReadBranch(alias: string): string { + return `${alias}.commit_sha IN (SELECT commit_sha FROM commits WHERE branch = '${READ_BRANCH}')`; +} diff --git a/bench/web/lib/queries.test.ts b/bench/web/lib/queries.test.ts index 3c8b883..ba9481c 100644 --- a/bench/web/lib/queries.test.ts +++ b/bench/web/lib/queries.test.ts @@ -14,8 +14,14 @@ vi.mock('next/cache', () => ({ })); import { READ_API_CACHE_CONTROL } from './cache'; -import { chartPayload, collectFilterUniverse, compareGroupSortKey } from './queries'; -import { chartKeyToSlug, type ChartKey } from './slug'; +import { + chartPayload, + collectFilterUniverse, + compareGroupSortKey, + groupChartPayloads, +} from './queries'; +import { collectGroupSummary } from './summary'; +import { chartKeyToSlug, type ChartKey, type GroupKey } from './slug'; import { type BenchDatabase, commitUrl, @@ -628,3 +634,94 @@ describe.skipIf(!databaseAvailable)('chartPayload seeded-window semantics', () = expect(all?.commits.map((c) => c.sha)).toEqual([D, E]); }); }); + +// Migration 012 records each commit's branch, and the site reads only `develop`. A commit from +// another branch, older or newer than every develop commit, must not reach any read. +describe.skipIf(!databaseAvailable)('reads see only the develop branch', () => { + let container: BenchDatabase; + const OLD_FEATURE = '1'.repeat(40); + const A = 'a'.repeat(40); + const B = 'b'.repeat(40); + const NEW_FEATURE = 'f'.repeat(40); + const chart: ChartKey = { + k: 'QueryMeasurement', + dataset: 'branchy', + dataset_variant: null, + scale_factor: '1', + storage: 'nvme', + query_idx: 1, + }; + const group: GroupKey = { + k: 'QueryGroup', + dataset: 'branchy', + dataset_variant: null, + scale_factor: '1', + storage: 'nvme', + }; + + beforeAll(async () => { + container = await startBenchDatabase(); + const pool = getPool(); + const commits: ReadonlyArray = [ + [OLD_FEATURE, '2026-05-01T12:00:00Z', 'ji/old-feature'], + [A, '2026-05-02T12:00:00Z', null], + [B, '2026-05-03T12:00:00Z', null], + [NEW_FEATURE, '2026-05-04T12:00:00Z', 'ji/new-feature'], + ]; + for (const [sha, ts, branch] of commits) { + // A NULL branch leaves the column to its `develop` default, as the ingest writer does. + await pool.query( + `INSERT INTO commits (commit_sha, timestamp, message, tree_sha, url, branch) + VALUES ($1, $2::timestamptz, 'm', $3, $4, COALESCE($5, 'develop'))`, + [sha, ts, TREE_SHA, commitUrl(sha), branch], + ); + } + // On develop, datafusion is faster. The feature commits flip that, so a read that let + // them through would rank duckdb first. + const rows: ReadonlyArray = [ + [OLD_FEATURE, 'datafusion', 9000], + [A, 'datafusion', 100], + [A, 'duckdb', 200], + [B, 'datafusion', 110], + [B, 'duckdb', 210], + [NEW_FEATURE, 'datafusion', 9000], + ]; + let id = 0; + for (const [sha, engine, value] of rows) { + id += 1; + await pool.query( + `INSERT INTO query_measurements + (measurement_id, commit_sha, dataset, dataset_variant, scale_factor, + query_idx, storage, engine, format, value_ns, all_runtimes_ns, commit_timestamp) + VALUES ($1, $2, 'branchy', NULL, '1', 1, 'nvme', $3, 'parquet', $4, '{1}'::bigint[], + (SELECT timestamp FROM commits WHERE commit_sha = $2))`, + [id, sha, engine, value], + ); + } + }); + + afterAll(async () => { + await resetPool(); + await container.stop(); + }); + + it.each(['1', '100', 'all'])('charts only develop commits for ?n=%s', async (n) => { + const window = parseCommitWindow(n); + const expected = n === '1' ? [B] : [A, B]; + const single = await chartPayload(chart, window); + const [grouped] = await groupChartPayloads(group, [chart], window); + for (const payload of [single, grouped]) { + expect(payload?.commits.map((c) => c.sha)).toEqual(expected); + expect(payload?.history.total_commits).toBe(2); + expect(payload?.series['datafusion:parquet']).toEqual(n === '1' ? [110] : [100, 110]); + } + }); + + it('ranks the summary on develop numbers only', async () => { + const summary = await collectGroupSummary(group); + expect(summary?.type).toBe('queryBenchmark'); + const rankings = summary?.type === 'queryBenchmark' ? summary.rankings : []; + expect(rankings.map((r) => r.name)).toEqual(['datafusion:parquet', 'duckdb:parquet']); + expect(rankings[0]?.totalRuntime).toBe(110); + }); +}); diff --git a/bench/web/lib/queries.ts b/bench/web/lib/queries.ts index 58d2615..85030c6 100644 --- a/bench/web/lib/queries.ts +++ b/bench/web/lib/queries.ts @@ -33,6 +33,7 @@ */ import type { PoolClient } from 'pg'; +import { BRANCH_COMMITS, factOnReadBranch } from './branch'; import { getPool } from './db'; import { randomAccessStorage } from './random-access'; import { groupDescription } from './descriptions'; @@ -266,7 +267,7 @@ async function seededCommitsInWindow( c.url, row_number() OVER (ORDER BY c.timestamp ASC, c.commit_sha ASC) AS rn, (count(*) OVER ())::int AS total_commits - FROM commits c + FROM ${BRANCH_COMMITS} c WHERE c.timestamp >= (${earliest}) ) SELECT commit_sha, @@ -312,7 +313,7 @@ function factWindowFilter(params: QueryParams, window: CommitWindow): string { } return ( ` AND c.commit_sha IN ` + - `(SELECT commit_sha FROM commits ORDER BY timestamp DESC, commit_sha DESC LIMIT ${params.bind(limit)})` + `(SELECT commit_sha FROM ${BRANCH_COMMITS} b ORDER BY timestamp DESC, commit_sha DESC LIMIT ${params.bind(limit)})` ); } @@ -337,9 +338,9 @@ function queryMeasurementWindowFilter(params: QueryParams, window: CommitWindow) return ( ` AND q.commit_timestamp >= ` + `(SELECT min(timestamp) FROM ` + - `(SELECT timestamp FROM commits ORDER BY timestamp DESC, commit_sha DESC LIMIT ${n}) w)` + + `(SELECT timestamp FROM ${BRANCH_COMMITS} b ORDER BY timestamp DESC, commit_sha DESC LIMIT ${n}) w)` + ` AND q.commit_sha IN ` + - `(SELECT commit_sha FROM commits ORDER BY timestamp DESC, commit_sha DESC LIMIT ${n})` + `(SELECT commit_sha FROM ${BRANCH_COMMITS} b ORDER BY timestamp DESC, commit_sha DESC LIMIT ${n})` ); } @@ -451,7 +452,8 @@ async function collectQueryChart( AND ${sargableDimEq(p, 'q2.dataset_variant', dataset_variant)} AND ${sargableDimEq(p, 'q2.scale_factor', scale_factor)} AND q2.storage = ${p.bind(storage)} - AND q2.query_idx = ${p.bind(query_idx)}`, + AND q2.query_idx = ${p.bind(query_idx)} + AND ${factOnReadBranch('q2')}`, window, ); if (seeded.commits.length === 0) { @@ -488,7 +490,7 @@ async function collectCompressionTimeChart( (p) => `SELECT MIN(c2.timestamp) FROM compression_times t2 - JOIN commits c2 ON c2.commit_sha = t2.commit_sha + JOIN ${BRANCH_COMMITS} c2 ON c2.commit_sha = t2.commit_sha WHERE t2.dataset = ${p.bind(dataset)} AND ${sargableDimEq(p, 't2.dataset_variant', dataset_variant)}`, window, @@ -504,7 +506,7 @@ async function collectCompressionTimeChart( SELECT t.commit_sha, t.format, t.op, t.value_ns::float8 AS value FROM compression_times t - JOIN commits c USING (commit_sha) + JOIN ${BRANCH_COMMITS} c USING (commit_sha) WHERE t.dataset = ${params.bind(dataset)} AND ${sargableDimEq(params, 't.dataset_variant', dataset_variant)}${factWindowFilter(params, window)} ORDER BY c.timestamp, t.format, t.op @@ -525,7 +527,7 @@ async function collectCompressionSizeChart( (p) => `SELECT MIN(c2.timestamp) FROM compression_sizes s2 - JOIN commits c2 ON c2.commit_sha = s2.commit_sha + JOIN ${BRANCH_COMMITS} c2 ON c2.commit_sha = s2.commit_sha WHERE s2.dataset = ${p.bind(dataset)} AND ${sargableDimEq(p, 's2.dataset_variant', dataset_variant)}`, window, @@ -541,7 +543,7 @@ async function collectCompressionSizeChart( SELECT s.commit_sha, s.format, s.value_bytes::float8 AS value FROM compression_sizes s - JOIN commits c USING (commit_sha) + JOIN ${BRANCH_COMMITS} c USING (commit_sha) WHERE s.dataset = ${params.bind(dataset)} AND ${sargableDimEq(params, 's.dataset_variant', dataset_variant)}${factWindowFilter(params, window)} ORDER BY c.timestamp, s.format @@ -562,7 +564,7 @@ async function collectRandomAccessChart( (p) => `SELECT MIN(c2.timestamp) FROM random_access_times r2 - JOIN commits c2 ON c2.commit_sha = r2.commit_sha + JOIN ${BRANCH_COMMITS} c2 ON c2.commit_sha = r2.commit_sha WHERE r2.dataset = ${p.bind(dataset)}`, window, ); @@ -579,7 +581,7 @@ async function collectRandomAccessChart( r.open_mode AS open_mode, r.value_ns::float8 AS value FROM random_access_times r - JOIN commits c USING (commit_sha) + JOIN ${BRANCH_COMMITS} c USING (commit_sha) WHERE r.dataset = ${params.bind(dataset)}${factWindowFilter(params, window)} ORDER BY c.timestamp, r.format, open_mode `; @@ -599,7 +601,7 @@ async function collectVectorSearchChart( (p) => `SELECT MIN(c2.timestamp) FROM vector_search_runs v2 - JOIN commits c2 ON c2.commit_sha = v2.commit_sha + JOIN ${BRANCH_COMMITS} c2 ON c2.commit_sha = v2.commit_sha WHERE v2.dataset = ${p.bind(dataset)} AND v2.layout = ${p.bind(layout)} AND v2.threshold = ${p.bind(threshold)}`, @@ -616,7 +618,7 @@ async function collectVectorSearchChart( SELECT v.commit_sha, v.flavor, v.value_ns::float8 AS value FROM vector_search_runs v - JOIN commits c USING (commit_sha) + JOIN ${BRANCH_COMMITS} c USING (commit_sha) WHERE v.dataset = ${params.bind(dataset)} AND v.layout = ${params.bind(layout)} AND v.threshold = ${params.bind(threshold)}${factWindowFilter(params, window)} @@ -684,7 +686,7 @@ async function windowCommits(client: PoolClient, window: CommitWindow): Promise< to_char(timestamp AT TIME ZONE 'UTC', 'YYYY-MM-DD HH24:MI:SS"+00"') AS ts_text, COALESCE(message, '') AS message, url - FROM (SELECT * FROM commits ORDER BY timestamp DESC, commit_sha DESC LIMIT $1) w + FROM (SELECT * FROM ${BRANCH_COMMITS} b ORDER BY timestamp DESC, commit_sha DESC LIMIT $1) w ORDER BY timestamp ASC, commit_sha ASC `; const rows = (await client.query(text, [commitWindowLimit(window)])).rows; @@ -792,6 +794,7 @@ function queryChartEarliest(pins: (alias: string) => string): string { AND q.engine = s.engine AND q.format = s.format AND q.commit_timestamp IS NOT NULL + AND ${factOnReadBranch('q')} ORDER BY ${oldestFirst} LIMIT 1 ) oldest`; @@ -915,12 +918,12 @@ function groupRowsQuery( columns, `SELECT MIN(c2.timestamp) FROM ${table} f2 - JOIN commits c2 ON c2.commit_sha = f2.commit_sha + JOIN ${BRANCH_COMMITS} c2 ON c2.commit_sha = f2.commit_sha WHERE ${pins('f2')}`, `SELECT f.commit_sha, ${fields.map((field) => `f.${field}`).join(', ')}, f.${value}::float8 AS value FROM ${table} f - JOIN commits c USING (commit_sha) + JOIN ${BRANCH_COMMITS} c USING (commit_sha) WHERE ${pins('f')}${factWindowFilter(params, window)}`, ); } @@ -934,14 +937,14 @@ function groupRowsQuery( [['dataset', 'text', keys.map((k) => k.dataset)]], `SELECT MIN(c2.timestamp) FROM random_access_times r2 - JOIN commits c2 ON c2.commit_sha = r2.commit_sha + JOIN ${BRANCH_COMMITS} c2 ON c2.commit_sha = r2.commit_sha WHERE r2.dataset = ch.dataset`, `SELECT r.commit_sha, r.format, r.open_mode AS open_mode, r.value_ns::float8 AS value FROM random_access_times r - JOIN commits c USING (commit_sha) + JOIN ${BRANCH_COMMITS} c USING (commit_sha) WHERE r.dataset = ch.dataset${factWindowFilter(params, window)}`, ); break; @@ -956,14 +959,14 @@ function groupRowsQuery( [['threshold', 'float8', keys.map((k) => k.threshold)]], `SELECT MIN(c2.timestamp) FROM vector_search_runs v2 - JOIN commits c2 ON c2.commit_sha = v2.commit_sha + JOIN ${BRANCH_COMMITS} c2 ON c2.commit_sha = v2.commit_sha WHERE v2.dataset = ${dataset} AND v2.layout = ${layout} AND v2.threshold = ch.threshold`, `SELECT v.commit_sha, v.flavor, v.value_ns::float8 AS value FROM vector_search_runs v - JOIN commits c USING (commit_sha) + JOIN ${BRANCH_COMMITS} c USING (commit_sha) WHERE v.dataset = ${dataset} AND v.layout = ${layout} AND v.threshold = ch.threshold${factWindowFilter(params, window)}`, @@ -973,7 +976,7 @@ function groupRowsQuery( } const text = ` WITH commit_times AS ( - SELECT array_agg(timestamp ORDER BY timestamp) AS sorted FROM commits + SELECT array_agg(timestamp ORDER BY timestamp) AS sorted FROM ${BRANCH_COMMITS} b )${blocks.join('\n UNION ALL')}`; return { text, values: params.values }; } diff --git a/bench/web/lib/summary.ts b/bench/web/lib/summary.ts index ce47ce8..1cd6671 100644 --- a/bench/web/lib/summary.ts +++ b/bench/web/lib/summary.ts @@ -36,6 +36,7 @@ * any sub-second commit timestamp), so the port keeps the timestamp in SQL. */ +import { BRANCH_COMMITS, factOnReadBranch, onReadBranch } from './branch'; import { getPool } from './db'; import { randomAccessStorage, type RandomAccessStorage } from './random-access'; import { compareCodeUnits } from './families'; @@ -430,7 +431,7 @@ async function collectRandomAccessSummary(storage: RandomAccessStorage): Promise r.open_mode AS open_mode, r.value_ns::float8 AS value FROM random_access_times r - JOIN commits c USING (commit_sha) + JOIN ${BRANCH_COMMITS} c USING (commit_sha) WHERE r.value_ns > 0 ORDER BY r.dataset, r.format, r.open_mode, c.timestamp DESC, @@ -488,7 +489,7 @@ async function collectVectorSearchSummary( v.flavor AS series, v.value_ns::float8 AS value FROM vector_search_runs v - JOIN commits c USING (commit_sha) + JOIN ${BRANCH_COMMITS} c USING (commit_sha) WHERE v.dataset = $1 AND v.layout = $2 AND v.value_ns > 0 @@ -618,7 +619,7 @@ async function compressionSamples(): Promise< AND p.dataset = t.dataset AND p.dataset_variant IS NOT DISTINCT FROM t.dataset_variant AND p.op = t.op - JOIN commits c ON c.commit_sha = t.commit_sha + JOIN ${BRANCH_COMMITS} c ON c.commit_sha = t.commit_sha WHERE t.op IN ('encode', 'decode') AND t.format = ANY($1::text[]) AND p.format = $4 @@ -653,7 +654,7 @@ async function compressionSamples(): Promise< s.dataset, s.dataset_variant, s.uncompressed_bytes::float8 AS uncompressed_bytes FROM compression_sizes s - JOIN commits c ON c.commit_sha = s.commit_sha + JOIN ${BRANCH_COMMITS} c ON c.commit_sha = s.commit_sha WHERE s.format = $4 AND s.uncompressed_bytes::float8 > 0 AND lower(s.dataset) NOT LIKE '%wide table%' @@ -781,7 +782,7 @@ async function compressionSizeSamples(): Promise< ON p.commit_sha = s.commit_sha AND p.dataset = s.dataset AND p.dataset_variant IS NOT DISTINCT FROM s.dataset_variant - JOIN commits c ON c.commit_sha = s.commit_sha + JOIN ${BRANCH_COMMITS} c ON c.commit_sha = s.commit_sha WHERE s.format = ANY($1::text[]) AND p.format = $4 AND s.value_bytes > 0 @@ -827,7 +828,7 @@ async function compressionSizeSamples(): Promise< s.dataset, s.dataset_variant, s.uncompressed_bytes::float8 AS uncompressed_bytes FROM compression_sizes s - JOIN commits c ON c.commit_sha = s.commit_sha + JOIN ${BRANCH_COMMITS} c ON c.commit_sha = s.commit_sha WHERE s.format = $4 AND s.uncompressed_bytes::float8 > 0 AND lower(s.dataset) NOT LIKE '%wide table%' @@ -922,6 +923,7 @@ async function collectQuerySummary( // `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. + // Rows of another branch's commits are dropped, as in every other read. // 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 @@ -1004,6 +1006,7 @@ async function collectQuerySummary( AND q.format = s.format AND q.value_ns > 0 AND q.commit_timestamp IS NOT NULL + AND ${factOnReadBranch('q')} ORDER BY ${indexOrder}, q.commit_timestamp DESC LIMIT 1) UNION ALL @@ -1011,6 +1014,7 @@ async function collectQuerySummary( FROM query_measurements q LEFT JOIN commits c ON c.commit_sha = q.commit_sha WHERE ${groupPred} + AND (c.commit_sha IS NULL OR ${onReadBranch('c')}) AND q.query_idx = s.query_idx AND q.engine = s.engine AND q.format = s.format