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
VALIDATEscans 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 CONSTRAINTstatements against the Up section's 76ADDplusVALIDATEones, 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 seededidwith SQLgen_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 targetsmaindirectly. No production code was affected: the onlygen_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_namespacewas grouped with thebigint-surrogate tables. It has noidcolumn at all.- The
require-timeout-settingssuppression said three migrations set alock_timeoutand all three bound anATTACH PARTITIONphase. There are four, and the fourth bounds aDROP TABLErun. - 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 CONSTRAINTtakes anobjectlock on eachpg_constraintrow 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
PGOPTIONSadvice cited an inverted precedent. It borrowed a "no session to SET in" reason from a migration that isNO TRANSACTIONin 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
VALIDATEscans real rows. The premise is now scoped per environment, and the recovery section names the literal-7digest derivation (which is the namespace case, notoverlay) and addsscripts/conformance/npm-e2e.shto the seed list. - The Down had one statement of coverage out of 38, and no job runs
goose down.DownReversesTheUpnow compares the Up'sADDandVALIDATEsets against the Down'sDROPset 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
pgErrorCarriesRowDataguard, on a table whose failing row echoes the plaintext pair. Restored, mirroringcontainerRemoteWriteError, with unit coverage. - Three checkable claims in
container_remote_manifest_relationships_constraints.mddid 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 thatdocs/dev/database.mdasks 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.godid occur, onknownHeadVersionalone, and was resolved as prescribed: the later20260831103000stays, because this migration is still the lexical head.structure.sqlwas regenerated withmise run db:dump-structurerather 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.
20260831055729now precedes this migration onmain, sogoose.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 itsDROP TABLErun, so it is five. - The Up header called the exclusions "the ones with no
uuid idto bound, which is the same rule that selects the 38".container_virtual_repositorieshas one and is still excluded. id_uuid_version.mdput 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_ruleslanded onmainafter the branch was cut, with auuid idand 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_SkipsAStoredZeroIDseededuuid.Nilon 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.mdand.claude/skills/db-review-prep/references/query-mode.md.
Related to #983 (closed)