cockroachdb / cockroachdb/cockroach

storage: audit pebbleMVCCScanner.seekVersion uncertainty checks

Open
#144,964 1 comment 0 reactions 0 assignees View on GitHub
A-storage branch-master C-bug T-storage
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

Creating an issue from a slack conversation. There's a lack of understanding on the current behavior of the code, and some deviation from the related commentary.: https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1745258504646689?thread_ts=1745253288.411869&cid=C0KB9Q03D

@jbowens: I noticed that in `pebbleMVCCScanner.seekVersion` we perform a step-before-seeking optimization. All the while, we check uncertainty intervals: https://github.com/cockroachdb/cockroach/blob/4d337776ab865f89b965bf6bd047d9f84c7220e1/pkg/storage/pebble_mvcc_scanner.go#L1406-L1453
Are some of these uncertainty interval checks unnecessary? Whether or not we even observe the intermediary versions is nondeterministic, dependent on our current `itersBeforeSeek` heuristic, so it seems like there must be unnecessary uncertainty interval checks here (although it may be more trouble than it's worth to attempt to refactor to avoid only the unnecessary checks)

@sumeerbhola: regarding uncertainty and `itersBeforeSeek` , I suppose you are talking about https://github.com/cockroachdb/cockroach/blob/77ff853eb89c4da67b6b0f06a5c407f96827d9de/pkg/storage/pebble_mvcc_scanner.go#L1422. If the condition in https://github.com/cockroachdb/cockroach/blob/77ff853eb89c4da67b6b0f06a5c407f96827d9de/pkg/storage/pebble_mvcc_scanner.go#L1449 is false, then the timestamp is higher than the uncertainty, so it is possible that the next version we find is uncertain, so we need to keep checking. The only callers with `uncertaintyCheck=true` are calling with `p.seekVersion(ctx, p.uncertainty.GlobalLimit, true)`. Which leads me to two questions (possibly for Arul:
why are these callers not calling with `p.uncertainty.LocalLimit` (when it is set)?
`seekVersion` commentary seems to be wrong. The specified timestamp in https://github.com/cockroachdb/cockroach/blob/77ff853eb89c4da67b6b0f06a5c407f96827d9de/pkg/storage/pebble_mvcc_scanner.go#L1383 is not the seekTS. If it were there would be an else block in https://github.com/cockroachdb/cockroach/blob/77ff853eb89c4da67b6b0f06a5c407f96827d9de/pkg/storage/pebble_mvcc_scanner.go#L1451 that would also return. Instead we keep iterating in that loop until `p.curUnsafeKey.Timestamp.LessEq(p.ts)`.

Jira issue: CRDB-49639

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.