feat(npm): add npm_versions.size_bytes with backfill and size index
🎯 What this MR does
Adds the ADR-007 artifact-level storage accounting column for hosted npm — npm_versions.size_bytes — together with its size-ordered list index and a backfill. This is step 1 of a three-MR stack implementing per-version size accounting for npm.
| Step | MR | What it delivers |
|---|---|---|
| 1 · this MR | npm_versions.size_bytes column + index + backfill |
|
| 2 | !1402 (merged) | Maintenance on publish & cache-fill, size in the management API |
| 3 | !1403 (merged) | versions_count / tags_count semantics fixes on unpublish |
🧩 Context — why this exists
ADR-007 specifies a pre-computed size_bytes on every Maven/npm version table: deriving one version's size is cheap, but the moment size becomes a sortable column on the version list, deriving it per row stops scaling. ADR-007 measured the difference at 29 ms vs 0.08 ms for a top-50-by-size page on ~26K versions.
Two of the four version tables carry the column today — npm_remote_versions and maven_remote_versions. npm_versions is the gap this MR closes: S11's buffered-column enumeration omitted it, and #549 (closed) tracks closing it. This stack implements #549 (closed) for hosted + remote npm. maven_versions is the remaining gap and is out of scope here: it is tracked separately as #550 (closed), the Maven sibling of #549 (closed).
Process note
🔍 : this stack ships without a prior plan MR by explicit operator decision — the plan-level research and decisions are recorded in these MR descriptions instead.
🔁 Changed since the last review round
Three review findings, all addressed:
- Rebased on
mainand the three migrations renumbered to20260811120000/20260811120100/20260811120200.mainhad gained20260810120000_add_ns_enc_keys_tombstone_index.sql, which collided with this MR's first prefix — goose refuses duplicate versions atNewProvider, so the merged chain would have failed every boot and every integration job. The new prefixes sort aftermain's head, whichWithAllowOutofOrder(false)requires for any environment that already applied it. Every comment that cited a companion by its timestamped filename now cites it by basename glob (sql/*_backfill_npm_versions_size_bytes.sql), so the next renumber sweeps nothing.migrations_checksum_test.gokeeps the full name, because there the version prefix is the subject — andTestHeadVersionrecomputes the head independently, so it fails ifmainever lands a migration sorting after this trio. - The backfill re-run now states its precondition — see the Deploy order section below. It is idempotent only against a quiet publish path, and it does not repair a version whose files have all been hard-deleted since.
- The
NO TRANSACTIONassertion moved from the extractedDownsection to the whole file. goose reads that directive file-level: one occurrence anywhere disables the transaction for both directions. A section-scoped assertion stayed green while a directive copied under-- +goose Upmade theDownnon-transactional.
📦 What's inside
Three migrations, one concern each.
- Migration
20260811120000—size_bytes bigint NOT NULL DEFAULT 0onnpm_versions(metadata-only, constant default). No non-negativityCHECK— see the section below, which reverses an earlier revision of this MR. - Migration
20260811120100— the backfill. Set-based recompute grouped by version, with the two properties ADR-007 pins:SELECT DISTINCT blob_sha256first, then the size join —SUM(DISTINCT size)would collapse equal-length blobs, and a duplicate attachment of one digest must count once;- soft-deleted files included — bytes leave the counter at hard-delete only; soft-delete and restore are no-ops.
- Migration
20260811120200— the partial index(namespace_id, npm_package_id, size_bytes DESC) WHERE soft_deleted_at IS NULL, mirroringindex_npm_remote_versions_on_ns_id_pkg_id_size_byteson the remote sibling. Ordered after the backfill on purpose: built before it, the index would cover an all-zero column and then have every entry rewritten, and each backfilled row would maintain one more btree while that happened. Built after, it is dense in one pass. It is a blocking parent-level build across the 64 partitions — safe while the tables are empty in dev and no production deployment exists; the file documents the per-partition path to use once that stops being true. - Jet models +
structure.sqlregenerated.structure.sqlis byte-identical whichever order the three migrations run in — the split changes the path, not the destination. - Schema integration tests — see the Testing section below.
⏱️ Deploy order — please read before merging
The backfill is a one-shot absolute recompute (SET size_bytes = agg.bytes). The code that keeps the column current is AddNpmVersionSizeBytes in !1402 (merged), which applies a delta (GREATEST(size_bytes + delta, 0)). A delta never repairs a value it did not start from, so any npm version published between this migration applying and !1402 (merged)'s binary serving traffic keeps size_bytes = 0 permanently. The window is at minimum the rolling-deploy overlap, since migrations run at pod boot, and at most the gap between the two releases if the stack merges out of order.
Re-running the backfill is the repair and is idempotent. Nothing performs it automatically: there is no size-reconciliation sweep anywhere in this stack. An earlier draft of this description credited !1402 (merged) with a "reconciliation recompute"; that was wrong, and the row above has been corrected. !1402 (merged) names SumNpmFileSizesByVersion in a doc comment but defines no such function, in that MR or in !1403 (merged) — that gap needs closing on !1402 (merged) before the counter has any verification path.
What this asks of the merge:
- Merge and deploy !1402 (merged) with or before this migration reaches any environment that holds npm data.
- If it reaches such an environment first, re-run the backfill (
sql/*_backfill_npm_versions_size_bytes.sql) after !1402 (merged) is live — with npm publishes quiesced and buffered deltas drained, or one namespace at a time while that namespace is idle.
The re-run's idempotence has two limits, and both are silent.
- A publish running concurrently loses or doubles its delta. The
UPDATEcomputesaggunder its own statement snapshot. A file and its delta that commit after the snapshot but before the row write have the delta overwritten; a file visible to the snapshot whose buffered delta flushes after the row write is counted twice. An undercount never trips the negative-total signal a delta-merged counter gives, and nothing converges it afterwards — there is no size-reconciliation sweep. Hence the quiesce above. - It does not repair a version whose
npm_filesrows have all been hard-deleted since.agghas no row for it, so theUPDATEskips it and a stale non-zerosize_bytessurvives untouched.DownthenUpis the repair for that shape: theDownresets every row to 0, which is the state theUprecomputes from.
Both migration files state this in their own comments, so the constraint survives outside this description. It is not feat-blocking on an empty database, which is the state of every environment today.
🧮 Why the counter has no non-negativity CHECK
Changed since the first revision of this MR. An earlier revision added
CHECK (size_bytes >= 0)and argued for it at length. It is removed. Reviewers who read that revision should read this section instead — the reasoning reversed.
This is not a deviation from ADR-007. The ADR specifies no constraint on this column, and at its buffered-counter section it classifies version-table size_bytes as a Counter that "sum[s] the buffered deltas into the existing value". Adding the CHECK was the departure; removing it is the return.
The line that decides it is measured-once versus merged-as-deltas:
| Constrained today | Shape |
|---|---|
blob_storage_blobs.size, container_manifests.size, container_remote_manifests.size, upload_sessions.size_bytes |
written from a single measurement — a negative is unreachable except through corruption, so rejecting it costs nothing |
npm_versions.size_bytes (this column), repositories.size_bytes, repositories.artifacts_count, repositories.downloads_count, npm_packages.versions_count, npm_packages.tags_count, npm_remote_versions.size_bytes, maven_remote_versions.size_bytes |
merged from a stream of deltas off the request path — none is constrained |
With the CHECK, this column would have been the only delta-merged counter in the schema carrying one.
Two reasons it does not:
- A negative total is information, not corruption. For a delta-merged counter it is the arithmetic reporting that more was subtracted than was ever added, which localizes the defect to the emitting site. A
CHECKdestroys that signal at the moment it appears. - It converts a wrong number into an outage without making the number right. The decrement sites are background jobs (the lifecycle purger, reconciliation). A
check_violationaborts the transaction and the job enters a retry loop; the counter stays wrong either way.
There was also a coherence problem worth naming, since it is what prompted the removal. AddNpmVersionSizeBytes in !1402 (merged) clamps its own arithmetic with GREATEST(size_bytes + delta, 0). Because the clamp runs first, the maintained path could never reach the constraint — so the CHECK was not making drift loud, it was only failing writers that bypassed the store. It bought no detection and cost availability.
Follow-on for !1402 (merged): with the constraint gone, the clamp is the remaining thing suppressing the signal. If a negative is what we want to see, GREATEST(..., 0) should come off too. That is a change to !1402 (merged) and is not made here.
A negative must therefore be handled at the read boundary — storage reporting reads this column, and the read is the only place that knows what to show a user. That obligation lands with the size field in !1402 (merged).
⚖️ Two deliberate deviations
1 · The backfill joins blob_storage_blobs, not ADR-007's shadow table. ADR-007's artifact-level recompute examples join blob_storage_blobs_by_namespace. That table does not exist yet — it arrives with S22's Steps 2a/2b, whose implementation has not started. The backfill joins blob_storage_blobs directly instead: per-version file cardinality is protocol-bounded (one tarball per npm version today), so the join is a handful of index probes and does not need the shadow's single-partition locality. This decouples the stack from S22's timeline; switching the join later is a one-line change.
2 · The index ships now, before anything reads it. ADR-007 specifies this index, so shipping it with the column is the spec-compliant choice and this MR takes it. The cost is worth naming rather than leaving implicit, because it is paid from the moment the index exists and migrations are immutable once merged:
- An update touching an indexed column is never HOT, and a non-HOT update writes an entry into every index that admits the row, including ones whose columns did not change.
npm_versionsnow has six indexes admitting an active row (the seventh is partial onsoft_deleted_at IS NOT NULL).BumpLastDownloadedAtfires once per served npm tarball and was already non-HOT becauselast_downloaded_atis indexed. It now writes six index tuples instead of five — roughly 20% more index write volume on that path.- No query in !1401 (merged), !1402 (merged) or !1403 (merged) orders or ranges on
size_bytes. The read that pays for the index is the size-ordered version list, which lands later.
The alternative — ship the column and backfill here, add the index in the MR that adds the size-ordered list — is reasonable and was considered. It was rejected because ADR-007 is the source of truth for the schema and splitting the index away from the column it indexes makes the ADR harder to check against the tree. Flagging it so the trade-off is a decision on the record rather than an omission.
🧪 Testing
Schema integration tests pin:
- column shape (
bigint NOT NULL DEFAULT 0); - the deliberate absence of the non-negativity
CHECK, together with the behaviour that absence buys: a negative total is storable on both write shapes — an insert of a negative literal and a decrement past zero — and reads back as written. The constraint assertion is made against the predicate (%size_bytes%>=%0%) rather than a constraint name, so re-adding theCHECKunder any name fails the test; - the index through the shared
indexDefhelper, sopg_index.indisvalidis asserted rather than only the definition text. This matters here specifically:pg_get_indexdefrenders a partitioned-parent index asCREATE INDEX ... ON ONLY ...whether or not its 64 children are attached, soindisvalidis the only signal separating a fully recursed build from a childless one — which is exactly what the per-partition escape hatch the migration documents would produce if a partition were missed; - all three
Downsections, including the goose lock directives every DDLDownin this package asserts (assertNoTransactionDirectivePresent,assertNoBatchingDoBlock,assertOneStatementPerDetachOrDrop). For the backfill the assertion is the absence ofNO TRANSACTION, and it is made over the whole file rather than the extractedDown: both its sections are single data statements and the transaction is what makes the recompute and the reset atomic, and goose reads the directive file-level, so one occurrence anywhere would disable both directions; - the ordering of the three migration files, since nothing but their version prefixes keeps the index build after the backfill;
- the backfill's dedup, equal-size and soft-delete properties, by replaying the migration's own extracted
Upstatement against seeded rows (newextractGooseUpSectionhelper, mirroring the existing Down extractor).
The backfill fixture is chosen so the correct query and each plausible wrong one produce a different total, which the previous fixture did not:
| Query | Total |
|---|---|
the recompute as written (DISTINCT digest, then join) |
500 |
SUM(DISTINCT size) over the un-deduplicated join |
300 |
plain SUM with no inner DISTINCT |
600 |
The fixture adds a fourth file under a distinct digest with the same byte count as an existing one. Without it, SUM(DISTINCT size) also returns the expected total and the property the migration calls load-bearing has no test.
✅ Verification
mise run db:lint(squawk): clean, 0 issues across 60 files.TestMigrations_UpDownUp(full chain up → down → up over the three-way split): pass.CHECKremoval verified on the dev database:pg_constrainthas 0 rows matchingcheck_npm_versions_size_bytes_non_negative(it had 65 — parent plus all 64 partitions — before the removal).structure.sqlregenerated. Against the previous revision it loses exactly 65 lines, all of them theCONSTRAINT ... CHECK ((size_bytes >= 0))clause;size_bytes bigint DEFAULT 0 NOT NULLis untouched on the parent and every partition. The three-way migration split changes nothing in the dump — the split changes the path, not the destination.- Full
./internal/datastore/migrations/integration suite: pass. golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0on the package: 0 findings in the files this MR adds or edits. The twoerrcheckfindings an earlier revision accepted are gone with theCHECK: the test no longer callsassertCheckViolationat all. Note CI lint never compiles these files:.golangci.yamlsets norun.build-tags.- Backfill plan on PostgreSQL 17: three unpruned
Appendscans (all 64 partitions ofnpm_files,blob_storage_blobsandnpm_versions) feeding two hash builds and oneUpdate on npm_versions. No partition pruning is possible at any level — there is nonamespace_idliteral, andblob_storage_blobsisPARTITION BY HASH (sha256)with the digest arriving from the join. Fine at current volumes; the migration file carries the populated-table caveat and names the batched replacement.
Known gaps, stated rather than hidden
- No runtime signal that the backfill produced correct values. goose logs
version,direction,stateandduration_seconds, not rows affected, so the operator sees20260811120100 ok, 0.28swhether it updated everything or nothing. This is existing practice for every migration in the repo, not something this MR introduces, but combined with the deploy-order note above it means drift is created silently and is not detectable at runtime. The integration test is the strongest check that exists today. lock_timeoutis unset, as it is for every migration in this repo. The waivers in all three files now name the lock each statement takes and describe the exposure as a bounded stall plus an unbounded wait, rather than claiming there is no contention. The schema-wide decision is tracked in #548.
Note on the review threads
🔒 : the AppSec review is answered in its own thread. Duo's review reported nothing to comment on.
🧪 E2E scenario impact
None: this MR is schema + backfill only — no request path changes behavior. The e2e catalog impact for the stack lands with MR 2 (API surface) and MR 3 (unpublish semantics), which state theirs individually.
Related to #549 (closed)