fix(query): avoid eager UID materialization on posting reads by gooohgb · Pull Request #9809 · dgraph-io/dgraph

gooohgb · 2026-08-06T04:54:16Z

Description

This PR avoids eagerly materializing the full UID slice for posting lists on read paths where that work is not useful or actively bypasses existing optimizations.

Changes:

Gate calculateUids() behind the posting-list cache being enabled, so --cache percentage=0,... does not build and immediately discard a full []uint64.
Avoid using calculatedUids for bounded Uids() reads with First or Intersect, preserving early-stop and compressed-intersection paths.
Reduce Uids() allocation size for bounded and small-intersect reads.
Avoid GetUids() in worker paths that do not consume a full UID list, including count, scalar comparison, has, uid_in, facets, pagination, and intersect paths.

Checklist

The PR title follows the
Conventional Commits syntax, leading
with fix:, feat:, chore:, ci:, etc.
Code compiles correctly and linting (via trunk) passes locally
Tests added for new functionality, or regression tests for bug fixes added as applicable

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

gooohgb · 2026-08-10T02:49:58Z

Hi, @matthewmcneely , could you please help review this PR? It’s affecting our production memory monitoring metrics and causing alerts. We’d really appreciate it if this could be reviewed and fixed as soon as possible. Thanks!

gooohgb · 2026-08-11T05:21:29Z

Hi @matthewmcneely , thank you for the review. I’ve updated the implementation based on your feedback. Could you please take another look when you have a chance?

gooohgb · 2026-08-12T05:33:53Z

Hi @matthewmcneely, thank you again for the detailed review.

I have updated the implementation based on your feedback. While validating the changes, I also found and fixed a regression affecting negative pagination (first: -N).

The CI checks are currently stuck. Could you please take another look at the updated implementation when you have a chance? Thank you!

gooohgb · 2026-08-17T01:00:51Z

HI, @matthewmcneely , any updates here?

gooohgb · 2026-08-28T09:40:43Z

Hi @matthewmcneely, just following up on #9809. Have you had a chance to review the latest changes and the incremental non-blocking cache-warming follow-up?

Please let me know whether you would prefer to merge the current PR first and handle the follow-up separately, or include it before merging. I’m happy to make any additional changes if needed.

Thanks again for your time and review!

…lock calculateUids held the published list's write lock across a full walk of the list, which for a multi-part list also reads every split from Badger. The commit path wants that same lock: UpdateCachedKeys -> updateItemInCache -> setMutationAfterCommit, called from commitOrAbort on the serial Raft apply loop, ahead of the ProcessDelta that releases waiting reads. One slow warm therefore stalled every commit for the group rather than only the readers of that key. Warm a private copy instead. A CAS elects one warmer per list, the walk runs on the copy readFromCache already makes, and the result is handed to the published list under a short write lock that drops it if a commit landed meanwhile. A reader that loses the election serves its read unwarmed, which is what every reader did before the optimization existed. Two related fixes on the same path: - A warm failure no longer fails the read. Warming is an optimization, so a transient Badger error reading a split part now logs and serves the list unmaterialized instead of turning a cache hit into a query error. - The re-set that refreshes the ristretto cost skips an entry that was evicted or dropped by a rollup during the warm, rather than resurrecting it. TestWarmCachedUidsWalksWithoutTheCachedListsWriteLock deadlocks against the previous behavior, so it fails when the walk is moved back onto the published list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ole walk A negative first has no early stop, so Uids() materializes the entire list before taking the last N off the end. Returning that as a view pinned the full []uint64 for the lifetime of the response: 8MB retained to hand back ten uids on a million-uid list. The retention only became reachable with the negative-pagination fix in 441d303. Before it, the opt.First != 0 stop check truncated the walk to a single posting, so the pinned array was one element long. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The uid_in branch still built ListOptions.First from int(q.First + q.Offset), the int32 arithmetic uidReadFirst was added to replace; MaxInt32 + offset wraps negative there. It is latent today, because 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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

matthewmcneely · 2026-09-01T18:32:24Z

Thanks for the patience, and for the turnaround on both earlier rounds — everything from them is addressed, and the concurrent test has real teeth.

On your merge-order question: I'd fold the non-blocking warm in before merging. When I went looking for how bad the convoy actually gets, it turned out to reach further than the readers of one key. calculateUids holds the published list's write lock across the walk, and the commit path takes that same lock on the serial Raft apply loop, one step ahead of the ProcessDelta that releases waiting reads:

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

So a slow warm — a large split list is the bad case, since iterate reads every part from Badger under that lock — stalls commits for the whole group, not only the queries touching that predicate. That moved it from "worth doing next" to "worth doing first" for me.

Rather than leave you to do it a third time, I've built on your gooohgb#1 and opened it against your branch: gooohgb#2. Merging it into fix-calculated-uids-materialization folds it into this PR. It's three independent commits — take, change, or drop any of them:

Non-blocking warm. Your design from Product Roadmap #1 (CAS-elected warmer, walk on the private copy, publish under a short write lock with a committedUidsTime recheck), with three changes: needsUidWarm() is checked on the published list under the read lock readFromCache already holds, because clone() never copies currentEntries (posting/list.go:133-148) so your lCopy check was always true; uidWarmState is realigned, since as written it made gofmt -l flag posting/list.go and trunk check would have failed; and the walk is factored into warmCachedUids with the ownership contract written on calculateUids, so it doesn't drift back onto a shared list later. Folded into the same commit: a warm failure now logs and serves the list unmaterialized instead of turning a cache hit into a query error, and the ristretto re-set skips an entry that a rollup dropped mid-warm rather than resurrecting it.
Negative-first tail copy. Your pagination fix is what makes this reachable — the walk now materializes the whole list, and returning the last N as a view pinned all of it. 8MB retained for ten uids on a million-uid list.
uid_in call site. worker/task.go:971 still builds First from int(q.First + q.Offset), the arithmetic uidReadFirst replaces. Latent today, but the invariant should live in one place.

The new lock test deadlocks against the current behavior — it fails in 5s if warmCachedUids is pointed back at the shared list — so it holds the property rather than just describing it. go test ./posting/ -race is green in full, as is ./worker/, and gofmt/go vet are clean.

Have a look when you get a chance. If you'd rather keep #9809 to the change it set out to make, I'm fine taking any of the three separately instead — say which and I'll move them out.

gooohgb · 2026-09-02T01:24:45Z

Hi @matthewmcneely, the changes look great. I’ve merged them into the PR branch. Thank you for your work on this!

Uids() returned a different uid set for a negative First depending on whether calculatedUids happened to be materialized. The memoized branch returned early and skipped the trim at the bottom of the function, so a warm read handed back the whole list where a cold one handed back the last N: first=-2 after=0 isect=nil cold=[8 10] warm=[2 4 6 8 10] first=-2 after=4 isect=nil cold=[8 10] warm=[6 8 10] The warm answer is the correct one, so the trim goes rather than being extended to both paths. Trimming here is unsound for an index read, because the worker post-filters the list afterwards, in helpProcessTask, after handleUidPostings has already returned: handleCompareFunction re-checks the real values when the tokenizer is lossy, and filterGeoFunction re-checks the real geometry. A uid this trim drops is one that filter never sees, and x.PageRange cannot put it back. name: string @index(term) . 0x1,0x2 name "great" 0x3,0x4,0x5 name "great wall" q(func: eq(name, "great"), first: -2) The term index is lossy, so the bucket for "great" is {1,2,3,4,5} and handleCompareFunction is what narrows it to the two exact matches. Trimmed to the last two first, the bucket is {4,5}, the filter drops both, and the query answers nothing. This is what a cold read does today; it is what both reads would do if the paths were aligned the other way. Nothing depended on the trim. Uids has eight production callers, only three can carry a nonzero First, and every one of them sits behind a pagination pass: calculatePaginationParams pushes a count down only when Params.Count != 0, and applyPagination no-ops only when Count == 0 && Offset == 0, so a pushdown always has an x.PageRange behind it. The one caller that does rely on worker-side truncation is `has` at root, which never reaches Uids because checkRoot leaves it with n == 0. The early stop for a positive first stays. It is paired with a real saving -- the read stops rather than materializing the rest -- and its own soundness rests on calculatePaginationParams keeping the functions that read a list per token and intersect them off the pushdown entirely. That was untested, so TestPaginationPushdownExcludesIntersectingFunctions now pins it. Removing the trim also removes a panic: a First of math.MinInt sliced out of range, because negating it wraps back to itself and the length guard then passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

opt.First is normalized to math.MaxInt32 at the top of Uids and never assigned again, so the `|| opt.First == 0` bail-out cannot fire, and the `&& applyIntersectWith` in the line below it is already known true. Both read as though a zero First takes some other route through the tail, which it does not. Rename applyIntersectWith to postProcess while renaming is cheap: it gates the truncation as well as the intersect, so the name was already wrong. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A failed warm was logged at warning level and then left the list open to being warmed again. Every reader that wins the election on that key repeats the whole walk, and for a multi-part list that means a Badger read per split, so both the wasted work and the log line recur at read QPS. The log rate was the symptom; the repeated walk is the cost. Park the list instead. uidWarmState gains a third state, and a walk that fails leaves it there so no later reader retries. setMutationAfterCommit lifts it again, which matters because remove-on-update defaults to false: an ordinary commit applies in place on the published list rather than replacing the entry, so without that the first failure would stick for the life of the entry. What is left is one attempt, and one log line, per commit to the key -- the same bound doRollup gets from its own per-key dedupe. finishUidWarm now compares and swaps rather than storing, so it is safe to defer alongside abandonUidWarm on the failing path. The warning on the disk path keeps firing unconditionally. It runs per cache miss rather than per read, and it is where an operator should first see that a list has stopped being readable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

uidReadFirst pushes down first + offset, but offset is parsed straight out of the query with no lower bound (params.fill in query/query.go), so a negative one subtracts from the read. `first: 10, offset: -1` pushes down 9 and the query comes back one uid short. A large enough one drops the bound altogether: `first: 10, offset: -100` pushes down -90, and a negative first reads the whole list. Clamp it, which is what x.PageRange itself does with a negative offset when it paginates the result, so the pushdown and the pass behind it now agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

require.Equal on two *uint64 falls through to reflect.DeepEqual, which follows both pointers and compares the uids they address. The assertion passed for two distinct arrays that happened to start with the same uid, which is exactly the case it was written to rule out: it was meant to show that publishCalculatedUids hands the slice over rather than recomputing it. require.Same compares the pointers. Verified by making publishCalculatedUids copy the slice -- require.Equal still passed, require.Same fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The comment on warmCachedUids said iterating the private copy is safe "because setMutationAfterCommit replaces those maps instead of writing into them". That is a property of one call site, not of the function: only the refresh=true path rebuilds committedEntries and committedUids before writing, and refresh=false writes both in place. Production only reaches a cached list through updateItemInCache, which passes true, so the code is correct -- but the comment as written would tell whoever adds the next caller that a refresh=false commit on a published list is fine, and it is not. That one is a fatal concurrent map access, not a race that might go unnoticed. Also softened the justification for swallowing a warm error. Warming must not be the thing that fails a read, but the read often fails anyway on the same unreadable split once it walks the list itself, so claiming the cache "can otherwise serve" it overstated the case. Comments only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

matthewmcneely · 2026-09-02T19:32:51Z

Thanks for merging those. I went back over the current state properly and found six things, so there's another PR against your branch: gooohgb#3. Merging it into fix-calculated-uids-materialization folds it into this PR. Three of the six are cleanup after my own last round.

The one worth your attention is that my first finding was pointing the wrong way, and I only caught it by trying hard to disprove it.

I'd reported that the memoized read path should trim a negative first the way the walk path does, so the two agree. Aligning them that way would have broken a query that works today:

name: string @index(term) .
0x1,0x2 name "great"    0x3,0x4,0x5 name "great wall"

q(func: eq(name, "great"), first: -2)

The term index is lossy, so the bucket for "great" is {1,2,3,4,5}, and handleCompareFunction is what narrows it to the two real matches — and it runs in helpProcessTask after handleUidPostings has returned. Trim the bucket to {4,5} first and the filter drops both, so the query answers nothing. x.PageRange behind it can't put back what the worker already discarded. allof is the same shape through intersection instead of post-filtering, since it isn't in calculatePaginationParams' exclusion list and needsIntersect matches it.

So the warm answer was the correct one all along. The trim comes out instead of being extended, which also fixes the cold path — wrong since 441d3033 — and makes warm and cold agree everywhere across a 28-case matrix. The positive-first early stop stays: it's paired with a real saving, and its soundness rests on the exclusion list, which had no test until now.

The other five:

refactor — opt.First is normalized to MaxInt32 at the top of Uids and never reassigned, so the || opt.First == 0 bail-out and the && applyIntersectWith below it are both dead.
perf — the warm give-up you asked about, plus a correction: I said it would self-heal when the cache entry was replaced. It wouldn't. remove-on-update defaults to false, so an ordinary commit applies in place and keeps the entry, and a single failure would have stuck for the life of it. setMutationAfterCommit now lifts the give-up.
fix(worker) — uidReadFirst is mine from last round and adds offset unclamped. offset is parsed with no lower bound, so first: 10, offset: -1 pushes down 9 and returns a uid short; -100 drops the bound entirely.
test — my slice-identity assertion used require.Equal on two *uint64, which reflect.DeepEqual resolves by following the pointers, so it passed for two distinct arrays. It was asserting nothing.
docs — my comment claimed iterating the private copy is safe because setMutationAfterCommit replaces the shared maps. That's true of the one production call site, not of the function: refresh=false writes both in place, and that's a fatal concurrent map access, not a soft race.

Three things I left open, listed in the PR body: a dropped publish still re-arms the warm, so a key that's both read-hot and write-hot can loop on a walk that gets thrown away (mine, from last round, and I under-described that trade — it's bounded to one walk in flight per key, not one per read); the pushdown is still unsound for a positive first on allof, eq over a lossy-only index, and the geo family, which is pre-existing and wants its own change; and I didn't add a cluster test for the eq case above, so that chain is established by reading the code, not by a test.

Have a look when you get a chance. Sorry for the extra round — better than shipping the first version of finding 1.

fix(posting): stop applying a negative first at the posting layer, plus review follow-ups for dgraph-io#9809

gooohgb · 2026-09-03T07:35:01Z

I took a look at fixing positive first pushdown for lossy eq, but it touches a broader set of query-planning and post-filtering paths than expected, so I agree that it should be handled separately.

I’ve reviewed and merged #3. The overall approach and test coverage look great. Thank you for the careful follow-up and for putting all of this together.

gooohgb requested a review from a team as a code owner August 6, 2026 04:54

matthewmcneely requested changes Aug 10, 2026

View reviewed changes

Comment thread posting/list.go Outdated

Comment thread worker/task.go Outdated

Comment thread posting/mvcc.go

Comment thread posting/list.go Outdated

Comment thread posting/list.go Outdated

Comment thread worker/task.go Outdated

Comment thread posting/list_test.go Outdated

gooohgb added 3 commits August 11, 2026 13:16

Avoid eager UID materialization on posting reads

8a3b438

fix: address calculated uid review feedback

c7ca27c

test: cover calculated uid precache decisions

f647955

gooohgb force-pushed the fix-calculated-uids-materialization branch from ce28734 to f647955 Compare August 11, 2026 05:17

matthewmcneely requested changes Aug 11, 2026

View reviewed changes

Comment thread posting/mvcc.go Outdated

Comment thread posting/mvcc.go Outdated

Comment thread posting/list.go Outdated

Comment thread posting/mvcc_test.go Outdated

Comment thread worker/precalculate_uids_test.go Outdated

gooohgb added 5 commits August 12, 2026 10:32

fix: serialize calculated uid cache warming

a5b2dad

perf: avoid write lock on warmed UID cache hits

e2b3d8f

Merge branch 'main' into fix-calculated-uids-materialization

e5ca5ee

fix(posting): preserve negative first pagination

441d303

chore: rerun CI

2b990b4

matthewmcneely reviewed Aug 19, 2026

View reviewed changes

Comment thread posting/mvcc.go Outdated

matthewmcneely and others added 3 commits September 1, 2026 14:26

matthewmcneely mentioned this pull request Sep 1, 2026

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

Merged

matthewmcneely and others added 6 commits September 2, 2026 15:18

matthewmcneely mentioned this pull request Sep 2, 2026

fix(posting): stop applying a negative first at the posting layer, plus review follow-ups for #9809 gooohgb/dgraph#3

Merged

Merge pull request #3 from dgraph-io/matthewmcneely/9809-followups-2

2078fd6

fix(posting): stop applying a negative first at the posting layer, plus review follow-ups for dgraph-io#9809

Uh oh!

Conversation

gooohgb commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gooohgb commented Aug 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gooohgb commented Aug 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gooohgb commented Aug 12, 2026

Uh oh!

gooohgb commented Aug 17, 2026

Uh oh!

Uh oh!

gooohgb commented Aug 28, 2026

Uh oh!

matthewmcneely commented Sep 1, 2026

Uh oh!

gooohgb commented Sep 2, 2026

Uh oh!

matthewmcneely commented Sep 2, 2026

Uh oh!

gooohgb commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

gooohgb commented Aug 6, 2026 •

edited

Loading