cockroachdb / cockroachdb/cockroach
storage: audit pebbleMVCCScanner.seekVersion uncertainty checks
- 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
Assessment
This issue has not been assessed yet.