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

[matthewmcneely]

Follow-ups to the review on dgraph-io#9809, based on your 71cae0b9. Six commits, each independent — take, change, or drop any of them. Merging into fix-calculated-uids-materialization folds them into dgraph-io#9809.

Three of these are things I got wrong or left wrong in the last round, so it is mostly cleanup after myself. One of them changed direction under verification, and that is the important one.

1. fix(posting): stop applying a negative first to the posting read

I originally reported this as "the memoized path should trim a negative first the way the walk path does". That was the wrong direction, and adversarial review caught it before it shipped. The trim comes out instead.

Trimming in Uids 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; filterGeoFunction re-checks the real geometry. A uid the trim drops is one that filter never sees, and x.PageRange behind it 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. That is what a cold read does today. It is what both reads would have done if I had aligned the paths the way I first proposed.

allof is the same shape through intersection rather than post-filtering: it is not in calculatePaginationParams' exclusion list, needsIntersect matches it, and the query layer intersects the per-token lists before paginating. tail(A) ∩ tail(B) ≠ tail(A ∩ B).

So the warm answer was the correct one all along, and removing the trim fixes the cold path too — which has been wrong since 441d3033. Over a 28-case matrix of First × AfterUid × Intersect, warm and cold now agree everywhere.

Nothing depended on the trim. Uids has eight production callers, only three can carry a nonzero First, and every one 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, and its soundness rests on calculatePaginationParams keeping the per-token-intersect functions 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.

This partially reverts bc52f9351 from the last round — the tail copy it added is unreachable once the trim is gone.

2. refactor(posting): drop the unreachable checks in the Uids tail

opt.First is normalized to math.MaxInt32 at the top and never reassigned, so || opt.First == 0 cannot fire and the && applyIntersectWith below it is already known true. Renamed to postProcess, since it gates the truncation as well as the intersect.

3. perf(posting): stop retrying a UID warm that cannot succeed

You flagged the log rate; the log was the cheap part. Every reader that wins the election on an unreadable key repeats the whole walk, plus a Badger read per split. So the list gets parked instead: uidWarmState gains a third state, and a failed walk leaves it there.

The correction to what I wrote last time: I claimed this would self-heal when the cache entry was replaced. It would not. remove-on-update defaults to false, so an ordinary commit applies in place on the published list and keeps the entry — a single failure would have stuck for the life of that entry. setMutationAfterCommit now lifts the give-up, so a failure costs one attempt per commit to the key, the same bound doRollup gets from its own per-key dedupe.

4. fix(worker): a negative offset must not shrink a bounded read

uidReadFirst is mine from the last round, and it adds offset unclamped. offset is parsed with no lower bound in params.fill, so first: 10, offset: -1 pushes down 9 and the query comes back a uid short; offset: -100 pushes down -90 and drops the bound entirely. Clamped, which is what x.PageRange does with a negative offset when it paginates behind it.

5. test(posting): assert slice identity in the warm handover

require.Equal on two *uint64 falls through to reflect.DeepEqual, which follows both pointers and compares the uids they address — so it passed for two distinct arrays starting with the same uid, which is exactly what it was written to rule out. require.Same. Confirmed by making publishCalculatedUids copy the slice: Equal still passed, Same fails.

6. docs(posting): state the copy-on-write guarantee where it actually holds

My comment said iterating the private copy is safe "because setMutationAfterCommit replaces those maps instead of writing into them". That is true of one call site, not of the function: only refresh=true rebuilds the maps first, and refresh=false writes both in place. Production only reaches a cached list through updateItemInCache, which passes true — but the comment as written would tell whoever adds the next caller that a refresh=false commit on a published list is fine, and that one is a fatal concurrent map access.

Verification

go build ./..., gofmt clean, go vet with no new findings, go test ./posting/ -race green in full, ./worker/ and ./query/ green. Each fix confirmed to fail without it, including the two panics and the two weak assertions.

Open, and deliberately not in here

A dropped publish still re-arms the warm. This is mine, from the non-blocking warm last round, and I under-described the trade at the time. When a commit lands mid-walk, publishCalculatedUids drops the result and finishUidWarm re-arms the election, so the next reader walks again. On a key that is both read-hot and write-hot that can loop. It is bounded to one walk in flight per key rather than one per read, since concurrent readers lose the CAS and serve unwarmed — so the cost is a background walk that achieves nothing, not a per-read cost. I did not fix it: parking on a drop would leave a key that stops being committed unwarmed indefinitely, and the doRollup-style per-key attempt throttle wants a mutex on the read path. Worth its own change with its own thinking.

The pushdown is still unsound for a positive first. Removing the negative trim fixes that sign for every post-filtered and intersected function. The positive early stop has the same problem and cannot be fixed the same way, because it is load-bearing as an optimization. calculatePaginationParams excludes regexp/alloftext/allofterms/match/ngram, but not allof (intersected), not eq on a lossy-only index (post-filtered), and not the geo family (post-filtered). Pre-existing and independent of this PR — happy to file it separately.

No cluster-level test for the eq-on-lossy-index case. It needs Docker, which I did not have here, so the end-to-end symptom above is established by reading the code rather than by a test. The unit tests pin the mechanism — that Uids does not apply a negative first on either path — which is the only way to reintroduce it.

Two small things I looked at and left. resetIfCurrent calls ml.cache.get purely to compare pointers, and that is not a pure accessor: it books a ristretto hit, which feeds x.PLCacheHitRatio. It is one extra hit per successful warm against every real read, and it is a genuine hit, so I left it rather than resurrect deleted entries to avoid it. And CachePL.Set (posting/mvcc.go:758) has no callers at all — pre-existing dead code, not mine to delete here.