chore(datastore): swap the image name unique index to partial

Stacked on chore(datastore): gate container image reads on... (!1466 - merged) • Hayley Swimelar • 19.3, merges after it.

Why

unique_container_images_ns_id_cr_id_name is total, so a soft-deleted image keeps its name occupied. A push that reuses that name conflicts with the tombstone and inserts nothing, which is the last thing standing between the marker column and an image delete that works.

Partial on soft_deleted_at IS NULL is the shape ADR-007 specifies for this index, and the shape the npm and Maven unique indexes already carry. The marked row leaves the index, the name frees up, and the re-push lands a fresh image row whose manifests, tags, and blobs hang off the new id, clear of the pending reap.

Nothing writes the marker yet, so merging changes no behavior. Every statement predicate this rests on landed in the parent MR, which leaves this one the migration, the regenerated structure.sql, and the tests and comment corrections that go with them.

Deploy order: not in the same release as !1466 (merged)

The migration applies at the boot of the first pod carrying it. A pod still serving ON CONFLICT (namespace_id, container_repository_id, name) without the marker predicate matches no arbiter against a partial index and fails every image push with SQLSTATE 42P10, "there is no unique or exclusion constraint matching the ON CONFLICT specification".

!1466 (merged)'s predicate therefore has to be fully rolled out, not merely merged. Ship both in one release and the swap applies while the rolling deploy still has predicate-less pods serving traffic, which moves the same fleet-wide push failure into the rollout window instead of preventing it.

Measured both directions on PostgreSQL 16.14: a predicate-carrying conflict target is accepted against the total index, because PostgreSQL infers a non-partial unique index for one, and a predicate-less target against the partial index raises 42P10. TestContainerImagesPartialUnique_PredicateLessArbiterIsRefused pins the code rather than the prose.

Rollback is the same constraint reversed, and it is manual. Run mise run db:rollback before any binary rollback past that predicate, because migrations.NewRunner wires Up alone and nothing runs the Down for you.

Worth a reviewer's attention

  • The migration is transactional, unlike its 20260812150200 sibling. All three statements lock the same parent and the same 64 partitions, so a transaction extends no lock the migration would not already take, and it buys an atomic swap: no committed state carries two unique indexes on the image triple, or the right index under a temporary name. It is available only because the builds are blocking, and a CONCURRENTLY build cannot run inside a transaction.
  • No ANALYZE, deliberately. ADD COLUMN left soft_deleted_at without a pg_statistic row, but ANALYZE on an empty partitioned parent creates none either (measured: reltuples 0, parent relpages -1, zero rows for any column), and empty tables are the state this runs in. Every reader of the index carries a literal namespace_id and prunes to one partition, where autoanalyze maintains the leaf statistics.
  • 64 child index names change. A create-then-rename swap builds the new children while the old ones still hold their generated names, so each new child takes a uniquifier that the 63-character identifier limit makes room for by dropping a character of the column part. That is most of the structure.sql diff: 65 index definitions gain the predicate, 64 children are renamed, and their ATTACH lines follow. Nothing unrelated drifted, verified by applying only this Up to the parent's dump and diffing.
  • The comment blast radius is wider than the index. Any doc claiming the image name triple is unique table-wide is now false, which reached internal/format/oci/store.go and internal/managementapi/container_list.go as well as the store. The cursor docs are the subtle ones, because keyset paging over image names no longer has a table-wide unique key to lean on.
  • An image re-resolve stops being a guaranteed no-op, which is the consequence with the longest reach. ContainerManifestPersister and the upload-finalize path both re-resolve an image by name and assumed they would get the same row back. Once a marker can land between two resolves, the second one creates a fresh row instead, so a manifest could persist under the new image while its blobs sit under the tombstone. Unreachable today, because nothing stamps the marker, so this MR corrects the comments at both sites and leaves the behavioral close to the step that adds the first marker writer, where the trigger first exists. That step wants either the resolved image id threaded through PersistManifestInput or a fail-closed check on created=true.
  • Size. 1,124 added lines outside the generated structure.sql, against the 500-line guardrail. 982 are tests, 56 are the migration, and 86 are comment-only edits in production files. No production statement changes, and splitting the tests off would ship a red pipeline on the first MR.
  • The duplicate-triple guard covers 2 of the 6 suites whose down-walk crosses this migration. The other four register namespace-wide cleanups, so they cannot leave the live-plus-marked pair behind that would make the Down's rebuild fail. The two guarded walks are this migration's blast radius.

Spec coverage

Scoped to the criterion this step's plan Acceptance line names, plus the adjacent marker criteria other steps own.

# Criterion Tests
AC-33 A reap interrupted partway leaves the target marked and absent from reads, and the next pass completes the subtree children-first Purger behavior, owned by S20-A. Not tested here.
AC-34 Every read under a marked image answers 404 from the mark onward Owned by the parent MR and unchanged here: TestContainerImageStore_SoftDeletedImageIsInvisibleToReads, TestContainerTagStore_ManagementReads_ExcludeSoftDeletedImage. This MR re-runs them against the partial index rather than adding coverage.
AC-35 A push that reuses a marked image's name before the reap runs succeeds as a fresh image row and is readable, rather than failing on the unique index, resurrecting the marked row, or returning a row no read can see TestContainerImagesPartialUnique_MarkedNameAcceptsAFreshPush/a_marked_name_takes_a_fresh_row (succeeds, distinct id, tombstone untouched) and TestContainerImageStore_Upsert_WithASoftDeletedSibling/the_marked_name_is_re-created_as_a_fresh_image (the store path, plus FindByID for the readable half)
The spec's index paragraph, sentence by sentence, plus the database-level rejections and the security concerns
Claim Tests
unique_container_images_ns_id_cr_id_name becomes partial on soft_deleted_at IS NULL TestContainerImagesPartialUnique_IndexShapeAtHead (parent relation, exact key list, exact predicate, and both UNIQUE and valid)
A marked image no longer blocks the re-push of its name TestContainerImagesPartialUnique_MarkedNameAcceptsAFreshPush/a_marked_name_takes_a_fresh_row
A soft-deleted prior row does not block a re-publish, matching the npm and Maven unique indexes Same subtest. The npm and Maven halves belong to their own suites and are unchanged.
The other container unique indexes stay total Not asserted. This migration touches one index, and a suite pinning the others would pass today whatever this migration did.
The image upsert's conflict target moves to the partial index and its fallback SELECT takes the filter Landed in the parent MR. TestUpsertContainerImageInsertStmt_ArbiterPredicate pins the rendered arbiter, and this MR only rewrites its stale doc comment.
Every partition carries the rebuilt index TestContainerImagesPartialUnique_EveryPartitionCarriesAValidChild (one valid attached child per catalog-read partition)
The Down restores the total index TestContainerImagesPartialUnique_TotalIndexRestoredAfterDown (predicate empty, keys unchanged, still UNIQUE, one child per partition, and it accepts a predicate-carrying arbiter again)

The spec's Error Cases table lists HTTP responses, so its rows belong to the handler steps that produce them. These are the database-level rejections this migration decides.

Condition Tests
A predicate-less conflict target matches no arbiter: SQLSTATE 42P10 TestContainerImagesPartialUnique_PredicateLessArbiterIsRefused
A second live row for one name is refused: SQLSTATE 23505 from this index TestContainerImagesPartialUnique_MarkedNameAcceptsAFreshPush/a_duplicate_live_name_is_refused_outright, which asserts the constraint name is a child of this index
A second upsert of a live name conflicts and inserts nothing TestContainerImagesPartialUnique_MarkedNameAcceptsAFreshPush/a_live_name_still_cannot_gain_a_second_row, TestContainerImageStore_Upsert_WithASoftDeletedSibling/a_live_name_stays_idempotent_beside_a_marked_row
The Down cannot rebuild the total index over a live and a marked row sharing a name TestContainerImagesPartialUnique_TotalIndexRestoredAfterDown checks for such a pair first, so a leak fails legibly instead of inside goose
Concern Tests
Tenant isolation: the index still leads with namespace_id, so every read prunes to one partition TestContainerImagesPartialUnique_IndexShapeAtHead (exact key list) and .../the_marker-filtered_lookup_rides_the_partial_index, plus the existing TestFindContainerImageInRepositoryStmt_SinglePartitionPrune and TestContainerImageStore_Upsert_SinglePartitionPrune
No live image name goes unguarded: the swap narrows the uniqueness guard rather than dropping it The two negative subtests above. Either would pass against a schema carrying no unique index at all if the positive shape assertion did not also require UNIQUE.
A name carrying a tombstone and a live row lists exactly once The ListContainerImages assertion inside TestContainerImageStore_Upsert_WithASoftDeletedSibling/the_marked_name_is_re-created_as_a_fresh_image. Verified by mutation: dropping the marker filter from listContainerImagesStmt fails it.
No availability window opened by the swap TestContainerImagesPartialUnique_MigrationStaysTransactional asserts neither section leaves the transaction.

Test plan

export ARTIFACT_REGISTRY_DATABASE_TEST_DSN=<a PostgreSQL 16 DSN>

# the swap's own suite: shape, behavior, the 42P10 gate, and the Down
go test -tags=integration -count=1 -run TestContainerImagesPartialUnique ./internal/datastore/migrations/

# the parent's down-walk, which now reverts this migration first
go test -tags=integration -count=1 -run TestArtifactTombstoneSchema ./internal/datastore/migrations/

# the store path, including the tightened marked-sibling upsert
go test -tags=integration -count=1 -run 'TestContainerImage' ./internal/datastore/

# integration-tagged lint, which CI lint cannot see
golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 \
  ./internal/datastore/ ./internal/datastore/migrations/

CI covers the rest: db:migrate applies the chain on every supported PostgreSQL version, and db:structure-check catches a structure.sql that disagrees with the migration.

No e2e scenario is added or affected. Nothing writes the marker yet, so no user-reachable behavior changes on merge, and the scenario that will exercise this (delete an image, then re-push its name) arrives with the delete endpoint.

Context for LLM agents

Design rationale, with the alternatives rejected

Create-then-rename over drop-then-create. Drop-then-create yields cleaner child index names (no uniquifier, so no structure.sql churn) but leaves a window with no unique guard on live image names. For a uniqueness constraint that trade is the wrong way round, so the name churn was accepted instead. The 20260626131100 keyset-index swap took drop-then-create, and the difference is that its index was not unique, so its gap cost nothing.

Transactional over NO TRANSACTION. Considered and rejected: copying the sibling 20260812150200's directive. That file's reason is that a transaction would have held container_images' partition locks through an unrelated maven_packages build. This migration's three statements all target the same parent and the same 64 partitions, so the reason does not transfer, and the transaction buys atomicity. 20260717141847, the other total-to-partial precedent, is NO TRANSACTION because it builds CONCURRENTLY, which cannot run in a transaction. Its directive is a consequence, not a convention.

No ANALYZE. Considered on the grounds that ADD COLUMN left soft_deleted_at without statistics. Rejected on measurement: ANALYZE on an empty partitioned parent creates no pg_statistic row at all, and the tables are empty pre-launch. A further measurement narrowed the premise itself, which is worth recording because it reads the other way at first glance: the image list page takes the same index scan and the same three shared-buffer execution with and without statistics on the column, because the partial index absorbs the predicate and no selectivity estimate is consulted. The swap is what closes that gap, rather than something an ANALYZE here would close.

Blocking builds over the per-partition concurrent path. The per-partition path (ON ONLY the parent, CONCURRENTLY per partition, then ATTACH PARTITION) is available on a partitioned parent and avoids the long lock. It also gives up the transaction, and the tables are empty pre-launch, so blocking is chosen. The per-partition recipe above is the alternative if this ever reruns against populated tables.

Non-goals

  • Marker predicates on container_images statements. All of them landed in the parent MR. This MR adds no production Go logic, and every non-test .go change in it is a comment.
  • Closing the re-resolve window behaviorally. The mark-window race described above (a manifest persisting under a fresh image row while its blobs sit under the tombstone) needs a marker writer to exist before it can fire, and no writer is in the tree. Adding the guard here would put production logic into a migration-and-comments MR and could not be tested end to end. It belongs with the first marker-writing step, whose Depends-on line should carry it.
  • Two stale present-tense claims in sibling specs. S17-rest-management-api.md and S20-a-lifecycle-closed-beta.md both still say this index "is total today", and S20-A additionally states the rebuild takes ACCESS EXCLUSIVE. Measured here, the CREATE takes ShareLock and only the DROP takes AccessExclusive. Cross-spec edits are out of scope for this MR, so both go to the spec authors instead.
  • A writer for the marker. Nothing stamps soft_deleted_at yet. The first writer is a later step of the same plan, and it depends on both this MR and the parent.
  • Reaping the tombstones. The purger belongs to S20-A. A marked row and its subtree persist, hidden, until it runs.
  • Markers on container_manifests and container_blobs. ADR-007 specifies both. Neither arrives here, because every read of one resolves its image first, so the image's marker hides the subtree.
  • A metric for the 42P10 failure mode. ContainerImageStore emits no query metrics yet (tracked as #54), so detection is by the log line's message text. Adding a bespoke counter ahead of that instrumentation would not match how the rest of the store is measured.
  • index_container_images_on_ns_id_cr_id_last_downloaded_at. Still total where ADR-007 would want it partial. Pre-existing, non-unique, and not what this step's plan scopes, so it is left for a follow-up.
  • The plan's Status table. Left to the conductor of this step series, because every branch in the stack would otherwise edit the same rows.

Database Review Evidence

Migrations

Note

Timings are from CI (db:migrate matrix, goose verbose) 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.

Timings come from pipeline 2757931390, the green run at pre-rebase head 0b7237d6. The migration's executable statements are identical at the current head (later pushes cut the file's comments and re-stamped its version only), so the numbers carry over.

Migration PG 16 PG 17 PG 18
20260814131541_make_container_images_name_unique_index_partial.sql OK (123.16ms / 138.82ms) OK (39.96ms / 87.45ms) OK (31.36ms / 35.31ms)

Migration notes:

  • PG 16 apply (123.16ms) is 3.1x PG 17 (39.96ms), past the 2x version-spread line. Consistent with the measured harness baseline (PG 16 runs the full chain 34-37% slower than 17/18), and PG 18's spread across the two green runs (31-88ms) shows runner noise dominates at these magnitudes. Absolute cost is 123ms against an empty database and a 5-minute boot budget.
  • PG 17 rollback (87.45ms) is 2.2x its apply, consistent across both green runs (2.26x, 2.19x). PG 16 and PG 18 are symmetric (1.13x).

Related to #313 (closed)

Edited by Hayley Swimelar

Merge request reports

Loading
Loading