feat(npm): npm_virtual_repositories schema (S31 plan: 1/19)

Stacked MRs

Part MR Scope Reviewable LOC Target
1/3 !1979 (merged) (merged) the RepositoryStore.Delete foreign-key comments, and the interrupted-down recovery rule 72 main
2/3 this MR the npm_virtual_repositories migration and the shape it declares 1078 main
3/3 !1988 (merged) the absences the table carries, and the rows they admit 686 dmeshcharakou/s31-npm-virtual-step-1

Merge order is 1, 2, 3. The two links are not equally strong, so both are stated plainly.

Part 2 before part 3 is a build dependency. Part 3's cases query the table part 2 creates, and its file uses the table name, the identifier-name constants and the seed helpers that part 2's file declares. It neither compiles nor passes without part 2.

Part 1 before part 2 is a correctness one, not a build one. Part 2 compiles and passes against main on its own. What part 1 buys is that part 2 does not leave RepositoryStore.Delete's comment saying seven inbound foreign keys to repositories when there are eight: part 1 deletes that enumeration, and part 2 adds the eighth and updates the one copy that survives, wantCascadeTargets in the schema guard. Merged the other way round, part 2 ships a comment the same change made wrong.

Nothing in GitLab enforces either link. No merge-request dependency is set. Part 1 has since merged and part 2 was retargeted to main, so the 1-to-2 link is discharged and only this table records that it existed; the 2-to-3 order is still carried by the target-branch chain.

Parts 2 and 3 are both row 1 of the S31 plan Status table, so both carry the 1/19 marker. One row shipping as several merge requests is this plan's own recorded precedent. Part 1 is not a plan step: it is review fallout from part 2's branch that turned out not to depend on the new table.

What this changes

A virtual npm repository stores nothing itself. It serves packages from an ordered list of other repositories. Before any of that can be built, there has to be a row that says "this repository is an npm virtual one", and something for the upstream list to hang off. That is all this MR adds.

npm_virtual_repositories is three uuid columns: namespace_id, id, and repository_id. It is a type marker and nothing more. Everything a repository has (its name, visibility, format, kind) is read from the parent repositories row, and everything it serves comes from the upstreams it composes, so there is no cached content here and no credential. The table is hash-partitioned on namespace_id into 64 partitions, with a unique index on (namespace_id, repository_id), matching ADR-007 exactly.

Nothing reads or writes it yet. repositoryCreateKindGate still refuses RepositoryKindVirtual outright, so the table cannot be populated by the running service and this MR cannot change how it behaves today. The association table the upstream list needs is step 2.

The migration comment carries a long catalogue of what the table deliberately does not have, and why: no url or auth token (a virtual repository dials no upstream, so a copy here would be a second unaudited home for a token), no cache-validity or health columns (it caches nothing and has nothing of its own to probe), no soft_deleted_at (ADR-007 puts soft deletion on the parent), and no timestamps (the row lives and dies with its parent). That catalogue is load-bearing, because scripts/ci/check-migration-immutability.sh freezes this file the moment it merges. The tests that hold those absences honest are part 3, which is where the reader should look for them; this MR carries the tests for the shape the migration positively declares.

Two things about the schema worth a moment

The composite foreign key is not a style choice. repositories is itself hash-partitioned on namespace_id, so a single-column reference to it cannot be declared at all. Carrying namespace_id in the reference also keeps the child in the same partition as its parent and rejects a row that pairs one namespace's child with another namespace's repository. It is ON DELETE CASCADE, which ADR-007 fixes for every format child table; NO ACTION would refuse the delete of an otherwise empty virtual repository.

One index covers both foreign keys. The unique index on (namespace_id, repository_id) is the whole access path and the whole FK coverage. PostgreSQL does not auto-index FK columns, so without it the integrity check on a repositories delete degrades to a sequential scan of the child partition. Leading with namespace_id keeps partition pruning and still answers the equality-on-both-columns probe the cascade issues, so a separate (repository_id, namespace_id) index would be redundant.

!1944 (merged) and the migration order

The earlier note on this MR concerned !1944 (merged), whose migration 20260826072147_add_npm_and_maven_remote_last_health_status_checks.sql timestamps earlier than this one's 20260826105942 while internal/datastore/migrations/migrations.go sets goose.WithAllowOutofOrder(false). Merging this first would have left !1944 (merged)'s migration out of order, migrations.Up would fail, and onFatal would abort the process for Kubernetes to restart into the same failure.

!1944 (merged) has since merged, so that is settled. This branch is rebased on top of it, goose up applies every 20260826 migration in order locally, and the head is 20260826105942. structure.sql was regenerated with scripts/db/dump-structure.sh on PostgreSQL 17, the version GL_PG_CURR_VERSION names, and comes out byte-identical to the committed file.

What a review of the branch found

I reviewed the branch before opening this and fixed seven real problems, five of them in a file that cannot be edited after merge. Two now live elsewhere in the stack: the RepositoryStore.Delete foreign-key enumeration is part 1 (this MR keeps only the surviving copy, wantCascadeTargets), and repositoryReapWalk's "a virtual repository owns no rows of its own" is part 3, next to the tests for the row it now owns. Two sibling comments say the same thing and travel with it, in lifecycle_reap_repository_test.go and reconcile_namespace_integration_test.go; the precise form the internal/managementapi sites already use is "owns no artifact rows". Inserting that word is not a one-line change: check-comment-caps.sh re-checks a whole block once the diff touches it, and the three sit in grandfathered blocks of 18, 15 and 32 lines against caps of 1, 2 and 2, so part 3 has to carry a compression or a sidecar move with it. The five in the migration file are below.

max_locks_per_transaction was described as a hard per-transaction ceiling. It is not; it sizes the shared lock table together with max_connections and caps no single transaction. The file said otherwise in three places, and falsified itself doing so: the parent CREATE it measures at 66 relations already exceeds the 64 it named as the limit. sql/20260804120000_create_container_remote_repositories.sql states the accurate version and records in so many words that the npm and Maven remote files phrase it the other way, so this branch had reverted a correction the repo already landed. This matters beyond wording: that sentence was the only recorded reason for the "do not batch the partition statements in a DO $$ ... LOOP" rule, so an author who tests the stated ceiling, finds a transaction happily taking 130 locks, and concludes the rule is folklore would collapse the loop and hold locks on the live repositories tree to commit. The layout is unchanged; only its justification.

The insertable-shape list said five and was missing one. The comment enumerates the row shapes the absent CHECK admits, and claims to be exhaustive. It named a soft-deleted parent but not a soft-deleted namespace: namespaces carries deleted_at, purged_at, blocked_at, disabled_at and suspended_at, and fk_nvr_namespace_id_namespaces checks only that the row exists. A half-list is worse than none, because the reader takes the shapes it does not mention as forbidden.

Also fixed: two present-tense references to a write path that does not exist and is explicitly refused (the same earlier comment pass had converted two sibling references to future tense and missed these); a false claim that every npm sibling carries a plain index, when the two repository-level siblings carry none and are the ones a transcription would come from; and an upTimeout claim that described it as bounding one statement's runtime when it bounds the whole Up pass and shares that budget with the advisory-lock retry.

The absence catalogue also moved out from between two column declarations, where it pushed the third column 59 lines from the second, into the table comment where it belongs.

Tests

Twelve cases: eight about the shape the migration positively declares, and four about its foreign keys.

  • _PartitionedTableShape pins the parent, its 64 pNN partitions as a sequence, HASH(namespace_id), and the primary key and its name.
  • _Columns pins the three columns with their types, nullability and column_default. Every sibling suite pins defaults and this one did not, which has teeth for id: a later DEFAULT gen_random_uuid() would hand out v4 ids against the UUIDv7 convention with nothing failing.
  • _UniqueRepositoryIndexShape pins the one index, its uniqueness and its ordered key columns, so a reordering that loses partition pruning fails here.
  • _ConstraintNamesFitIdentifierLimit checks every identifier against 63 characters, because truncation past it is silent and builds a duplicate.
  • _EveryPartitionInheritsTheParentIndexes and _PartitionRoutingByHashOfNamespaceID cover the partition fan-out from both ends.
  • _DownLockBudget and _DownReversesEveryUpObject read the migration file itself, so they hold without applying anything.
  • Constraints_ForeignKeyNamesAndActions reads both foreign keys back from pg_constraint by name and pins each one's confdeltype. It is also what binds the two FK name constants to the schema: their only other reader measures their length against a literal in the same file, so before this a rename in the migration failed no test. The action half is the only reliable guard on the namespaces edge, since no insert can isolate it.
  • Constraints_RepositoriesFKRejectsAbsentRepository, Constraints_RejectsAbsentNamespace and Constraints_RepositoriesFKCascadesOnRepositoryDelete cover the behaviour, mirroring the three npm_remote cases. Checked by mutation: renaming the constant fails the catalog case, and so does expecting a where the migration declares ON DELETE CASCADE.

Run locally against PostgreSQL 16: the 8 cases pass with no skips, and the full ./internal/datastore/migrations package passes at 194s, which includes TestMigrations_UpDownUp proving this migration applies, reverts and re-applies, and TestHeadVersion agreeing with the moved knownHeadVersion. wantCascadeTargets gains npm_virtual_repositories and the internal/datastore delete and reap tests pass. Also go vet -tags=integration, gofmt, squawk (0 issues), pg_format idempotency, scripts/ci/check-comment-caps.sh, and golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false, which reports nothing in the new file.

Size

1078 reviewable lines: 582 source, all of it the migration, 485 test (482 the shape and foreign-key suite, 3 the knownHeadVersion and wantCascadeTargets lines), and 11 documentation (4 the S20-A correction, 7 the lifecycle_reap_repository.md cascade list). A further 1273 lines are generated and excluded (structure.sql 1170, go-jet 103).

The plan applies the 500-line ceiling to reviewable source, and exempts the three schema steps on a named argument rather than on the 2.15:1 test-ratio convention it uses elsewhere: "Partition DDL is what justifies them: the measured band for a sibling partitioned table is 376 to 600 lines of parent plus 64 partitions plus indexes, and goose.WithAllowOutofOrder(false) forbids splitting one table across two migrations." See "How the ceiling is applied" in docs/plans/2026-08-21-s31-npm-virtual.md. At 582 source this sits inside that 376 to 600 band, and it is now the whole of the source in this MR.

The migration itself cannot be split further: one table cannot span two migrations without tripping the out-of-order guard the ## !1944 and the migration order section above is about. Everything that could be split out was, into parts 1 and 3.

E2E scenarios

No scenario in docs/testing/ is added or affected, and the out-of-scope line for virtual npm repositories in docs/testing/e2e/npm.md, which names S31 as their owner, is still correct. This step adds a table with no reader, no writer, no route and no HTTP surface, so there is no end to end flow to script. The plan gives the S31 catalog section a single owning step, step 19.

Follow-ups

  • #548 tracks the schema-wide lock_timeout decision this file defers to. The Up takes ShareRowExclusive on namespaces and the whole 65-relation repositories tree with nothing bounding the wait, which is the existing posture for every partitioned create in the tree, not something this MR introduces.

Database Review Evidence

Collected by the db-review-prep skill, migration mode only. Query mode does not apply: the three-dot diff against origin/main changes six Go files, and each one is either a *_test.go file or jet-generated code, so no hand-written file in this branch dispatches a statement.

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.

Migration PG 16 PG 17 PG 18
20260826105942_create_npm_virtual_repositories.sql OK (396.98ms / 219.74ms) OK (362.85ms / 256.32ms) OK (471.6ms / 233.91ms)

Migration notes: no anomalies flagged.

  • No version-specific regression: the slowest apply (PG 18, 471.6ms) is 1.2x the second-slowest (PG 16, 396.98ms), against a 2x flag threshold.
  • Well inside the boot budget: 471.6ms is the worst apply on an empty database, against the 5-minute per-migration cap in Time budget.
  • Apply and rollback stay close: apply is 1.4x rollback on PG 17, 1.8x on PG 16 and 2.0x on PG 18, which is the band the other partitioned creates sit in. On PG 17, create_repositories.sql is 1.9x, create_npm_remote_files.sql 1.7x and create_container_remote_repositories.sql 1.4x.
  • The apply cost matches the closest sibling by shape almost exactly: create_npm_repositories.sql, the other hash-partitioned per-format repository marker table, applies in 362.92ms on PG 17 against this table's 362.85ms.

Evidence from the merged-results pipeline #2797656312 for MR head d1c7b7f3. The db:migrate re-apply phase is idempotency verification only and is excluded from the table.

Related to #882 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading