refactor(datastore): tie a relationship edge to its manifests' image

Hayley Swimelarrequested to merge
1339/relationships-composite-image-fk into main

Why

The table's four foreign keys reach namespaces once, container_images once, and container_manifests twice, none of them tied to the container_image_id the row carries. A row naming one image while its child manifest sits under another satisfies all four. No production writer creates that row and the reads do not re-check, so a future writer, a backfill, or a repair script could put one in front of the API. Three integration tests recorded the gap rather than asserting it away. Raised in review on !2581.

What

This replaces the two narrow manifest keys, it does not add to them. All four columns are NOT NULL, so MATCH SIMPLE never escapes and the narrow pair is fully implied. Keeping both doubles the insert's referential-integrity work: 20,000 inserts measured 1185 to 1363 ms with the narrow pair, 2756 to 2899 ms with both, and 1389 ms with the composite pair.

NOT VALID is refused on a partitioned table on PostgreSQL 16.15 and 17.10 with SQLSTATE 42809, and accepted only on 18.4. All three sit in the db:migrate matrix, so the keys add validating in one statement, deviating from docs/dev/database-migrations.md's two-migration recipe. The scan is linear in the edge count, 14 to 24 microseconds per edge over a 470 ms lock floor, so the pair holds ShareRowExclusive on both tables and all 128 partitions for 14.3 s at 254,256 edges.

The referenced index is invalid until the 64th ATTACH, and an invalid index silently backs no foreign key, so both constraint statements come after the attach phase. The schema test asserts indisvalid.

The rename moves mapCreateCMRError's needles to parent_cm_id and child_cm_id. No name keeping the full child_container_manifest_id token and also naming the image column fits 63 bytes, measured at 64 and 65. Create has no production caller, so the deploy window where a new binary meets the old schema is reachable only from tests.

The equal-digest tiebreak becomes unreachable. Two manifests can share a digest only across images, so once both keys hold every child of a parent has a distinct digest. The ORDER BY trailing key stays as a deterministic cursor, and the flipped test says so.

container_remote_manifest_relationships holds the same gap on purpose and nothing here touches it.

Size

14 files, 2,639 raw lines, and 1,991 reviewable, past the 500 that docs/dev/development-model.md asks a split or a justification for. Reviewable by group: the new schema integration test 1,130, migration SQL 252 of its 636 with 384 mechanical 64-times lines, three flipped test files 373, Go production 185, three dev docs 47, the run recipe and knownHeadVersion 4, and regenerated structure.sql 0 of its 264. The bulk is that one test file, and the only split available separates it from the migration it covers, which guardrail 6 forbids.

Merge order

migrations_checksum_test.go moves knownHeadVersion to this migration's version, and the same line moves to a different version on fix(datastore): serve maven package-id FK check... (!2762 - merged) • Moaz Khalifa • 19.5. Whichever lands second re-derives the constant from the new lexical head, and no pipeline on either MR reports the collision.

No e2e scenario is added or affected. The catalogued scenarios reaching this table are e2e.docker.publish.push-multiarch, e2e.docker.publish.buildx-default-push, e2e.docker.discover.manifest-detail-via-api, and its e2e.oci.discover.manifest-detail-via-api twin, and all exercise the push path and the read, both unchanged.

The S12 spec and ADR-007 text amendments ship separately, as a docs(specs) MR and a handbook MR.

The attach-phase abort window this migration layout carries predates the branch and is tracked in Three container migrations cannot replay after ... (#1403 - closed) • Hayley Swimelar.

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.

Source: pipeline 2859818380, green on merge commit 8455887d4. The branch was rebased onto main after that run, and every SQL statement in this migration is unchanged across both heads, with only the two validating-add pragma comments edited since.

Migration PG 16 PG 17 PG 18
20260921135818_add_container_manifest_relationships_image_composite_fks.sql OK (893.32ms / 549.64ms) OK (759.68ms / 535.26ms) OK (942.57ms / 538.56ms)

Migration notes:

  • No anomaly fires: every phase reports OK, the spread across versions is 1.06x on apply and 1.02x on rollback, and apply stays under 1 s on all three. The idempotency re-apply reaches 1.02 s on PG 18.
  • Apply runs 1.4x to 1.8x rollback, the benign direction. The Up builds 64 partition indexes and adds two validating keys, and the Down drops the parent index in one cascade.
  • CI lands below the 0.96 s to 1.76 s the file's header records for an empty 64-partition schema, at 0.76 s to 0.94 s here. Different hardware, and below rather than above the documented cost.
  • CI says nothing about the header's 14 to 24 microseconds per edge, because every matrix instance applies against an empty schema and both validating adds scan 0 edges. The boot budget is set by the header's populated measurement (15.45 s at 513,710 manifests and 254,256 edges, 14.3 s of it the two adds), not by this table.

Queries

No query evidence to collect. All three changed non-test Go files dispatch SQL, but every statement chain is byte-identical to the merge-base. container_manifest_relationship_children.go and lifecycle_reap_container.go changed comments only, and the one code change, in mapCreateCMRError, inspects a driver error and dispatches nothing.

Context for LLM agents

Rationale

  • Leaf-to-leaf foreign keys, partition ordinal to partition ordinal. Both tables hash namespace_id on the same modulus, so a per-leaf key naming the matching leaf costs 19 locks against 1103, measured. Rejected because the constraint would not appear on the parent table at all, so every catalog read filtering conrelid = 'container_manifest_relationships'::regclass AND conparentid = 0 shows it missing, which is the query the existing remote constraints suite uses. It also hard-codes that both tables keep the same modulus forever, and the lock saving buys nothing because 1103 of 6400 slots already fits.
  • A CHECK constraint or a row-level trigger. PostgreSQL cannot express a CHECK over another table's rows, so this reduces to two lookups per inserted row in PL/pgSQL. It escapes pg_constraint, so no schema test can assert its form the way this repository's suites assert foreign keys. docs/dev/database.md settles the general question.
  • Keep both pairs. Roughly 70 microseconds per row for a guarantee the composite pair already makes, so a 200-child Image Index push pays about 14 ms for nothing.
  • Two migrations, the index then the constraints. The index exists only to back the constraints, so the first migration would leave an index nothing references and that adds no uniqueness the primary key does not already give, and the second is unapplyable until the first lands.
  • Keep the existing constraint names and widen their column lists. The names are 54 and 55 bytes, so both fit, and mapCreateCMRError would need no change. Rejected because fk_cmr_child_container_manifest_id_container_manifests would then name one of three columns, against the fk_<table>_<columns>_<referenced table> convention in docs/dev/database.md. A same-named constraint with a different definition is also the kind of change a structure.sql diff shows quietly, and the same reasoning drove the rename in feat(maven): enforce maven_files version↔packag... (!566 - merged) • Moaz Khalifa • 19.2.
  • Purpose-name the constraints. fk_cmr_child_belongs_to_image fits easily and states the invariant. Rejected because docs/dev/database.md's pattern is the column list, the maven pair is the only deviation in the tree, and a column-list name reads as the successor of the constraint it replaces.

Non-goals

Spec coverage

Spec: docs/specs/S12-container-oci-hosted.md, #### container_manifest_relationships. Criteria numbered as in the working plan's Acceptance criteria section.

# Criterion Tests
AC-1 An edge whose child manifest belongs to a second image is refused with SQLSTATE 23503 naming fk_cmr_ns_id_and_ci_id_and_child_cm_id_container_manifests TestContainerManifestRelationshipStore_ForeignImageChildEdgeRefused, TestContainerReaper_CrossImageRelationshipRefused, TestCMRImageCompositeFK_RefusesAnEdgeWhoseManifestSitsElsewhere/the_child_manifest_sits_under_a_second_image
AC-2 An edge whose parent manifest belongs to a second image is refused with SQLSTATE 23503 naming fk_cmr_ns_id_and_ci_id_and_parent_cm_id_container_manifests TestContainerManifestRelationshipStore_EqualDigestsAcrossImagesRefused, TestCMRImageCompositeFK_RefusesAnEdgeWhoseManifestSitsElsewhere/the_parent_manifest_sits_under_a_second_image
AC-3 unique_container_manifests_ns_id_ci_id_id exists at the parent, keys (namespace_id, container_image_id, id) in order, is unique and total, and is indisvalid TestCMRImageCompositeFK_ReferencedIndexKeysThreeColumnsInOrder
AC-4 All 64 partition indexes exist under their written-out names and all 64 are attached, and the partitions sweep reports no unattached child TestCMRImageCompositeFK_EveryPartitionCarriesItByName, TestCMRImageCompositeFK_OrphanSweepSeesAnUnattachedChild
AC-5 Every identifier the migration introduces is at most 63 bytes TestCMRImageCompositeFK_NamesFitIdentifierLimit
AC-6 Both new constraints exist with conparentid = 0, confdeltype = 'a', confupdtype = 'a', and the exact pg_get_constraintdef text TestCMRImageCompositeFK_BothKeysCarryTheirDefinitions (one subtest per key)
AC-7 Both narrow manifest keys are absent from the table, counted without the conparentid filter TestCMRImageCompositeFK_NarrowManifestKeysAreGone (one subtest per key)
AC-8 Per-referenced-parent multiplicity over conparentid = 0 is namespaces: 1, container_images: 1, container_manifests: 2 TestCMRImageCompositeFK_EveryKeyPointsAtItsDeclaredParent (one subtest per parent)
AC-9 fk_cmr_container_image_id_container_images and fk_container_manifest_relationships_namespace_id_namespaces survive unchanged TestCMRImageCompositeFK_ImageAndNamespaceKeysSurvive (one subtest per key)
AC-10 The Down, by static parse: NO TRANSACTION, no DO $$, both composite drops before DROP INDEX IF EXISTS, each re-add guarded by a preceding DROP CONSTRAINT IF EXISTS, SET/RESET lock_timeout bracketing its statements TestCMRImageCompositeFK_DownRestoresTheNarrowKeysBounded
AC-11 The migration's version prefix equals the 14 digits of its filename TestCMRImageCompositeFK_VersionMatchesItsFilename
AC-12 The foreign-image child test asserts the refusal naming the child-side key, and its read-assertion body is gone TestContainerManifestRelationshipStore_ForeignImageChildEdgeRefused, renamed from TestContainerManifestRelationshipStore_ListChildrenByParentIDs_ForeignImageChildServed
AC-13 The equal-digest test asserts the refusal naming the parent-side key, and its doc comment states the schema refuses the only form that reached the ordering's trailing key, which stays as a deterministic cursor TestContainerManifestRelationshipStore_EqualDigestsAcrossImagesRefused, renamed from TestContainerManifestRelationshipStore_ListChildrenByParentIDs_EqualDigestsBreakOnTheChildID
AC-14 The reaper test and its raw-insert helper assert the refusal, and the helper's comment no longer claims nothing holds the three ids together TestContainerReaper_CrossImageRelationshipRefused, renamed from TestContainerReaper_RootManifestPage_ChildOfAnotherImage, through assertCrossImageEdgeRefused, which replaced seedCrossImageRelationship and the branch's own assertRawChildEdgeRefused. TestContainerReaper_RootManifestPage_ExcludesAReferencedChild is new and keeps the direct assertion on the page's own output, which the cross-image premise no longer carries
AC-15 The Up, by static parse: one ON ONLY parent build, 64 CREATE UNIQUE INDEX CONCURRENTLY leaf builds each preceded by its own guard, every build before the first ATTACH PARTITION, and both ADD CONSTRAINT after the last TestCMRImageCompositeFK_UpLayoutIsRecoverable
AC-16 mapCreateCMRError still maps a parent-side violation to ErrParentManifestMissing and a child-side one to ErrChildManifestMissing, and the two existing subtests pass without edits TestContainerManifestRelationshipStore_Create/missing_parent_surfaces_ErrParentManifestMissing, TestContainerManifestRelationshipStore_Create/missing_child_surfaces_ErrChildManifestMissing, both unedited
n/a Not an acceptance criterion. rootManifestPage returns the image's manifests that no relationship row names as a child, which this change does not alter. Restored coverage: the flip above removed the suite's only direct call to the method TestContainerReaper_RootManifestPage_ExcludesAReferencedChild, new, and green at this phase by design
n/a Not an acceptance criterion. mapCreateCMRError matches its two needles as substrings, so it needs exactly one key per needle and no key carrying both. The comment above the switch claimed that and nothing asserted it TestCMRImageCompositeFK_OneKeyPerErrorNeedle (one subtest per needle), new

Related to #1339 (closed)

Edited by Hayley Swimelar

Merge request reports