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.

📚 The stack (each MR targets the one before it):

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:

  1. Rebased on main and the three migrations renumbered to 20260811120000 / 20260811120100 / 20260811120200. main had gained 20260810120000_add_ns_enc_keys_tombstone_index.sql, which collided with this MR's first prefix — goose refuses duplicate versions at NewProvider, so the merged chain would have failed every boot and every integration job. The new prefixes sort after main's head, which WithAllowOutofOrder(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.go keeps the full name, because there the version prefix is the subject — and TestHeadVersion recomputes the head independently, so it fails if main ever lands a migration sorting after this trio.
  2. 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.
  3. The NO TRANSACTION assertion moved from the extracted Down section 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 Up made the Down non-transactional.

📦 What's inside

Three migrations, one concern each.

  • Migration 20260811120000size_bytes bigint NOT NULL DEFAULT 0 on npm_versions (metadata-only, constant default). No non-negativity CHECK — 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_sha256 first, 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, mirroring index_npm_remote_versions_on_ns_id_pkg_id_size_bytes on 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.sql regenerated. structure.sql is 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:

  1. Merge and deploy !1402 (merged) with or before this migration reaches any environment that holds npm data.
  2. 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 UPDATE computes agg under 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_files rows have all been hard-deleted since. agg has no row for it, so the UPDATE skips it and a stale non-zero size_bytes survives untouched. Down then Up is the repair for that shape: the Down resets every row to 0, which is the state the Up recomputes 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:

  1. 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 CHECK destroys that signal at the moment it appears.
  2. 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_violation aborts 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_versions now has six indexes admitting an active row (the seventh is partial on soft_deleted_at IS NOT NULL).
  • BumpLastDownloadedAt fires once per served npm tarball and was already non-HOT because last_downloaded_at is 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 the CHECK under any name fails the test;
  • the index through the shared indexDef helper, so pg_index.indisvalid is asserted rather than only the definition text. This matters here specifically: pg_get_indexdef renders a partitioned-parent index as CREATE INDEX ... ON ONLY ... whether or not its 64 children are attached, so indisvalid is 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 Down sections, including the goose lock directives every DDL Down in this package asserts (assertNoTransactionDirectivePresent, assertNoBatchingDoBlock, assertOneStatementPerDetachOrDrop). For the backfill the assertion is the absence of NO TRANSACTION, and it is made over the whole file rather than the extracted Down: 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 Up statement against seeded rows (new extractGooseUpSection helper, 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.
  • CHECK removal verified on the dev database: pg_constraint has 0 rows matching check_npm_versions_size_bytes_non_negative (it had 65 — parent plus all 64 partitions — before the removal).
  • structure.sql regenerated. Against the previous revision it loses exactly 65 lines, all of them the CONSTRAINT ... CHECK ((size_bytes >= 0)) clause; size_bytes bigint DEFAULT 0 NOT NULL is 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=0 on the package: 0 findings in the files this MR adds or edits. The two errcheck findings an earlier revision accepted are gone with the CHECK: the test no longer calls assertCheckViolation at all. Note CI lint never compiles these files: .golangci.yaml sets no run.build-tags.
  • Backfill plan on PostgreSQL 17: three unpruned Append scans (all 64 partitions of npm_files, blob_storage_blobs and npm_versions) feeding two hash builds and one Update on npm_versions. No partition pruning is possible at any level — there is no namespace_id literal, and blob_storage_blobs is PARTITION 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, state and duration_seconds, not rows affected, so the operator sees 20260811120100 ok, 0.28s whether 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_timeout is 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)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading