feat(npm): npm_virtual_repository_upstreams schema 1/3 (S31 plan: 2/19)

Stacked MRs

Part MR Scope Reviewable LOC Target
1/3 this MR schema, generated companions, and the delete-path claims 1,045 main
2/3 !2067 (merged) the shape and constraint suite 1,044 dmeshcharakou/s31-npm-virtual-step-2a
3/3 !2068 (merged) the absences and the rows they admit 962 dmeshcharakou/s31-npm-virtual-step-2b

Merge 1/3, then 2/3, then 3/3. The order is forced, not a preference. Part 2/3 asserts a table part 1/3 creates, and it also defines the fifteen fixtures part 3/3 consumes, among them seedNPMVirtualUpstreamTree, npmVirtualUpstreamsInsert and assertDeferredUniqueViolation, so part 3/3 does not compile without it. Nothing flows the other way.

Total 3,051 reviewable LOC, split as evenly as the atomic units allow (1,045 / 1,044 / 962, within about 6% of equal thirds). The migration is one file and cannot land in halves, and neither test suite can be cut without separating the fixtures from the tests that use them, so no part clears the 500-line ceiling and each states its own justification below.


Joins one npm_virtual_repositories row to the ordered list of upstreams it resolves through: four uuid columns and one int position, hash-partitioned on namespace_id into 64 partitions, with a composite foreign key per reference so every row stays in its parent's partition and cannot pair across namespaces.

Three foreign keys, and the third is the one to read carefully. fk_nvru_upstream_repository_id_repositories is the first NO ACTION key in this schema whose referenced table is repositories itself, so a 23503 from a DELETE FROM repositories no longer implies the repository has contents. It can now also mean the repository is still listed as an npm virtual upstream, which emptying it does not clear.

That widening falsifies thirteen sites. Nine are corrected here rather than at a stack position later, because they go wrong the moment this migration lands. Four are comments whose text was the stated justification for mapRepositoryDeleteError and mapRepositoryFinalizeError collapsing every 23503 to one sentinel. The fifth is the FK schema guard's wantBlocking list, which fails on arrival without its new entry and so cannot be separated from the migration at all. Two are operator-facing, the purge_outcomes_total Help string and docs/dev/observability.md, and the last two are TestMapRepositoryDeleteError's doc and its assertion message. The remaining four are listed under Review notes, with the comment-cap reason they are deferred.

The delete-path prose moves to a new internal/datastore/repositories.md, because correcting it in place would have to fit the single line an unexported doc comment gets. Both mappers now carry a one-line doc that points there, which is the split lifecycle_reap_repository.md already uses. That sidecar also records the three admitted row shapes that wedge the purger and which path has to refuse each.

The one reachable through the service is an ordinary cross-listed upstream, and the delete route creates it: a live hosted or remote repository that some other virtual repository lists, deleted with destructive=true, is tombstoned by the destructive arm and then refused by the upstream key on every purge. No association-side guard closes it, because the association was already legal when the tombstone was written, so the delete route has to check the listing before it tombstones, under either destructive value. The second shape is an already soft-deleted upstream, which the association route refuses.

The third is an upstream that is its own virtual parent's repositories row, and it is measured rather than reasoned about. On PostgreSQL 17.11 and 18.6 the upstream key refuses that delete with 23503 even though the same statement's cascade through npm_virtual_repositories would have removed the association, because the clearing cascade runs one trigger level deeper than the check and PostgreSQL fires a deeper level only after every after-trigger of the current one. It is not that the check sees only the statement's starting snapshot: such a check does see rows an earlier statement of the same transaction deleted, so clearing the associations first unwedges the delete. That shape is refused at the SQL level only and never reaches either mapper, because both Go delete paths gate the kind first and repositoryDeleteKindGate refuses RepositoryKindVirtual. The shape suite in part 2/3 pins the SQL-level outcome.

Lock sets and the three squawk-remedy refusals are measured on PostgreSQL 16.15 and again on 17.11, the version GL_PG_CURR_VERSION pins: ShareRowExclusive on 131 foreign-key-target relations on the Up, AccessExclusive on the same 131 when the Down drops the parent, nothing on any target for a partition drop, and all three remedies refused with the messages quoted in the file.

The Down carries the PGOPTIONS lock_timeout recipe the sibling migration gives, because dropping the parent takes ACCESS EXCLUSIVE across the whole live repositories tree and so blocks reads, and the rollback entrypoint leaves no session to issue a SET in.

S20-A's three passages resting on the old topology are amended, since this change is what falsifies them.

Reviewable LOC 1,045, of which the migration is 781 and roughly 260 of those are mechanical partition DDL and blanks. Generated companions are 2060 more: structure.sql and three go-jet files. The 43 lines of production Go change no logic, only doc comments and one inline comment. Splitting further would have to cut the migration file, which cannot land in halves. Table-specific shape and constraint tests are parts 2/3 and 3/3 of this stack; what covers this part is the FK schema guard, the migrations checksum and head-version tests, and the shared TestMigrations_UpDownUp with assertNoApplicationSchemaResidue, which apply and roll back the whole chain.

Review notes

  • e2e catalog: no scenario is added or affected. Step 19 of the plan owns the catalog for this workstream.
  • Integration-tagged lint: golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/datastore/... was run and introduces no new finding. Measured by diffing the run against the pre-change tree, because the package carries a large pre-existing backlog that the uncapped flags surface.
  • ADR-007 amendment: still outstanding. This table declares four divergences the ADR does not cover: the ON DELETE correction, the missing reverse-lookup index, the CHECK constraints recorded only as prose value legends, and DEFERRABLE INITIALLY DEFERRED worded as a property of an index. The plan records that the handbook amendment MR gates no step, and its URL belongs here once it exists. No issue tracks the amendment; the three tables themselves are tracked by #882 (closed), #883 (closed) and #884 (closed).
  • Shape tests land in part 2/3. This part carries the FK schema guard, the migrations checksum and head-version tests, and the shared TestMigrations_UpDownUp with assertNoApplicationSchemaResidue, which apply and roll back the whole chain. It carries no column, index or constraint assertion for the new table; those are part 2/3. That is a deliberate consequence of splitting for size and it differs from step 1, which shipped its schema suite alongside its migration.
  • No DDL changed after the schema was dumped, so structure.sql needs no regeneration; the migrations checksum and head-version tests cover the pair. The follow-up commit's migration diff is comment-only for the same reason.
  • This reverses an explicit S31 ordering, deliberately. S31's Dependencies require "the implementation of the S17 repository-delete guard ... lands before this spec's schema MR creates npm_virtual_repository_upstreams", because "the widening and the guard cannot be separated by a release". This MR creates the table with no guard. The merged plan takes that reversal and argues the safety: the spec's own rationale is scoped to "from the moment the first association row exists", and no association row can exist while repositoryCreateKindGate refuses RepositoryKindVirtual. The npm arm of the guard rides with the association route in S17 Phase 6 Step 41, and #314 carries the narrower ask that Step 41 not be split so the association route cannot merge ahead of the guard.
  • Two step-2 plan deviations. The constraint names take nvru rather than the nvr the plan named, because fk_nvr_namespace_id_namespaces is already spent by npm_virtual_repositories and identifier names are schema-global. The identifier-length arithmetic lives in the migration comment rather than the schema test, because that test is part 2/3. disallowed-unique-constraint is a deliberate first use in this repo, which the migration header records.
  • Four Go doc blocks keep the falsified reading, and are left for a follow-up: internal/datastore/sqlstate.go, internal/lifecycle/metrics.go at the purge_outcome label and at purgeOutcomeLabelValue, and the doc on TestPurgeWorker_Work_APinnedParentFailsTheJobRatherThanReportingNotEmpty. Each is a single 17-to-40-line block that check-comment-caps.sh scores against a cap of 2 or 3 the moment it is touched, so correcting one sentence means extracting the whole block to a new sidecar in a package this MR otherwise does not touch. None of the four is operator-facing; the two that are, the purge_outcomes_total Help string and docs/dev/observability.md, are corrected here.

Related to #883 (closed)

Ordering confirmation for the S17 delete guard is tracked in #314

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading