storage: disable checkUncertainty on failOnMoreRecent in scanner

[tbg]

It was possible for reads with failOnMoreRecent to hit a
ReadWithinUncertaintyIntervalError instead of the desired
WriteTooOldError. This commit disables uncertainty checks when
failOnMoreRecent is active, as the latter is a stronger check anyway.

Fixes #119681.
Fixes #131005.

Epic: none
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/storage/pebble_mvcc_scanner.go line 890 at r1 (raw file):

				p.mostRecentKey = append(p.mostRecentKey, p.curUnsafeKey.Key...)
			}
			// We don't have to fall through to `p.checkUncertainty` because FIXME.

We don't have to fall through to p.checkUncertainty because we are already considering any key that is more recent to be a write-write conflict which will cause a write-too-old error to be thrown. Uncertainty checks are only needed if we would otherwise ignore keys that are more recent.

Here, we could return a WriteTooOld error immediately, but we defer that through mostRecentTS so that we return the maximum conflicting timestamp. We are trying to do the same thing below in processRangeKeys, but notice that there's no early return in the p.failOnMoreRecent case (we don't want an early return, but a continue or an else if would work). As a result, the deferred-but-preferred error case loses out to the immediate-but-less-preferred case.

This does raise the question of whether we should actually make p.failOnMoreRecent and p.checkUncertainty mutually exclusive. We could set checkUncertainty to false when failOnMoreRecent is set to true, so that we don't need to rely on the logic in each of these functions getting it right. This would be easy to do in pebbleMVCCScanner.init when setting checkUncertainty.

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/storage/pebble_mvcc_scanner.go line 890 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We don't have to fall through to p.checkUncertainty because we are already considering any key that is more recent to be a write-write conflict which will cause a write-too-old error to be thrown. Uncertainty checks are only needed if we would otherwise ignore keys that are more recent.

Here, we could return a WriteTooOld error immediately, but we defer that through mostRecentTS so that we return the maximum conflicting timestamp. We are trying to do the same thing below in processRangeKeys, but notice that there's no early return in the p.failOnMoreRecent case (we don't want an early return, but a continue or an else if would work). As a result, the deferred-but-preferred error case loses out to the immediate-but-less-preferred case.

This does raise the question of whether we should actually make p.failOnMoreRecent and p.checkUncertainty mutually exclusive. We could set checkUncertainty to false when failOnMoreRecent is set to true, so that we don't need to rely on the logic in each of these functions getting it right. This would be easy to do in pebbleMVCCScanner.init when setting checkUncertainty.

Done. I still need to add a test.

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/storage/pebble_mvcc_scanner.go line 890 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Done. I still need to add a test.

Done.

[nvanbenschoten]

Reviewed 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

[tbg]

bors r+
TFTR!

[craig]

Build failed:

• lint

[tbg]

/github-actions-runner/_work/_temp/76ea3ede-7a2b-4587-9cc3-dbbe871ca2b7.sh: line 1: ./build/github/summarize-build.sh: No such file or directory

🤷🏽

bors r+

[craig]

Build failed:

• macos_amd64_build

[arulajmani]

bors retry

[craig]

Build succeeded:

• lint
• linux_amd64_fips_build
• linux_amd64_build
• docker_image_amd64
• unit_tests
• windows_build
• examples_orms
• local_roachtest_fips
• acceptance
• macos_amd64_build
• local_roachtest
• linux_arm64_build
• check_generated_code
• macos_arm64_build