chore(datastore): bound every application-generated id to UUIDv7

What this does

39 tables declare id uuid with no server-side DEFAULT, so Go supplies the value on every INSERT: datastore.newID returns uuid.Must(uuid.NewV7()), and the call sites that already return an error call uuid.NewV7 directly.

Nothing in the database checked that value. A caller that never assigned the field stored the all-zero UUID cleanly, and that row then read as a legitimate primary key while sorting outside the time-ordered range every other row occupies. A stray uuid.New() would store a version-4 id the same way.

This adds one CHECK per table on the UUID version nibble:

CHECK ((get_byte(uuid_send(id), 6) >> 4) = 7)

Byte 6's high nibble is the version. Measured in psql: the all-zero UUID reads 0, gen_random_uuid() reads 4, and a v7 value reads 7, so the predicate accepts exactly the intended set.

New migration: 20260831103000_add_id_uuid_version_checks.sql, covering 38 of the 39. The 39th, container_virtual_repositories, landed from !2159 (merged) with the same CHECK declared inline; Scope has the detail.

Scope

The 11 tables with no uuid id are excluded, because they have no version nibble to read. blob_storage_blobs, blob_storage_attachments and upload_sessions keep a bigint surrogate fed by a sequence; blob_storage_blobs_by_namespace, repository_collection_repositories and namespace_statistics have no id column at all, the first keying on (namespace_id, sha256); goose_db_version and the four river_* tables belong to tooling that generates its own keys.

container_virtual_repositories is the one exclusion that does have a uuid id. !2159 (merged) merged it with check_container_virtual_repositories_id_uuid_version declared inline in its CREATE TABLE, under this migration's name and predicate, so it needs no ALTER here. That is what makes the schema-wide set 39 and this migration's 38. Measured on PostgreSQL 17 with both migrations applied: the coverage test's catalog query returns 39 tables, and all 39 carry the validated constraint with the pinned definition.

Why now

The tables are empty before the closed beta, so VALIDATE scans nothing and the constraint can be added and validated in one step. After go-live the same change needs NOT VALID plus a separate validation pass over real data.

Transaction and lock budget

NOT VALID followed by VALIDATE is squawk's idiom for adding a CHECK: a bare validating ADD draws constraint-missing-not-valid, measured on this expression. Both run inside goose's transaction rather than the two migrations database-migrations.md prescribes, because the ADD takes ACCESS EXCLUSIVE on the parent and holds it to COMMIT, so a VALIDATE beside it runs under that lock instead of the lighter SHARE UPDATE EXCLUSIVE a standalone VALIDATE would take.

A NOT VALID CHECK on a partitioned parent recurses to all 64 partitions, so each of the 37 partitioned tables costs 65 AccessExclusiveLocks and the unpartitioned namespaces costs 1: 2406 held to COMMIT. Measured, not just derived: replaying the Up section inside one transaction and counting pg_locks for the backend reports 2406. max_locks_per_transaction does not cap that, it sizes the shared lock table together with max_connections.

-- +goose NO TRANSACTION with one statement per table was considered and rejected: it would cap the peak at 65, but ADD CONSTRAINT has no IF NOT EXISTS form, so a mid-way failure would leave a partial set applied with no goose version recorded and no safe replay. Atomicity is worth more than the narrower lock window on a migration that only ever runs against an empty database. A data-bearing version of this change would have to go one table per migration and answer the lock_timeout question (#548) first.

Database Review Evidence

Migrations

Note

Timings are against an empty database, in apply / rollback order per PG version. Production-scale validation via Database Lab is not yet available. See Database review evidence for the matrix rationale and how to read the numbers.

Important

The numbers below are a local re-measurement, taken after the rebase onto main. The earlier CI matrix in this description covered 37 tables and no longer describes the migration, so it has been replaced rather than kept alongside. The db:migrate matrix on the current head pipeline is the authority and this table is replaced with its numbers once it reports.

Each cell is one fresh postgres:<version>-alpine container at its default max_locks_per_transaction=64, migrated up to the preceding version, then goose up and goose down on this migration alone:

Migration PG 16 PG 17 PG 18
20260831103000_add_id_uuid_version_checks.sql OK (341.90ms / 261.55ms) OK (348.07ms / 264.43ms) OK (352.03ms / 263.23ms)

Migration notes:

  • All three versions apply well inside the budget, and the spread across them is small. 342ms, 348ms and 352ms, far inside the 5-minute per-migration boot budget (Time budget) and nowhere near the 2x bar for a version-specific regression. There is no production-scale amplification to worry about, because the tables are empty whenever this migration runs, so VALIDATE scans nothing and the work is catalog-only. No action proposed.
  • Rollback is faster than apply on all three (about 263ms against about 347ms). The Down section runs 38 DROP CONSTRAINT statements against the Up section's 76 ADD plus VALIDATE ones, and each recurses to 64 partitions, so the asymmetry is catalog bookkeeping rather than data work. No action proposed.
  • The fixture debt this constraint exposed is already on main. Every fixture that seeded id with SQL gen_random_uuid() mints a version-4 UUID the constraint refuses. Those sites were converted to UUIDv7 in !2113 (merged), which has merged, so this MR now targets main directly. No production code was affected: the only gen_random_uuid() references in non-test Go are comments explaining why it is avoided.

Tests

New suite internal/datastore/migrations/id_uuid_version_schema_integration_test.go:

Test What it pins
CoversEveryApplicationGeneratedIDTable every table whose uuid id has no default carries the constraint, with the exact pg_get_constraintdef output, validated
RecursesToEveryPartition all 64 partitions of repositories inherit it, validated
RejectsIDsThatAreNotVersion7 real inserts: v7 accepted; all-zero and v4 rejected, naming the declared constraint
LeavesTheExcludedTablesAlone all six excluded tables carry none, in the two groups the migration excludes them for
MigrationTextIsTransactional no NO TRANSACTION directive, no batching DO $$ block, IF EXISTS on every Down drop
DownReversesTheUp from the text alone: the Up's ADD set, its VALIDATE set and the Down's DROP set name the same tables and constraints, and every DROP carries IF EXISTS

The coverage test derives its table set from the catalog rather than carrying a list. That is the property worth having: a table added later with a uuid id and no default fails this suite until it carries the constraint, which no hand-maintained list would catch, and the suite needs no edit when a table is added correctly.

The suite's reasoning lives in a sidecar, id_uuid_version.md, because scripts/ci/check-comment-caps.sh caps every comment block in a _test.go file at two lines. This follows repository_parent_gate.md, which exists for the same reason.

Four existing suites change with the constraint, each renamed and inverted rather than deleted:

Suite Was Now
container_remote_manifest_relationships_constraints_integration_test.go ..._NoCheckConstraints ..._CarriesOnlyTheIDVersionCheck
npm_virtual_repositories_rowshapes_integration_test.go ..._NoCheckConstraints ..._CarriesOnlyTheIDVersionCheck
npm_virtual_repository_upstreams_rowshapes_integration_test.go ..._NoOtherCheckConstraints, ..._AcceptsAnyIDShape ..._CarriesOnlyPositionAndIDVersionChecks, ..._IDShapes
npm_virtual_upstream_rules_*_integration_test.go three CHECKs, ..._AcceptsAnyIDShape four CHECKs, ..._IDShapes

The rules-table pair is the one the rebase added. Its CheckShapes test asserted three CHECKs and named work item 983 as the change that makes it four; it now asserts four and covers the id rule by name and by rendered definition, and NoOtherCheckConstraints counts the fourth across the parent and all 64 partitions. Both _IDShapes suites keep every case they had: the v4 id asserts refusal and names the constraint, and the ids equal to another uuid column on the row still assert acceptance. No subtest was dropped.

Green locally against PostgreSQL 17: the whole of internal/datastore/migrations and the whole of internal/managementapi, both with -tags=integration -count=1, plus scripts/conformance/maven-provision. golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false over both packages reports nothing on any file this MR touches, and squawk reports nothing on the migration.

Reviewable LOC

Past the 500-line guidance, so the split by file group:

File group Lines Notes
structure.sql 3121 / 715 generated by mise run db:dump-structure, not hand-written
migration SQL 358 76 statements plus one comment block
new test file 254 five tests and two helpers, before the round-4 additions below
id_uuid_version.md 151 the suite's rationale, prose only
container_remote_manifest_relationships_constraints.md 84 prose displaced by the comment cap, see below
four rowshape and schema suites 142 / 83 the renames and inverted assertions
bulk_container_worker_integration_test.go 13 / 109 the zero-id fixture, retired
docs and skill prose 15 / 14 pre-landing hedges the CHECK closes
migrations_checksum_test.go 1 knownHeadVersion bump

Splitting would not help. The 38 constraints are one atomic transaction, so dividing the SQL by format would create four migrations that must all land before the constraint is complete, and the coverage test derives its set from the catalog, so it would fail until the last one merged. The bulk of the migration is 76 near-identical statements that review by spot-check.

Not affected

No e2e scenario changes. A correct client never sends an id at all, so no request-level behavior changes; the constraint only refuses a write the application should not have made. No jet regeneration: a CHECK constraint does not appear in generated types, matching !1944 (merged), which added CHECK constraints and touched no gen/ file.

ADR conformance and the ADR-007 amendment

This change conforms; there is no deviation to escalate. ADR-007's opening rule already covered every uuid id with no server-side default, and its table diagrams annotate 37 of these 38 columns "UUIDv7, application-generated" (namespace_encryption_keys appears in no diagram). So the constraint enforces what the ADR states rather than departing from it.

The amendment is still worth having, for a different reason than conformance: three committed migrations record the absence of this CHECK as a deliberate reading, and lint:migration-immutability means they cannot be corrected in place.

Migration What it records
20260826105942_create_npm_virtual_repositories.sql "id may be any UUID version"
20260828102654_create_npm_virtual_repository_upstreams.sql "Any UUID version in id, and id equal to any of the other three uuid columns"
20260829134210_create_npm_virtual_upstream_rules.sql "Any UUID version in id, and id equal to either of the other two uuid columns"

A fourth, 20260814150300_create_container_remote_manifest_relationships.sql, records "No CHECK constraint either, which makes this the one table in the family without one", which this change also falsifies.

handbook!20939 has merged. It records the constraint as an amendment and corrects one sentence that described namespace_id as leading the composite primary keys when it follows id. Because it landed first, upstream ADR-007 now asserts that the schema enforces the version, which stays false until this merge request merges; merging promptly closes that window. Treating the amendment as a merge gate was the cautious call rather than a required one, since there was no deviation to escalate.

Documented absences this closes

Two suites asserted their table carried no CHECK, on the grounds that every column is uuid and a uuid's type is its whole domain. A third, on npm_virtual_repository_upstreams, asserted instead that its one position rule was the whole set, so the all-uuid reason never applied to it: that table carries a non-uuid position column. That reason still holds for every value column. What all three missed is that a version rule bounds id, and id was the one column nothing held.

The tests are renamed and their assertions inverted, not deleted:

Was Now Change
TestContainerRemoteManifestRelationshipsConstraints_NoCheckConstraints ..._CarriesOnlyTheIDVersionCheck 0 → exactly 1, and it is the id rule
TestNPMVirtualRepositoriesSchema_NoCheckConstraints ..._CarriesOnlyTheIDVersionCheck 0 → 65 (parent + 64 partitions)
TestNPMVirtualUpstreamsSchema_NoOtherCheckConstraints ..._CarriesOnlyPositionAndIDVersionChecks 65 → 130, counted per rule
TestNPMVirtualUpstreamsConstraints_AcceptsAnyIDShape ..._IDShapes the v4 case now asserts refusal

_IDShapes keeps all four of its cases. The v4 id asserts refusal and names the constraint; the three ids equal to another uuid column on the row still assert acceptance, because nothing compares those columns. No subtest was dropped.

One judgment call worth a reviewer's eye. Renaming the container test forced two long comment blocks in its file to the two-line cap (scripts/ci/check-comment-caps.sh applies the cap to any block the diff touches, and blank-separated paragraphs count as one block). Rather than delete that prose I moved it to a sidecar, container_remote_manifest_relationships_constraints.md, following repository_parent_gate.md. Nothing was dropped, but it is a larger edit to that file than the constraint alone needs, and narrowing the constraint to skip the table was the alternative I rejected: uniformity across all 38 tables is the point, and a table excluded on the grounds that its tests were inconvenient is the weakest possible reason.

Review pass (commit 3)

A branch review found the constraint, the 38-table set and the 2406 lock figure correct, and several claims around them wrong. 9463c99 fixes them. The three that mattered most sit in the migration file, which lint:migration-immutability freezes at merge:

  • blob_storage_blobs_by_namespace was grouped with the bigint-surrogate tables. It has no id column at all.
  • The require-timeout-settings suppression said three migrations set a lock_timeout and all three bound an ATTACH PARTITION phase. There are four, and the fourth bounds a DROP TABLE run.
  • The ADR-007 diagram claim was false for namespace_encryption_keys.

The lock note now says that an ungranted ACCESS EXCLUSIVE queues readers behind it, which the precedent it cites spells out at a nineteenth of the lock count, and the Down section gains the PGOPTIONS='-c lock_timeout=5s' mise run db:rollback guidance it had none of. id_uuid_version.md gains the locating query and recovery order for a VALIDATE refusal.

One behaviour change. The npm, Maven and container remote-repository classifiers match SQLSTATE 23514 without discriminating on constraint name, documented as feeding a future client-input mapping. The id version rule bounds a column the application mints and no request carries, so isIDUUIDVersionCheck now excludes it and it falls through to the generic wrap instead of a client-facing sentinel. Nothing outside internal/datastore consumes those sentinels today, so no live behaviour changes; unit cases cover all three classifiers plus the suffix predicate.

One cost of that narrowing, named so the grandfathered path is not mistaken for the intended target. The refusal used to render as constant text, groupable as error_message; the generic wraps it now falls through to interpolate namespace=%s, repository=%s, so for this SQLSTATE error_message stops being groupable. maven_remote_repositories_errors.go names the identifier-free error strings rule in docs/dev/database-query-patterns.md and records the npm twin as grandfathered against it. The practical cardinality is nil, because no production path can mint a non-v7 id: every insert binds datastore.newID.

The zero-id decision, stated explicitly. The CHECK makes a stored uuid.Nil unreachable, so both defensive sites could have been retired. They are kept: the marker's argument guard still covers a caller-supplied uuid.Nil in the batch args, which the schema does not police, and the scope read's conditional arm reduces to an ordinary first-page sentinel. Their comments now say that rather than resting on "nothing in the database checks id".

docs/dev/database.md's new-table skeleton now carries the constraint, so the next uuid id table does not fail the catalog-derived coverage test from a file its author never opened.

Verified locally on PostgreSQL 17: internal/datastore/migrations and internal/managementapi both green with -tags=integration -count=1, golangci-lint with --build-tags=integration reports nothing on any of the 21 changed files, check-comment-caps.sh --base origin/main passes, and squawk reports nothing on the migration.

Review pass (round 4)

A second branch review raised twelve findings. Eight are fixed on this branch; the rest are recorded in their own threads.

  • The Down's lock footprint was understated by 2x. DROP CONSTRAINT takes an object lock on each pg_constraint row on top of the relation lock, so the Down holds 4815 distinct lock objects to the Up's 2409. The Up's 2406 is correct and stays, because two lock modes on one relation share a single lock-table entry.
  • The Down's PGOPTIONS advice cited an inverted precedent. It borrowed a "no session to SET in" reason from a migration that is NO TRANSACTION in both sections, which is the opposite of this transactional Down. It now says the real reason: this file declines the in-file bound and leaves the schema-wide timeout question to #548, the position the Up already takes.
  • "The tables are empty whenever it runs" was unconditional. Migrations run in-process at pod boot and staging has served Maven and OCI round trips since 2026-08-24, so there VALIDATE scans real rows. The premise is now scoped per environment, and the recovery section names the literal-7 digest derivation (which is the namespace case, not overlay) and adds scripts/conformance/npm-e2e.sh to the seed list.
  • The Down had one statement of coverage out of 38, and no job runs goose down. DownReversesTheUp now compares the Up's ADD and VALIDATE sets against the Down's DROP set from the migration text, with no database.
  • The exclusion loop stated a reason false for one of its four tables and was neither set cleanly. It now covers all six excluded tables in two groups, each with the reason that holds for it, and the sidecar sentence matches.
  • The credential-update fallback lost its row-data guard. With the id version CHECK unclassified, that refusal reaches a wrap that was the only one on these three tables with no pgErrorCarriesRowData guard, on a table whose failing row echoes the plaintext pair. Restored, mirroring containerRemoteWriteError, with unit coverage.
  • Three checkable claims in container_remote_manifest_relationships_constraints.md did not hold: two counting conventions seventeen lines apart, a pointer at a file this MR emptied, and a missing record of the spelled-out constraint name that docs/dev/database.md asks for.

The stored zero-id fixture

Decision: retired. seedZeroIDImage dropped check_container_images_id_uuid_version to insert uuid.Nil and replayed the constraint on cleanup. The mechanics were sound and the isolation held, but an in-tree helper that lifts a production CHECK is a copyable precedent for disabling a schema constraint inside a test, and the state it built is unreachable once this migration lands. It is gone, together with the subtest that needed a stored row and with seedImageWithID, whose only reason to be separate from seedImage was that caller.

The reachable half is untouched. A subset batch can still name uuid.Nil, which needs no stored row, and the "the zero id is a skip that never reaches the marker" case of TestContainerBulkWorker_SubsetImages_AppliesResolvedEntriesOnly covers it. The scope read's conditional cursor arm keeps only its "start the pass" job, which every delete_all test exercises. docs/dev/go-testing.md now records that no fixture stores a refused value, and why none may.

The write-error rationale moved to sidecars

The narrowing left three doc comments describing the old behaviour, and npm_remote_repositories_errors.go's "No SQLSTATE this table can currently raise reaches here at all" was false once the id version CHECK fell through to that fallback. check-comment-caps.sh caps an unexported doc comment at one line and checks any block a diff touches, so rewording in place would have collapsed all three blocks (23, 18 and 11 lines). The prose moved to npm_remote_repositories_errors.md, container_remote_repositories_errors.md and maven_remote_repositories_errors.md, corrected there, with a one-line doc comment pointing at each. maven_remote_credentials.go's helper already fit its cap and is fixed in place.

The npm_tags cursor comment is left alone

The tags arm's comment gives the empty-name sentinel as the reason it is conditional, where the reason is that an empty AfterName means "first page", which is what the id arm now states. Nothing in this MR falsified it: the id arm was rewritten and this one was left, which is what makes the pair read inconsistently. Editing one line of that pre-existing block would pull the whole block into the comment-caps ratchet for no correctness gain, so the rewrite belongs with whoever next touches that arm.

Stack

# MR What State
1 !2113 (merged) fixture seeds mint UUIDv7 (107 sites) merged
2 this one the CHECK holding id to UUIDv7 on 38 tables open, targets main

Merge order against !2159 (merged), settled

!2159 (merged) merged on 2026-09-01, ahead of this MR, which is the order this section asked for. The branch is rebased onto the resulting main and both points are closed:

  • The predicted conflict in migrations_checksum_test.go did occur, on knownHeadVersion alone, and was resolved as prescribed: the later 20260831103000 stays, because this migration is still the lexical head. structure.sql was regenerated with mise run db:dump-structure rather than taken from the textual auto-merge. The regenerated dump is byte-identical to the auto-merged one, so the auto-merge happened to be correct here; the regenerated file is what is committed.
  • The out-of-order deploy risk is gone. 20260831055729 now precedes this migration on main, so goose.WithAllowOutofOrder(false) has nothing to reject.

The rebase also falsified three prose claims, fixed in docs(datastore): account for the 39th uuid-id table after rebase:

  • The squawk header counted four migrations setting a lock_timeout. !2159 (merged)'s Down bounds its DROP TABLE run, so it is five.
  • The Up header called the exclusions "the ones with no uuid id to bound, which is the same rule that selects the 38". container_virtual_repositories has one and is still excluded.
  • id_uuid_version.md put the tooling tables "outside the catalog query that selects the 38". That query now selects 39, so the sentence names the query instead of the count.

This migration's own table set is unchanged, so the 2406-lock figure and the timing table above still describe it. Re-measured on PostgreSQL 17 after the rebase: replaying the Up in one transaction still takes 2406 AccessExclusiveLocks.

!2113 (merged) has merged, so this MR targets main and its diff shows the constraint alone. The branch has been rebased onto main and the two commits !2113 (merged) carried are gone from it.

The rebase picked up three things main moved under the branch, each of which is in this MR's diff:

  • npm_virtual_upstream_rules landed on main after the branch was cut, with a uuid id and no default. The coverage test derives its set from the catalog, so the table has to carry the constraint too: that is what moves the count from 37 to 38 and the lock budget from 2341 to 2406. Its own suite anticipated this, counting three CHECKs and naming work item 983 in the failure message as the change that makes it four. It is four now.
  • TestBulkContainerWorkerIntegration_DeleteAllImages_SkipsAStoredZeroID seeded uuid.Nil on purpose, which the CHECK now refuses. It is retired; see "The stored zero-id fixture" below for the decision and what still covers the behaviour.
  • Four prose sites hedged the constraint as unlanded ("once !2106 (merged) lands"). They now read in the present tense: scripts/conformance/provision.sh, scripts/conformance/maven-provision/main.go, docs/dev/go-testing.md and .claude/skills/db-review-prep/references/query-mode.md.

Related to #983 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading