feat(npm): the batched upstream rules read 2/2 (S31 plan: 4/19)

Dzmitry (Dima) Meshcharakourequested to merge
dmeshcharakou/s31-npm-virtual-step-4b into main

The second of Step 4's two fixed-count reads. Part 1 (!2164 (merged)) has merged, so this MR now targets main directly and is no longer gated behind anything. Neither read is wired yet, Step 11 being the first caller, so nothing here is reachable in production on merge.

One statement returns the rules of every named association, grouped by association id. It keys on the ids part 1's list read already returned, so it re-joins neither the association table nor repositories: membership and liveness were gated there, and re-deriving either would only add partition metadata to every execution.

The stack

# MR Branch Contents LOC State
1/2 !2164 (merged) dmeshcharakou/s31-npm-virtual-step-4a the ordered upstream list read 1221 merged
2/2 this MR dmeshcharakou/s31-npm-virtual-step-4b the batched rules read, plus the assertion that needs both reads 873 open

Both figures are re-derived at this MR's head against each MR's own merge base, not carried over from an earlier revision.

The namespace equality is required, not decorative

index_nvur_on_ns_id_repository_upstream_id leads on namespace_id, so without that equality the predicate matches no usable index prefix, gets no hash pruning, and the read becomes an Append over all 64 partitions, growing with the table rather than the rule set. Measured on PostgreSQL 16.15 over 200k rows in 20k namespaces: 0.039 ms and 3 buffers with the equality, 11.5 ms and 2303 buffers without. TestNpmVirtualUpstreamRules_PlanShape gates it with EXPLAIN.

Absence, emptiness and the zero id

An association holding no rules has no entry, so "no rules" and "rules this read dropped" stay apart. An empty id list issues no statement at all, and that check sits ahead of the client guard so a zero-value store answers an empty batch with an empty map rather than an error: with no association to ask about there is no round trip to refuse.

A zero id anywhere in the batch is rejected. It binds like any other id and matches no row, so it would report a position the caller failed to resolve as one holding no rules, leaving it eligible for everything its rules denied.

That guard closes one instance of a class it cannot close in general: any id matching no row reads the same way, whether it is stale, from another repository, or an association deleted after the list read returned it. The zero UUID is rejected because it is the wiring slip a type cannot express; the rest is the composed read's to own, since separating them means re-joining the association table on every request. The sidecar names all three readings and says which fail open, per A Documented Absence Names Every Reading It Leaves Open.

The sidecar also now says what the composed read can and cannot do about the third reading. Taking the ids from its own list read in the same request narrows that window; it does not close it, because both methods query s.client.DB() and neither takes a qrm.DB, so the two statements always run as separate autocommit READ COMMITTED statements. Closing it outright would need both reads under one snapshot. The residual window is recorded as accepted.

Neither discriminator is filtered

Both are projected verbatim, out-of-range values included. The column's range CHECK is the first line of defense and the matcher the second, so a read that dropped an unrecognized value would turn a corrupt deny rule into a no-op. All six rule_type x target_field combinations get a positive hit, each with its own pattern so a swapped projection fails.

The bound fails closed

Nothing caps rules per association, so this read's row count is not bounded by the upstream count. It selects one row past its ceiling and returns a data error when that row arrives, rather than serving a shortened set: a short rule set is indistinguishable from a permissive one, and the rules past the bound may all be denials.

No caller exists yet, so nothing routes this error anywhere. Which status a refusal becomes is the resolution facade's to decide when it lands at Step 11; until then the returned error is the only signal and nothing reads it. An earlier revision of this description asserted the error already answers 500 through a caller's data-error arm, which was not true on either side of that MR and is corrected here.

Both sides of the off-by-one are covered: at the ceiling every row comes back, one past it is refused whole and returns no rules at all.

The error names no identifier

The wrap names the operation, the table and the association count, and nothing else, per Identifier-free error strings in docs/dev/database-query-patterns.md ("the rule attaches when a method is added"). A handler logs a wrapped error as one error_message field that operators group and alert on, so a per-tenant value there would give every refusal its own group. The association count stays because a magnitude is not an identifier. namespace_id is now the caller's to log as its own structured field, which the exported error's doc comment says and the sidecar explains. The sibling NpmVirtualRemoteLookupStore.MetadataByPositions follows the same rule and cites it the same way.

The refusal is also invisible in metrics and traces on its own: instrumentQuery has no outcome dimension and the error is raised after the statement succeeded, so the span is marked OK. That makes the caller's log line the only place the two identifiers can appear.

What the ceiling is sized on

The ceiling is sized off ADR-004's 20-upstream cap and not off part 1's list bound, and it bounds the sum of rules rather than a per-association share. One association holding 20,001 rules therefore crosses it from a perfectly legal one-position list. Making the bound unreachable from every legal configuration needs two write-time caps rather than one: ADR-004's 20 upstreams, and a cap on rules per association. Neither exists yet, and docs/specs/S31-npm-virtual.md leaves the second an open question routed to the management API's rule-CRUD surface, so this MR neither adds it nor changes the ceiling.

maxRulesPerAssociation is a multiplicand and not an enforced per-association limit, which its own comment and the sidecar both say. The name overstates what the code does, and it is kept because the merged plan fixes it at that spelling; a rename belongs with the write-time cap that would make it real.

The bound rests on an absent struct tag

npmVirtualUpstreamRuleRow carries no sql:"primary_key" tag, so go-jet keys each row on its own row number and len(rows) is the true returned count the bound is checked against. Tagging the association id, which is the obvious move for anyone attacking the allocation cost the sidecar quantifies, would instead collapse every row of one association into a single struct: len(rows) would become the number of distinct associations, the ceiling could never be reached, and each association would come back holding one rule. The absence is load-bearing rather than an oversight, and the sidecar now says so, since nothing about a missing tag looks deliberate. The bound's own test is what fails if it is ever added.

One deviation from the spec's notation

The spec writes the predicate as WHERE namespace_id = $1 AND npm_virtual_repository_upstream_id = ANY(...). go-jet's builder has no array-bind form, so this renders IN ($1, $2, ...): 40 positions bind 42 parameters instead of 3. The two are the same predicate, and PostgreSQL folds an IN-list of same-typed constants into the one ScalarArrayOpExpr that = ANY produces, so the plan and the partition pruning are identical. The deployment also runs pgx.QueryExecModeSimpleProtocol, so nothing is server-side prepared and the 65535-parameter ceiling does not apply. Dropping to raw SQL to match the notation would forfeit the EXPLAIN plan-shape test, which builds from the statement builder. Named here rather than left for a reader to notice.

The assertion that needs both reads, and the two plan-shape tests

Per-read statement count is independent of upstream count is the one assertion that exercises both statements and so cannot live in part 1: seeded lists of 2 and 20 upstreams produce an identical count.

Plan shape is asserted per read, in two tests rather than one over both. An earlier revision added a second EXPLAIN of the list read alongside the rules one, which would have planned the list read twice under two test names once part 1 merged. Instead:

  • TestNpmVirtualUpstreamList_PlanShape is part 1's, and gains the exact-set assertion this MR was carrying, so a fifth joined table fails rather than passing quietly. Nothing before it caught a table being added.
  • TestNpmVirtualUpstreamRules_PlanShape is new and covers the rules read only. It pins single-partition pruning, pins that the read joins nothing, then runs EXPLAIN (ANALYZE, BUFFERS) with enable_seqscan off and requires an Index Scan on a child of index_nvur_on_ns_id_repository_upstream_id with namespace_id bound as an index qual. Pruning alone also passes a plan that pruned to one partition and then read all of it, and the 3-buffers-against-2303 measurement is an index claim. The association column is deliberately not required in the Index Cond: the planner moves it between Index Cond and Filter on row counts, so requiring it would assert a planner choice the fixture does not control.

Acceptance criteria

The spec's ### Resolution list has been renumbered since the plan was written. The plan's Step 4 acceptance cites #14 (soft delete), #15 (query count) and #16 (closed) (health); those are now #15, #16 (closed) and #17 (closed). This table uses the spec's current numbering. Stated rather than silently corrected, because a step MR must not edit the plan.

Spec AC (current numbering) Covered here Test
#16 (closed) per-read query count independent of upstream count store half, both reads ..._StatementCountIsIndependentOfUpstreamCount
#15 a soft-deleted upstream is skipped part 1 —
#17 (closed) the health status costs no statement not this step's. Asserted at Step 11 over a composed resolution, for the reason the plan gives: a store-level statement count passes while the composed read fails —

Separately, docs/specs/S31-npm-virtual.md line 257 says the per-read cost is "one query per upstream association", and the section it cites specifies the opposite, "one query for the rules of every association in that list", which is what this MR implements and what AC #16 (closed) asserts. The implementation follows the design section; the one-line summary is the defect and belongs in its own docs(specs) MR. Named here so a reader landing on that line first does not conclude the code drifted.

Files-list deviation from the plan

Two departures, both of which part 1 also records: npm_virtual_resolution_test.go (implied by the Step 4 Tests list, absent from its Files list) and the npm_virtual_resolution_contract.md sidecar.

internal/metrics/cardinality.go was a third departure in an earlier revision of this MR and is no longer touched. That line used to inline a query-name count; part 1 removed the count on the ground that nothing enforces it and every merge adding a name rots it, and re-landing a bumped figure would have reinstated exactly that. The catalog has already moved from 481 to 482 since, which is the argument.

Size

873 reviewable LOC (843 added, 30 removed). By file group, on that same added-plus-removed metric: 139 source (npm_virtual_resolution.go 136, query_names.go 3), 589 test (integration 398, unit 191), 145 sidecar doc.

Not split further. This is already 2/2 of the split the plan mandates, 589 of the 873 lines are tests, and the remaining source is one store method plus its row struct and two constants. The statement-count assertion cannot move to part 1 because it needs both reads to exist, and moving the sidecar out would separate the bound from the argument for its size. What is left after the tests and the doc is 139 lines.

The 30 deleted lines, by file rather than as one category:

File Deleted What they are
npm_virtual_resolution.go 5 the file header (2), the maxNpmVirtualUpstreams comment (2) and one line of the store's doc comment, all rewritten to cover both reads
npm_virtual_resolution_test.go 15 the file header (2), the realigned var block (2), two doc comments (4), and 7 lines of ..._ZeroValue restructured into subtests with both original assertions kept verbatim
npm_virtual_resolution_integration_test.go 6 the file header (2) and the four assertSinglePartition calls in part 1's plan-shape test, replaced by a loop over the same set plus the exact-set assertion
npm_virtual_resolution_contract.md 4 the H1, the intro's two-subject sentence, and the old section title

Most of it is churn from the split, but not all of it, so it is enumerated rather than waved past.

e2e scenarios

None. The plan assigns the whole e2e catalog to Step 19 and states that every other step's e2e statement is "none, because ...". This MR adds one datastore read with no caller and no route, so no docs/testing/ scenario is added or affected.

Review fixes in this revision

Rebased onto main after part 1 merged, resolving four conflicting files by hand rather than by hunk. The two commits are squashed into one, because the feat commit's body claimed the error carries a row count that the follow-up had already removed, and this MR has squash off, so both bodies land verbatim.

Beyond the conflicts: the error's namespace=%s is gone (identifier-free rule); the RulesByUpstreams godoc now points at the sidecar's three readings instead of naming one; the sidecar says "narrows" rather than "closes" for the delete-between-reads window, names the absent primary_key tag as load-bearing, gives docs/specs/S31-npm-virtual.md by path, and drops a "needs a plan amendment" clause that would have gone stale; and the duplicate list-read EXPLAIN is gone.

Database Review Evidence

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17 container (matching GL_PG_CURR_VERSION from .gitlab-ci-other-versions.yml; server reported 17.11), with synthesized seed data rolled back per query and the container torn down at the end of the run. Numbers reflect moderate cardinality and do not capture production-scale effects. See Database review evidence for seed sizing, methodology, and the anomalies the skill flags. Expand each row's details for the seed shape, rendered SQL, bound args, and raw plan.

Only RulesByUpstreams is covered. UpstreamsByVirtualRepository also dispatches a statement, but npmVirtualUpstreamListStmt's chain is byte-identical to the merge base, so this revision renders no new SQL for it.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.RulesByUpstreams.OneAssociation Limit -> Index Scan index_nvur_on_ns_id_repository_upstream_id 25 / 25 45.53 0.040ms 27 / 0 1/64
datastore.RulesByUpstreams.TwentyAssociations Limit -> Bitmap Heap Scan index_nvur_on_ns_id_repository_upstream_id 500 / 500 182.51 0.760ms 53 / 0 1/64

No anomalies. In particular the two the plan names as this step's risk, full partition fan-out and partial partition fan-out, did not fire: the namespace_id equality the statement carries alongside the association IN (...) prunes to a single partition in every run below, including the ceiling probe. Reported index names are the declared parent index; the plans below name the partition-local child (npm_virtual_upstream_rules_p0_namespace_id_npm_virtual_rep_idx7).

datastore.RulesByUpstreams.OneAssociation

Summary: Plan matches the method's intent. The planner takes an Index Scan on index_nvur_on_ns_id_repository_upstream_id, whose (namespace_id, npm_virtual_repository_upstream_id) leading columns are exactly the two equalities the statement carries, and the namespace_id literal prunes to 1 of 64 partitions. Estimate matches reality (25 / 25), all 27 buffers are hits, and execution is 0.040ms. No anomalies.

Seed shape: namespaces=1, repositories=210, npm_virtual_repositories=10, npm_virtual_repository_upstreams=200, npm_virtual_upstream_rules=5000

Rendered SQL:

SELECT npm_virtual_upstream_rules.npm_virtual_repository_upstream_id AS "npm_virtual_upstream_rules.npm_virtual_repository_upstream_id",
     npm_virtual_upstream_rules.rule_type AS "npm_virtual_upstream_rules.rule_type",
     npm_virtual_upstream_rules.target_field AS "npm_virtual_upstream_rules.target_field",
     npm_virtual_upstream_rules.pattern AS "npm_virtual_upstream_rules.pattern"
FROM public.npm_virtual_upstream_rules
WHERE (npm_virtual_upstream_rules.namespace_id = $1::uuid) AND (npm_virtual_upstream_rules.npm_virtual_repository_upstream_id IN ($2::uuid))
LIMIT $3;

Bound args: [11111111-1111-4111-8111-111111111111, 22222222-2222-4222-8222-222222220001, 20001]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=0.28..45.53 rows=25 width=47) (actual time=0.009..0.031 rows=25 loops=1)
   Buffers: shared hit=27
   ->  Index Scan using npm_virtual_upstream_rules_p0_namespace_id_npm_virtual_rep_idx7 on npm_virtual_upstream_rules_p07 npm_virtual_upstream_rules  (cost=0.28..45.53 rows=25 width=47) (actual time=0.009..0.030 rows=25 loops=1)
         Index Cond: ((namespace_id = '11111111-1111-4111-8111-111111111111'::uuid) AND (npm_virtual_repository_upstream_id = '22222222-2222-4222-8222-222222220001'::uuid))
         Buffers: shared hit=27
 Planning:
   Buffers: shared hit=14
 Planning Time: 0.134 ms
 Execution Time: 0.040 ms

Timings: planning 0.134ms, execution 0.040ms, total 0.174ms.

datastore.RulesByUpstreams.TwentyAssociations

Summary: The batch at ADR-004's 20-upstream cap keeps the same index and the same single partition; the planner switches from an Index Scan to a Bitmap Heap Scan, which is the expected shape once a 20-element array probe makes heap access worth batching. Estimate matches reality (500 / 500), all 53 buffers are hits, and execution is 0.760ms. No anomalies; the partition key stays bound, so there is no fan-out at 20 associations any more than at one.

Seed shape: namespaces=1, repositories=210, npm_virtual_repositories=10, npm_virtual_repository_upstreams=200, npm_virtual_upstream_rules=5000

Rendered SQL:

SELECT npm_virtual_upstream_rules.npm_virtual_repository_upstream_id AS "npm_virtual_upstream_rules.npm_virtual_repository_upstream_id",
     npm_virtual_upstream_rules.rule_type AS "npm_virtual_upstream_rules.rule_type",
     npm_virtual_upstream_rules.target_field AS "npm_virtual_upstream_rules.target_field",
     npm_virtual_upstream_rules.pattern AS "npm_virtual_upstream_rules.pattern"
FROM public.npm_virtual_upstream_rules
WHERE (npm_virtual_upstream_rules.namespace_id = $1::uuid) AND (npm_virtual_upstream_rules.npm_virtual_repository_upstream_id IN ($2::uuid, $3::uuid, $4::uuid, $5::uuid, $6::uuid, $7::uuid, $8::uuid, $9::uuid, $10::uuid, $11::uuid, $12::uuid, $13::uuid, $14::uuid, $15::uuid, $16::uuid, $17::uuid, $18::uuid, $19::uuid, $20::uuid, $21::uuid))
LIMIT $22;

Bound args: [11111111-1111-4111-8111-111111111111, 22222222-2222-4222-8222-222222220001, 22222222-2222-4222-8222-222222220002, 22222222-2222-4222-8222-222222220003, 22222222-2222-4222-8222-222222220004, 22222222-2222-4222-8222-222222220005, 22222222-2222-4222-8222-222222220006, 22222222-2222-4222-8222-222222220007, 22222222-2222-4222-8222-222222220008, 22222222-2222-4222-8222-222222220009, 22222222-2222-4222-8222-222222220010, 22222222-2222-4222-8222-222222220011, 22222222-2222-4222-8222-222222220012, 22222222-2222-4222-8222-222222220013, 22222222-2222-4222-8222-222222220014, 22222222-2222-4222-8222-222222220015, 22222222-2222-4222-8222-222222220016, 22222222-2222-4222-8222-222222220017, 22222222-2222-4222-8222-222222220018, 22222222-2222-4222-8222-222222220019, 22222222-2222-4222-8222-222222220020, 20001]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=39.76..182.51 rows=500 width=47) (actual time=0.666..0.734 rows=500 loops=1)
   Buffers: shared hit=53
   ->  Bitmap Heap Scan on npm_virtual_upstream_rules_p07 npm_virtual_upstream_rules  (cost=39.76..182.51 rows=500 width=47) (actual time=0.666..0.713 rows=500 loops=1)
         Recheck Cond: ((namespace_id = '11111111-1111-4111-8111-111111111111'::uuid) AND (npm_virtual_repository_upstream_id = ANY ('{22222222-2222-4222-8222-222222220001,22222222-2222-4222-8222-222222220002,22222222-2222-4222-8222-222222220003,22222222-2222-4222-8222-222222220004,22222222-2222-4222-8222-222222220005,22222222-2222-4222-8222-222222220006,22222222-2222-4222-8222-222222220007,22222222-2222-4222-8222-222222220008,22222222-2222-4222-8222-222222220009,22222222-2222-4222-8222-222222220010,22222222-2222-4222-8222-222222220011,22222222-2222-4222-8222-222222220012,22222222-2222-4222-8222-222222220013,22222222-2222-4222-8222-222222220014,22222222-2222-4222-8222-222222220015,22222222-2222-4222-8222-222222220016,22222222-2222-4222-8222-222222220017,22222222-2222-4222-8222-222222220018,22222222-2222-4222-8222-222222220019,22222222-2222-4222-8222-222222220020}'::uuid[])))
         Heap Blocks: exact=50
         Buffers: shared hit=53
         ->  Bitmap Index Scan on npm_virtual_upstream_rules_p0_namespace_id_npm_virtual_rep_idx7  (cost=0.00..39.58 rows=500 width=0) (actual time=0.023..0.023 rows=1000 loops=1)
               Index Cond: ((namespace_id = '11111111-1111-4111-8111-111111111111'::uuid) AND (npm_virtual_repository_upstream_id = ANY ('{22222222-2222-4222-8222-222222220001,22222222-2222-4222-8222-222222220002,22222222-2222-4222-8222-222222220003,22222222-2222-4222-8222-222222220004,22222222-2222-4222-8222-222222220005,22222222-2222-4222-8222-222222220006,22222222-2222-4222-8222-222222220007,22222222-2222-4222-8222-222222220008,22222222-2222-4222-8222-222222220009,22222222-2222-4222-8222-222222220010,22222222-2222-4222-8222-222222220011,22222222-2222-4222-8222-222222220012,22222222-2222-4222-8222-222222220013,22222222-2222-4222-8222-222222220014,22222222-2222-4222-8222-222222220015,22222222-2222-4222-8222-222222220016,22222222-2222-4222-8222-222222220017,22222222-2222-4222-8222-222222220018,22222222-2222-4222-8222-222222220019,22222222-2222-4222-8222-222222220020}'::uuid[])))
               Buffers: shared hit=3
 Planning:
   Buffers: shared hit=23
 Planning Time: 0.248 ms
 Execution Time: 0.760 ms

Timings: planning 0.248ms, execution 0.760ms, total 1.008ms.

Supplementary: the rules ceiling. npm_virtual_resolution_contract.md states under ## Per-request cost of the rules ceiling that crossing the bound pays the full scan and then discards it. A separate run seeds 1001 rules on each of one virtual repository's 20 associations (20,020 rows) plus 25 on each of the other 180, for a 24,520-row partition, and asks for the same 20 associations. The read returns LIMIT-many rows (20,001) and the planner switches to a Seq Scan of the one partition, still pruned 1/64, at 2.781ms and 268 buffer hits with no reads. That is the database-side half of the sidecar's cost claim; the Go-side allocation cost it also describes is not measured here.

 Limit  (cost=0.05..755.43 rows=20001 width=46) (actual time=0.005..2.367 rows=20001 loops=1)
   Buffers: shared hit=268
   ->  Seq Scan on npm_virtual_upstream_rules_p07 npm_virtual_upstream_rules  (cost=0.05..756.15 rows=20020 width=46) (actual time=0.004..1.553 rows=20001 loops=1)
         Filter: ((namespace_id = '11111111-1111-4111-8111-111111111111'::uuid) AND (npm_virtual_repository_upstream_id = ANY ('{22222222-2222-4222-8222-222222220001,22222222-2222-4222-8222-222222220002,22222222-2222-4222-8222-222222220003,22222222-2222-4222-8222-222222220004,22222222-2222-4222-8222-222222220005,22222222-2222-4222-8222-222222220006,22222222-2222-4222-8222-222222220007,22222222-2222-4222-8222-222222220008,22222222-2222-4222-8222-222222220009,22222222-2222-4222-8222-222222220010,22222222-2222-4222-8222-222222220011,22222222-2222-4222-8222-222222220012,22222222-2222-4222-8222-222222220013,22222222-2222-4222-8222-222222220014,22222222-2222-4222-8222-222222220015,22222222-2222-4222-8222-222222220016,22222222-2222-4222-8222-222222220017,22222222-2222-4222-8222-222222220018,22222222-2222-4222-8222-222222220019,22222222-2222-4222-8222-222222220020}'::uuid[])))
         Buffers: shared hit=268
 Planning:
   Buffers: shared hit=26
 Planning Time: 0.218 ms
 Execution Time: 2.781 ms

Related to #885 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports