refactor(datastore): convert container_manifest queries to jet
Why
internal/datastore/container_manifest.go is the last datastore file built entirely on raw SQL strings, and docs/dev/database-query-patterns.md mandates the jet builder for datastore queries. The review of the stacked base converted only the new list statement and raised converting the rest as follow-up. The stacked base has merged, so this targets main directly.
What
- Rows scan into the generated
model.ContainerManifestsand convert through the newcontainerManifestFromModel, the idiom the rest of the package uses. We rejected alias tags on the 16 fields and a wider domain-type surface (29 use sites). The tag route carries the silent zero-scan tracked in Document go-jet's silent zero-scan in the query... (#410 - closed) • Hayley Swimelar. - jet's qrm zeroes SQL NULL into
json.RawMessagenatively, so the hand scanner's NULL plumbing retires with the scanner. ChildManifestsExistbinds a single= ANY($n::bytea[])array parameter, independent of digest count.qrm.ErrNoRowsis now the single not-found sentinel.- The referrers builder drops its unreachable no-LIMIT branch.
- Both page reads keep a bounded pre-allocation on the scan destination. qrm honors a capacity the caller sets, so retiring the hand scanner's hint would otherwise have cost real bytes and time (see
## Benchmark). The manifests list hints its fetch size rather than its page size, because the extra probe row would force a growth on every full page.
Size
The diff is ~1,850 changed lines against the 500-line guardrail. The scanner retirement is atomic: converting statements one MR at a time would leave two scanning mechanisms in the file. Roughly 85% of the delta is test churn, and 752 of the lines are deletions.
Mechanical gates
Three greps stand in for two claims, "no raw SQL remains" and "the retired symbols are gone". All three return nothing on this branch:
rg -n '=\s*`(SELECT|INSERT|DELETE)' internal/datastore/container_manifest.go
rg -n 'const stmt' internal/datastore/container_manifest.go
rg -wn 'scanContainerManifest|scanContainerManifestRows|queryOneContainerManifest|rowScanner|buildReferrersQuery|fakeRow|assignScanValue|scanFixture' internal/ docs/dev/The first anchors on assignment, so the doc-comment code span quoting SELECT digest, size in prose survives deliberately.
Benchmark
Three benchmarks cover the converted read paths, hand scanner vs jet, benchstat at n=10. The gate this conversion set for itself: a statistically significant sec/op regression at or below 30% ships with the delta recorded, and above 30% ships with an explicit callout and the hybrid fallback put to the reviewer.
BenchmarkManifestScanWithCapAnnotations drives GetContainerManifestByDigest against a ~520 KB annotations row:
| Metric | Hand scanner | jet | Delta |
|---|---|---|---|
| sec/op | 3.376m ± 21% | 4.179m ± 14% | +23.80% (p=0.000) |
| B/op | 1.508Mi ± 2% | 2.696Mi ± 1% | +78.82% (p=0.000) |
| allocs/op | 140.0 ± 1% | 542.0 ± 0% | +287.14% (p=0.000) |
BenchmarkListReferrersPage (1000 rows) and BenchmarkListContainerManifests (100 rows) are new here, added because review asked for the page paths and the conversion had measured only the single-row read. Both leave annotations NULL, so the numbers isolate row-count scaling:
| Benchmark | Metric | Hand scanner | jet | Delta |
|---|---|---|---|---|
| ListReferrersPage | sec/op | 3.763m ± 19% | 7.790m ± 10% | +107.02% (p=0.000) |
| ListReferrersPage | B/op | 1.329Mi ± 0% | 3.672Mi ± 0% | +176.38% (p=0.000) |
| ListReferrersPage | allocs/op | 27.97k ± 0% | 53.37k ± 0% | +90.78% (p=0.000) |
| ListContainerManifests | sec/op | 757.0µ ± 20% | 1259.4µ ± 10% | +66.37% (p=0.000) |
| ListContainerManifests | B/op | 198.8Ki ± 0% | 389.5Ki ± 0% | +95.93% (p=0.000) |
| ListContainerManifests | allocs/op | 2.712k ± 0% | 5.135k ± 0% | +89.34% (p=0.000) |
Read sec/op with its spread, not to two decimals. jet allocates roughly 3x the bytes per page, so it drives 3x the GC rate, which makes wall time sensitive to GOGC and to a container Postgres sharing cores. B/op and allocs/op reproduce to within 0.05% across runs and carry the confidence. Round the timings to "roughly 2x slower on referrers, 1.7x on the list".
Those figures already include the pre-sizing commit on this branch. Without it the destination grows unhinted and the same benchmarks read +143.01% and +211.03% on referrers, +124.48% and +111.95% on the list, so restoring the bound is worth 36 points of referrers sec/op and 35 of its B/op.
Over the gate: a reviewer decision
Both page reads clear the 30% sec/op gate by a wide margin, so the escalation applies.
Three terms make up the +25,410 allocations and +2.4 MB on a 1000-row referrers page:
- Roughly 23 allocations per row on qrm's per-column reflection assign path. This is the bulk of it and it is inherent to scanning through the generated model.
- 2.26 allocations and 364 bytes per row in qrm's result-grouping machinery, which builds a
uuid.String()-formatted group key per row and retains a 1000-entry map. That is multi-table dedup work running on a flat single-table SELECT, measured by dropping the model's twoprimary_keytags. - The destination's growth, now bounded by the pre-sizing commit.
Per-row cost dominates: the absolute allocation delta is +25,410 at 1000 rows against +2,430 at 101, a 10.5x spread for a 9.9x change in row count. A fixed per-page term cannot produce that.
The pull path is not what regressed. GetContainerManifestByDigest, the read every manifest pull and push idempotency probe goes through, is the +23.80% row. The two paths that regress hardest are the OCI referrers API and a management-API list.
Two caveats on scope. The NULL-annotations fixture understates the real gap rather than bounding it: jet's path copies an annotations value four times (pgx decode, database/sql clone, qrm's assign through sql.NullString, then containerManifestFromModel) against the hand scanner's two, so the byte delta grows with payload instead of holding constant. And the two referrers statements are not textually identical across the comparison, though they select the same 16 columns with the same predicates, order, and LIMIT, so they return the same rows. jet's is longer and aliased, which costs per page under the simple protocol, not per row.
The hybrid fallback stays on the table: keep the hand scanner for these two page reads, take jet for everything else. We do not implement it here, because it re-creates the two-scanning-mechanisms state this change exists to end. Whether a 2x referrers page is worth that trade is the reviewer's call.
Neither page read is gated in CI, and no Go benchmark in this repo is. A test:bench-regression job that hard-gated B/op and allocs/op existed and was reverted in e4f6ebcf. Rebuilding it inside this refactor is the wrong place for it, so it is tracked in Gate Go benchmarks in CI on B/op and allocs/op (#524) • Unassigned.
Spec coverage
| # | Criterion | Tests / gates |
|---|---|---|
| AC-1 | No raw SQL remains in internal/datastore/container_manifest.go, and every statement builds with jet |
The first two greps in ### Mechanical gates |
| AC-2 | Retired symbols gone repo-wide, code and comments | The third grep in ### Mechanical gates |
| AC-3 | All store method signatures unchanged, diff confined to the planned files, no caller edits | go build ./... with callers untouched, plus a diff audit against main |
| AC-4 | Unit suite green, integration suite green, lint clean | go test ./..., go test -tags=integration ./internal/datastore/, go-lint-ci ./... |
| AC-5 | The four .Sql() builder tests and the converter test pass, including the 3-args-at-5-digests N-independence pin and the 14-arg Create shape |
TestCreateContainerManifestStmt_SQL, TestGetContainerManifestByDigestStmt_SQL, TestChildManifestsExistStmt_SQL, TestListReferrersPageStmt_SQL, TestContainerManifestFromModel |
| AC-6 | Converted EXPLAIN tests pass against builder-produced SQL, still asserting single-partition pruning, Index Scan, truncated child-index suffixes | TestGetContainerManifestByDigest_SinglePartitionPruning, TestListReferrersPage_PartialIndexScan, TestListContainerManifests_ViewsRideTheirOwnIndex |
| AC-7 | NULL-vs-{} annotations contract holds end to end |
TestContainerManifestFromModel NULL/empty/multi-key cases, TestGetContainerManifestByDigest_NullAnnotations, TestListReferrersPage_NullAnnotations, TestCreateContainerManifest_AnnotationsRoundTrip_GeneratedShapes, and TestListContainerManifests_RowFidelity sparse row |
| AC-8 | Benchmark delta recorded in the MR description | The two tables in ## Benchmark |
| AC-9 | docs/dev/database-query-patterns.md jsonb section describes the shipped jet form |
Doc edit in the implementation commit |
| AC-10 | MR description: why-first, Related to line, e2e-catalog exemption statement |
This description |
Test plan
go test ./...
go test -tags=integration ./internal/datastore/
go-lint-ci ./...To reproduce the page-read deltas you need the integration DB, and the benchmark file only exists on this branch, so the hand-scanner side runs with that one file dropped onto main:
BENCH='-tags=integration -run ^$ -benchmem -count=10'
NAMES='BenchmarkList(ReferrersPage|ContainerManifests)$'
SRC=internal/datastore/container_manifest_bench_integration_test.go
go test $BENCH -bench "$NAMES" ./internal/datastore/ > /tmp/jet.txt
git worktree add ../ar-bench-base main
git show HEAD:$SRC > ../ar-bench-base/$SRC
(cd ../ar-bench-base && go test $BENCH -bench "$NAMES" ./internal/datastore/) > /tmp/base.txt
benchstat /tmp/base.txt /tmp/jet.txtOCI conformance runs in CI (no local rig in the authoring environment). The integration suite covers the store contract locally.
Behavior-preserving refactor: no e2e scenario added or affected, docs/testing/ untouched.
Context for LLM agents
Rationale
- Alias tags on all 16 manifest columns. Rejected: a mistyped tag scans zero values with no error.
- Widening the domain type to match jet's model. Rejected: 29 use sites churn for a datastore-internal concern.
- Hybrid fallback, keeping the hand scanner for the annotations column only. Rejected: two scanning mechanisms in one file is the state this MR ends. The page-read hybrid described in
### Over the gate: a reviewer decisionis the same trade at a different boundary, and is open rather than rejected. - Chosen: scan into generated
model.ContainerManifests, convert incontainerManifestFromModel, the package's single NULL-normalization point.
Non-goals
- A
.Sql()unit test forlistContainerManifestsStmt. That builder shipped in the stacked base, which owns its test. - Config or behavior changes. Store method signatures and semantics are unchanged, callers untouched.
- Restoring a benchmark-regression CI gate. No Go benchmark is gated in this repo, so that is infrastructure work rather than a container_manifest concern.
- Narrowing the manifests-list projection off
annotations, tracked in Narrow the manifests-list projection behind a d... (#453 - closed) • Hayley Swimelar.
Related to #430 (closed)