[slopfix] perf(universaldb): fold postgres drain_batch into claim+nextval and single apply CTE

[MasterPtato]

No description provided.

[MasterPtato]

Stack for rivet-dev/rivet

• #5552 [slopfix] fix(universaldb): match fdb conflict semantics in …
• #5551 [slopfix] fix(universaldb,util,pegboard-envoy): index dedup …
• #5398 feat(kitchen-sink): add churnDb actor and seed driver for co …
• #5397 chore: Clean up
• #5382 [slopfix] docs(self-hosting): correct postgres config format …
• #5356 [slopfix] fix(pegboard): decrement kv_storage_used metric on …
• #5354 [slopfix] refactor(universaldb,ups): single/multi-node udb o …
• #5353 [slopfix] perf(universaldb): fold postgres drain_batch into … 👈
• #5342 [SLOP(claude-opus-4-8-high)] perf(universaldb): batch leader …
• #5341 [slopfix] docs(self-hosting): promote postgres from experime …
• #5340 [slopfix] fix(self-host): repair dev compose generation, kit …
• #5337 [SLOP(claude-opus-4-8-high)] feat(universaldb): graceful pos …
• #5336 [slopfix] test(universaldb): postgres leader failover + abor …
• #5335 [SLOP(claude-opus-4-8-high)] feat(ups): table-backed postgre …
• #5329 [SLOP(claude-opus-4-8-high)] feat(universaldb): postgres lea …
• #5327 [SLOP(claude-opus-4-8)] feat(pegboard-envoy): rate-limit env …
• #5326 [SLOP(claude-opus-4-8)] feat(util): add rate limiter primiti …
• #5322 [SLOP(claude-opus-4-8)] feat(rivetkit-core): add serde durat …
• #5321 Add observation for serde_bare
• #5320 Add observation and ser/de metrics for serde calls
• #5314 [SLOP(claude-opus-4-8-medium)] chore: add profiling cargo pr …
• #5306 [SLOP(claude-opus-4-8)] fix(kitchen-sink): serve built front …
• main

Get stack: forklift get 5353
Push local edits: forklift submit
Merge when ready: forklift merge 5353

_{change pnzolyqx}

[claude]

Review

Perf-focused change to the postgres UDB resolver's drain_batch: folds the separate nextval version-allocation query into the claim query, merges the point-delete/upsert/status-stamp/watermark-advance into a single CTE statement, switches the in-process TransactionConflictTracker from a Vec to a BTreeMap<commit_version, _>, and adds a throwaway diagnostic benchmark. Overall the round-trip reduction (2 queries + 3 statements to 1 query + 2 statements per batch) is a solid, well-reasoned win, and the comments explaining why each restructuring is safe (CTE snapshot isolation, disjoint write sets, sort-order defensiveness) are genuinely good.

Good catch: remove() correctness fix

conflict_tracker.rs:125 - previously remove matched by start_version via linear scan, but start_version is not unique across concurrent transactions (two txns can legitimately share one). That could remove the wrong tracker entry or silently miss the intended one. Keying by commit_version (globally unique, from either the postgres sequence or the atomic counter) and updating the rocksdb caller (transaction_task.rs:430) to pass commit_version fixes a real latent bug, not just a refactor.

Worth checking: pruning assumes insertion order == commit_version order

conflict_tracker.rs:37-40,71-79 - the new front-truncation pruning relies on the comment's claim that "commit versions grow with commit time," i.e. that BTreeMap key order (commit_version) matches insertion order (insert_instant). For the rocksdb driver, commit_version = next_global_version() (transaction_task.rs:416) is fetched via an atomic fetch_add before the caller acquires the tracker's internal mutex to actually insert. Two racing callers can therefore acquire the lock and insert in an order that doesn't match their commit_version order (e.g. the task holding cv=100 gets descheduled between the fetch_add and the lock acquisition, so cv=101 is inserted first). When that happens, pruning-from-the-front can stop at a not-yet-expired low-key entry while a higher-key entry that was actually inserted earlier is still expired and unpruned.

This isn't a soundness bug for conflict detection itself (an extra retained entry only makes the window slightly more conservative, never lets a real conflict slip through), but it does mean entries can outlive TXN_CONFLICT_TTL by more than the assumed bound in rare races, and the doc comment states the invariant as fact rather than "true except for a narrow race window." Might be worth loosening the comment, or confirming this race is intentionally accepted as harmless.

Worth verifying: inline nextval under ORDER BY ... LIMIT ... FOR UPDATE SKIP LOCKED

resolver/mod.rs:363-369 - Postgres only avoids evaluating the full target list (including the nextval side effect) for every WHERE-matching row when the ORDER BY can be satisfied without a blocking Sort node (e.g. via an index scan). udb_commit_requests_pending is a partial index on (id) WHERE status = 'pending', which should cover the status = 'pending' predicate, but the query also filters on epoch = $1, applied as a non-indexed filter. It's very likely fine given the existing partial index was presumably built for this exact query shape, but given the whole point of this change is cutting a nextval-per-row round trip, it'd be worth confirming with EXPLAIN (ANALYZE, BUFFERS) under load that the plan doesn't fall back to a blocking Sort, which would burn through far more sequence values than batch_len per drain (harmless correctness-wise per the existing "harmlessly skipped losers" reasoning, but it would undercut the perf motivation and accelerate sequence advancement under sustained load).

Test coverage

The only new test, tests/kv_upsert_scaling.rs, is explicitly labeled a "Throwaway diagnostic" benchmark in its own doc comment, is #[ignore]d, and exercises none of the actually-changed logic. This PR touches genuinely tricky, correctness-sensitive code (conflict-window pruning semantics, versionstamp monotonicity under out-of-order nextval evaluation, and the new range-delete-then-CTE ordering for overlapping point writes), none of which get a dedicated regression test. The general integration.rs suite runs the postgres driver end-to-end so basic correctness is exercised incidentally, but a couple of targeted tests would be valuable:

• an upsert landing on a key inside a concurrently range-deleted region in the same batch (exercises the "range delete must run before the CTE" ordering this PR relies on)
• a conflict-tracker test asserting remove behaves correctly when two txns share a start_version (regression test for the bug fixed above)

Per this repo's convention that benchmark artifacts belong in ~/.agents/benchmarks/ rather than the checked-in test suite, it's also worth considering whether kv_upsert_scaling.rs should move out of tests/ now that its investigation is done, rather than shipping permanently as an ignored test.

Minor: permanent per-batch instrumentation

resolver/mod.rs:568-601 - drain_batch now takes 8 Instant::now() samples and logs 9 extra fields (plus a tracker.len() call that re-acquires the tracker mutex) on every batch at info level. The overhead itself is negligible next to the DB round trips, but it reads like debugging instrumentation from the investigation that produced this PR rather than a pruned-down set of fields meant to ship long-term. Worth deciding whether all of decode_us/conflict_us/resolve_us/read_us/fold_us/apply_us/commit_us/notify_us/tracker_len are meant to be permanent, or trimmed once the investigation this PR is diagnosing is resolved.

No security concerns - this is internal engine-trusted-boundary code per CLAUDE.md's trust model, and the changes don't touch any client- or envoy-facing input handling.