fix: don't fail open when a subject's namespace code is undecodable - #1645
Conversation
…ecodable `generate_upsert_deletions` skips existing-value lookups for subjects that are absent from both the persisted dictionary and novelty. Proving absence requires resolving the subject's IRI, and the guard reported "present" whenever the pre-transaction snapshot could not decode its namespace code. That defeats the skip for every IRI shape that allocates a namespace per subject. Under `MostGranular`, `urn:...:<id>:r:<sig>` splits at the last `:`, so each such node mints its own prefix; those codes are not in the pre-transaction snapshot, so every one of their predicates issued a point query that could not build a bound-subject filter and degraded into a whole-novelty clone and sort. On a 547-transaction, 1.55M-flake persist that was 1,510 such queries per transaction and 10.9s of an 11s staging. - stage.rs: resolve the IRI through the snapshot, then fall back to the store's own namespace table, and report absent only when neither can decode the code -- a code neither knows cannot name a base-index row. Novelty presence still backstops, so a subject that exists only in unindexed commits is never skipped. - stage.rs: allow the skip before any index exists, where the absence of a range provider makes novelty authoritative on its own. - binary_scan.rs: apply the bound (s,p,o) match inside the overlay walk in `open_overlay_only_fallback` instead of cloning every overlay flake, sorting, and then retaining. Retraction resolution treats each (s,p,o,dt,m) fact independently, so the filter drops whole facts and can never separate an assertion from its retraction. 547 transactions: 823.1s reported in production, 32-76s here; median upsert 68ms and flat, with the rebuilt ledger at exactly 1,548,373 triples across 365,406 subjects. Docs note the data-modelling hazard, which also burns the u16 namespace code space.
zonotope
left a comment
There was a problem hiding this comment.
This looks good, but I flagged one test-related thing in line, and there is another thing I think we should follow up on. for_each_overlay_flake accepts first/rhs bound flakes (fluree-db-novelty/src/lib.rs:1438) and can seek, but the call still passes None, None. This commit removes the clone-and-sort, which is the big win, but the 1,510-probes-per-transaction shape still costs 1,510 × |novelty| comparisons. It's worth following up in another branch for novelty-only subjects, which the stage.rs skip doesn't cover.
| /// ones must still be skipped rather than dragging the whole transaction | ||
| /// through a per-(subject, predicate) scan. | ||
| #[tokio::test] | ||
| async fn upsert_indexed_replaces_values_for_per_subject_namespaces() { |
There was a problem hiding this comment.
This test passes on both old and new code — the old None => true was conservatively correct (extra queries, identical results), so nothing in the test distinguishes them. It guards the skip's correctness but not the skip, which is the entire point of the commit. The pattern_queries counter added alongside is exactly the right observable but is only logged; asserting on it (or skipped_subjects) would make the regression detectable.
The existing regression test passes against the old `decode_sid` fail-open: reporting "present" when the namespace code could not be decoded was conservatively correct, costing extra queries but producing identical retractions. A results-only assertion therefore cannot detect the skip silently switching off, which is the regression the change guards against. Assert on the counters `generate_upsert_deletions` already reports instead. Verified to fail against the pre-fix behaviour (skipped_subjects 0 vs 3) while the correctness test still passes.
Problem
Upsert against subjects with new IRIs was dramatically slower than against existing ones, and the cost grew with ledger size. A customer's 547-transaction / 1.55M-flake identity persist took 823.1s of transactor time in production, with 93 transactions at a median of 6.7s each.
generate_upsert_deletionsalready skips existing-value lookups for subjects absent from both the persisted dictionary and novelty (#8ae18197b,bb2a042f8) — and that skip works. Instrumenting it showed it firing for exactly 151 of 302 subjects per transaction. The other half went down the slow path.Root cause
Proving a subject absent requires resolving its IRI. The guard's last line reported "present" whenever the pre-transaction snapshot could not decode the subject's namespace code:
That defeats the skip for every IRI shape that allocates a namespace per subject. Under
MostGranular, an opaque IRI splits at its last:, sourn:…:evidence:<hex>:r:<sig>gives each revision node its own prefix — 151 new codes per transaction, none of them in the pre-transaction snapshot. Each of those subjects' predicates then issued a point query that could not build a bound-subject filter and degraded into cloning and sorting the entire novelty set.Measured per transaction:
subject_count=302 skipped_subjects=151 pattern_queries=1510, and those 1,510 queries were 10.9s of an 11s staging. Commit itself was 33ms; background indexing 137ms and fully overlapped. Replaying the same payloads throughinsertinstead ofupserttook 42ms each, flat.Changes
stage.rs— resolve the IRI through the snapshot, then fall back to the store's own namespace table; report absent only when neither can decode the code. A code neither knows cannot name a base-index row. Novelty presence still backstops, so a subject that exists only in unindexed commits is never wrongly skipped.stage.rs— allow the skip before any index exists, where the absence of a range provider makes novelty authoritative on its own. That case alone cost 2.6s/txn.binary_scan.rs— apply the bound(s,p,o)match inside the overlay walk inopen_overlay_only_fallbackinstead of cloning every overlay flake, sorting, and thenretaining. It was O(novelty) clones plus an O(n log n) sort per call. Safe becauseresolve_overlay_retractionsdecides each(s,p,o,dt,m)fact independently, so the filter drops whole facts and can never separate an assertion from its retraction.docs/design/namespace-allocation.md— the doc's namespace-explosion preflight covers bulk import only; ordinary transactions get no such detection. Notes the hazard, which also burns the u16 code space (~151 codes/txn ≈ 48k for this dataset against ~65.5k available).Measurements
Apple M4 Max, local file storage, default config, applied serially over HTTP to a fresh ledger.
Matched 20-upsert subset, identical payloads and machine:
…evidence:<hex>:r:<sig>…evidence:<hex>:r:<sig>The middle row is worth noting on its own: IRI shape alone was an 80× penalty on the unmodified binary. The customer has since normalized their IRIs, which independently confirmed the diagnosis in production — staging median 148ms → 22ms, p90 6,392ms → 447ms. On already-normalized data this branch is worth a further ~1.45× (34.9s → 24.1s), concentrated in four transactions where subjects exist as objects but not as subjects and still land in the overlay fallback. Its main value is removing the engine's sensitivity to IRI shape.
Testing
fluree-db-transact,fluree-db-query,fluree-db-ledger,fluree-db-noveltyandfluree-db-api.upsert_indexed_replaces_values_for_per_subject_namespacespins both directions: per-subject-namespace subjects that already exist still have their old values retracted (before and after a subsequent index build, so retractions are staged rather than masked by novelty), and brand-new ones are still skipped.Not addressed
--profile dev-fast, not--release; before/after ratios are from matched builds.(subject, predicate)at a time.batched_subject_probe_binary(join.rs:3208) is the primitive that would collapse them.for_each_overlay_flakesupports a bounded seek that no leaf caller uses. Tracked in perf(query): seek bounded ranges in overlay walks instead of scanning the whole graph's novelty #1648.DictTreeReader::reverse_lookupspending ~10% of CPU purely in clock reads (moka cache bookkeeping per leaf access). Separate issue, worth a look.