chore(datastore): add npm_versions.attestation_predicate_type (S11 Provenance Attestations plan: 5/20)

Hayley Swimelarrequested to merge
1273/step-05-attestation-predicate-type-migration into main

Why

The packument render has to answer two questions about every version: whether to emit dist.attestations at all, and which predicate type to name when it does. S11 answers both from one nullable column on npm_versions, so the render gets what it needs from the single-table SELECT it already runs and never reaches the blob store. NULL means the version has no stored bundle, and that absence is the presence flag on purpose.

The value is stored rather than assumed because it varies by CI provider. libnpmpublish emits https://slsa.dev/provenance/v0.2 from its GitLab CI branch and https://slsa.dev/provenance/v1 from its GitHub Actions branch.

Nothing reads or writes the column until Step 9 plumbs it through the store. The plan records that as an accepted smell.

Targets main. It was stacked on Step 2, which has merged as refactor(npm): split the publish charge into a ... (!2803 - merged) • Hayley Swimelar • 19.5. The dependency was chain-only: this migration uses nothing Step 2 landed. The transplant onto main needed no re-stamp, because 20260918231856 still sorts above main's newest migration, 20260918120000.

What

The two timeout rules are satisfied rather than suppressed, and that departs from the plan's text. Step 5 asks for "the same three squawk-ignore-file headers the size_bytes precedent carries". This file carries no file-level header and one statement-level suppression. Stripping every pragma and running squawk 2.64.0 fires three rules: require-lock-timeout and require-statement-timeout on the Up's ALTER TABLE, and ban-drop-column on the Down's DROP COLUMN. The first two have a compliant form that docs/dev/database-migrations.md names, so both sections bracket their ALTER TABLE with SET lock_timeout = '5s' and SET statement_timeout = '30s' and reset each after it. Only ban-drop-column is suppressed, on the line squawk reports it on, because PostgreSQL offers no way to un-add a column without dropping it. Guardrail 25 requires that derivation, so please read this as a deliberate improvement rather than drift from the plan.

The precedent the plan points at is stale twice over. Its own header claims "No migration in this repo sets lock_timeout", which 21 files now falsify, and its third header prefer-robust-stmts does not fire here at all, because both statements carry IF NOT EXISTS and IF EXISTS. A probe confirmed the plan's squawk-ignore-file require-timeout-settings still silences both timeout rules under 2.64.0, so following the plan literally would have gone green while excusing a finding that has an answer.

The two values come from 20260915153000_drop_npm_remote_plaintext_credentials.sql, whose Down brackets a restoring ALTER TABLE ... ADD COLUMN at the same pair, over a 65-relation family of its own in npm_remote_repositories. 30s covers the accumulation case of six waits that each stay under the 5s lock_timeout. Six waits that each reach 5s is not a case it has to cover, because lock_timeout aborts at the first of them with 55P03 and does not retry. Worth knowing for review: the same-table precedent, 20260811120000_add_npm_versions_size_bytes.sql, sets no timeouts at all and defers the question to Set lock_timeout on the migration Up pass, prov... (#548) • Unassigned, so it is a precedent for the column and not for these values.

text with no length bound, while every other text column on this table bounds at 255 characters. The spec bounds this value only through the bundle cap. A column-level rule would be a second and tighter bound, and the plan takes no such decision, recording the gap for the spec's owner. Two tests pin it in both directions: no CHECK constraint names the column, and an 8 KiB value round-trips verbatim. Nothing writes the column yet, so the bound question arrives with the publish path rather than with this migration.

Two files beyond the plan's declared list, both forced by a failing gate. migrations_checksum_test.go moves its knownHeadVersion pin, which its own doc comment says a landing migration does. npm_schema_integration_test.go gains three lines, because TestNPMSchema_Columns asserts an exact column count on npm_versions. The pin rides the implementation commit rather than the test commit on purpose: that file carries no build tag, so bumping it earlier would have made TestHeadVersion red under the go-test hook and forced a bypass. Deferring it is why this branch carries zero --no-verify commits.

Size

482 added lines, inside the 500 reviewable-LOC guardrail and further inside it once generated files come out: 274 test, 88 SQL, and 120 generated (structure.sql 65, the jet model and table 55).

Test plan

docker run --name ar-pg-s5 -e POSTGRES_USER=artifact_registry \
  -e POSTGRES_PASSWORD=password -e POSTGRES_DB=artifact_registry \
  -p 5432:5432 -d postgres:16-alpine -c max_locks_per_transaction=4096

ARTIFACT_REGISTRY_DATABASE_TEST_DSN="postgres://artifact_registry:password@localhost:5432/artifact_registry?sslmode=disable" \
  mise exec -- go test -tags=integration ./internal/datastore/migrations/
Covers Test
The column is text, nullable, and carries no default TestNpmVersionsAttestationPredicateType_ColumnIsTextNullableNoDefault
An insert omitting the column leaves NULL, not the empty string TestNpmVersionsAttestationPredicateType_AbsentValueReadsAsNull
A real predicate type round-trips off a hash partition TestNpmVersionsAttestationPredicateType_ValueRoundTripsToPartition
No length bound and no CHECK, so an 8 KiB value stores verbatim TestNpmVersionsAttestationPredicateType_LongValueStoresUnbounded
The Up adds a bare nullable column, with no default, CHECK, or index TestNpmVersionsAttestationPredicateType_UpAddsBareNullableColumn
Both sections set each bound before the statement and reset it after TestNpmVersionsAttestationPredicateType_BothSectionsBoundTheirLocks
The Down drops exactly one column, from npm_versions TestNpmVersionsAttestationPredicateType_DownDropsColumn

Reversibility rides TestMigrations_UpDownUp, which executes this Down on its way to zero, plus the static Down test above.

Each assertion was checked against the mutation it exists to catch: a varchar(255) column, text NOT NULL DEFAULT '', a second DROP COLUMN, a DROP COLUMN retargeted to npm_packages, a SET and RESET pair in the wrong order, an added CREATE INDEX, and an out-of-order stamp each fail a named test. A DROP COLUMN appearing only inside a comment does not, because the count strips SQL line comments first.

Guardrails

Database Review Evidence

Migrations

Note

apply / rollback per PG version, from goose verbose against an empty database. The numbers come from local postgres:<version>-alpine containers running the db:migrate job's own three-phase script at the three matrix versions, measured before this MR had a pipeline, so the version headers carry no job links. Production-scale validation via Database Lab is not yet available. docs/dev/database-migrations.md covers the matrix rationale and how to read the numbers.

Migration PG 16 PG 17 PG 18
20260918231856_add_npm_versions_attestation_predicate_type.sql OK (12.46ms / 23.8ms) OK (13.29ms / 18.06ms) OK (12.91ms / 25.93ms)

Migration notes:

  • No anomaly. Apply spans 12.46ms to 13.29ms across the three versions and rollback 18.06ms to 25.93ms, so the slowest version is 1.03x the second-slowest on apply and 1.09x on rollback, well under the 2x divergence bar. Every phase reported OK on every version, and the slowest number is 25.93ms against a five-minute boot budget.
  • Rollback runs 1.4x to 2.0x the apply on each version. That is run-to-run noise rather than an asymmetric Down: the excluded re-up phase re-applied the same migration at 15.87ms, 13.77ms, and 18.86ms, so apply alone varies 1.5x between runs of one version on this host.
  • The apply is metadata-only, established from the catalog rather than from the statement text. An md5 over pg_class.relfilenode for all 65 relations in the npm_versions partition tree is identical before and after the ALTER TABLE on all three versions (PG 17: 7ca5ad0a082a58317e76ee92c60e3807 both times), while the count of relations carrying attestation_predicate_type goes from 0 to 65. The statement rewrites no heap.
  • The statement locks 65 relations, as the file claims. Running the exact ALTER TABLE inside a transaction and reading pg_locks gives 1 AccessExclusiveLock on the partitioned parent plus 64 on the leaf partitions, and no other relation held ACCESS EXCLUSIVE. Identical on PG 16, 17, and 18.
  • An empty database measures the uncontended cost only. It does not exercise the lock wait those 65 acquisitions queue behind on a live fleet, which is what the lock_timeout and statement_timeout bounds in the file are for.
Context for LLM agents

Rationale

  • Copy the size_bytes precedent's three squawk-ignore-file headers, which is what the plan's Files entry asks for. Rejected because guardrail 25 and docs/dev/database-migrations.md both require deriving each suppression from what actually fires, and the two timeout rules have a compliant form. A probe confirmed the copied squawk-ignore-file require-timeout-settings still silences both rules under squawk 2.64.0, so the copy would have passed CI while excusing a finding that has an answer. The precedent's third header fires on nothing here.
  • Keep statement_timeout at the 60s the first draft used. Rejected because 60s appears in no other migration (eight siblings carry 5s, 30s, or 120s), and the only justification available for it bounds the value from above without deriving it, which would equally justify 120s. 60s doubles the window in which the parent's ACCESS EXCLUSIVE blocks every packument render.
  • Scope the bounds with SET LOCAL. Rejected because -- +goose NO TRANSACTION means there is no transaction to scope to.
  • Bound the column with varchar(n) or a length CHECK, matching the table's other text columns. Rejected because the bound is a spec decision the plan explicitly declines to take.
  • Chosen: satisfy both timeout rules with a bracketed SET and RESET pair at the two values the structurally comparable sibling derives, and suppress only ban-drop-column.

Consequences: the SET and RESET pair leaks the session bounds when a statement between them fails. goose pins one connection for a NO TRANSACTION migration, returns on the first error without resetting the session, and hands that connection back to the pool. All 21 sibling migrations setting these bounds behave identically, and the blast radius is closed by wiring rather than by SQL: NewMigrationClient forces MaxConns=1 and Runner.Start shuts the client down on a failed Up. Diverging here alone would buy nothing and break the house form.

Non-goals

  • No index. Nothing filters or orders on the column, and the packument render reads it off a row it already has.
  • No backfill. Every version that exists predates the store path, so NULL is already the correct value for all of them.
  • No store, handler, or render wiring. Later steps plumb the column through the store, write it at publish, and render it into dist.attestations.
  • No ADR 007 amendment. ADR 007 is not a complete table definition, and S11's Data Model is where this column is specified.
  • No answer to the absent length bound. That is a spec decision, recorded in the plan for the spec's owner, and a migration is the wrong place to settle it.

Related to #1273

Edited by Hayley Swimelar

Merge request reports