perf(posting): non-blocking UID cache warming, plus review follow-ups for #9809

[matthewmcneely]

Follow-ups to the review on dgraph-io#9809, based on your 2b990b48. Three commits, each one independent — take, change, or drop any of them.

Merging this into fix-calculated-uids-materialization folds it into dgraph-io#9809.

1. perf(posting): warm cached UID slices off the published list's write lock

This supersedes #1 — same shape you landed there, plus the reason I now think it should go in before dgraph-io#9809 merges rather than after.

calculateUids held the published list's write lock across a full walk, which for a multi-part list also reads every split from Badger (readListPart). The commit path wants that same lock:

commitOrAbort (worker/draft.go:966)
  -> txn.UpdateCachedKeys        (draft.go:1019)
    -> updateItemInCache
      -> List.setMutationAfterCommit -> l.Lock()
  -> posting.Oracle().ProcessDelta  (draft.go:1022)   <- releases waiting reads

commitOrAbort runs on processApplyCh, which is explicitly serial, and the ProcessDelta that releases waiting reads comes after UpdateCachedKeys. So one slow warm stalled every commit for the group rather than only the readers of that key.

The fix is what you wrote in your PR: a CAS elects one warmer, the walk runs on the copy readFromCache already makes, and the result is handed over under a short write lock that drops it if a commit landed meanwhile. Differences from yours:

• needsUidWarm() is checked on the published list under the read lock that readFromCache already holds. Your version checked lCopy.mutationMap.currentEntries == nil, but clone() never copies currentEntries (posting/list.go:133-148), so that term was always true and said nothing about the cached list. publishCalculatedUids rechecks it under the lock either way, so it was harmless — it just read as if it meant something.
• uidWarmState atomic.Int32 broke the alignment of the List field block, so gofmt -l flagged posting/list.go and trunk check would have failed. Realigned.
• The warm is factored out into warmCachedUids, and calculateUids carries a doc comment stating the ownership contract, so the walk doesn't drift back onto a shared list later.

Two related fixes on the same path, folded into this commit because they touch the same lines:

• A warm failure no longer fails the read. readFromCache propagated the error, so a transient Badger error reading a split part (or ErrTsTooOld out of iterate) turned a cache hit into a query error — something that path could not do before fix(query): avoid eager UID materialization on posting reads dgraph-io/dgraph#9809. Warming is an optimization, so it logs and serves the list unmaterialized. Same on the disk path, where the behavior predates this PR.
• The ristretto re-set no longer resurrects a dropped entry. ml.cache.set(key, cacheItem) re-inserted even if updateItemInCache called ml.del(key) during the warm (the rollup branch, p.Pack != nil). resetIfCurrent only re-sets while it is still the live entry. The minTs <= readTs <= maxTs guard kept this from serving a wrong snapshot, so it was hygiene rather than correctness, but the window was as long as a warm.

TestWarmCachedUidsWalksWithoutTheCachedListsWriteLock holds the published list's read lock across the walk, so it deadlocks against the old behavior — I confirmed it fails in 5s when warmCachedUids is pointed back at the shared list. TestReadUidsWarmsCachedPostingListConcurrently now asserts what actually has to hold with a non-blocking warm: every one of the 32 readers returns all 4096 uids whether or not it won the election, and the cached entry is warm at the end.

2. perf(posting): copy the negative-first tail instead of pinning the whole walk

Your negative-pagination fix is right, and it's what makes this reachable. A negative first has no early stop, so Uids() materializes the whole list and then takes the last N off the end — as a view, which pinned the full []uint64 for the lifetime of the response. 8MB retained to hand back ten uids on a million-uid list, in a PR about not materializing UID slices. Before 441d303 the opt.First != 0 stop check truncated the walk to one posting, so the pinned array was one element long.

3. fix(worker): route the uid_in read through uidReadFirst

worker/task.go:971 still built First from int(q.First + q.Offset), the int32 arithmetic uidReadFirst exists to replace; MaxInt32 + offset wraps negative. Latent today, since calculatePaginationParams forces offset to zero whenever first is the unbounded sentinel and the branch only tests whether the intersection came back non-empty — but the invariant belongs in one place. worker/match.go:76 and worker/trigram.go:29 pass int(q.First) with no offset, so they're unaffected.

Verification

go build ./..., gofmt clean, go vet with no new findings, go test ./posting/ -race green in full (176s), go test ./worker/ green.

One gap worth naming: the uid_in call-site change in commit 3 has no test of its own. TestUidReadFirst covers the arithmetic including the near-sentinel boundary, but the call site itself needs a live posting list.

Also still open from the review, and deliberately left alone here: deleting the cap() assertion rather than fixing it leaves the allocation-size bullet in the dgraph-io#9809 description untested. Your call whether that's worth a test.