storage: restore lower-bound of 1 on pebbleMVCCScanner.itersBeforeSeek

[sumeerbhola]

This was supposed to be temporarily removed in
b258e82 but was never restored.
It accounts for a 2x slowdown in a benchmark where a scan encounters a few keys at the beginning of the scan with > 5 versions, that cause itersBeforeSeek to drop to 0, and then for the remaining 1000s of keys with only 1 versions it uses SeekGE instead of Next.

Informs #96361

Also see https://github.com/cockroachlabs/support/issues/2033

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]

Nice find! Sorry for all the trouble this caused over in #96361 and the corresponding support ticket.

This was supposed to be temporarily removed in b258e82 but was never restored.

It sounds like that commit was the issue, but I don't recall ever planning to revert this one part of the change. I misunderstood how this itersBeforeSeek accounting works. I assumed that even if we let itersBeforeSeek drop to 0, we would reset/increment it again after the next seek. That's not the case though — once it drops to 0, we never iterate, so we never increment the value. Instead, we stop iterating entirely.

I do think it's worthwhile keeping support for the metamorphic maxItersBeforeSeek to be 0 so that we exercise a seek-only version of pebbleMVCCScanner in tests. If you agree, then we'll need to change this PR to let decrementItersBeforeSeek keep itersBeforeSeek at 0 if maxItersBeforeSeek is also set to 0.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens and @nicktrav)

[sumeerbhola]

I do think it's worthwhile keeping support for the metamorphic maxItersBeforeSeek to be 0 so that we exercise a seek-only version of pebbleMVCCScanner in tests. If you agree, then we'll need to change this PR to let decrementItersBeforeSeek keep itersBeforeSeek at 0 if maxItersBeforeSeek is also set to 0.

Done

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens and @nicktrav)

[nvanbenschoten]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jbowens, @nicktrav, and @sumeerbhola)

---

pkg/storage/pebble_mvcc_scanner.go line 558 at r2 (raw file):

			p.itersBeforeSeek = 1
		} else if p.itersBeforeSeek < 0 {
			// INVARIANT: maxItersBeforeSeek == 0 && p.itersBeforeSeek < 0.

Should this say // INVARIANT: maxItersBeforeSeek == 0 && p.itersBeforeSeek == 0.?

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jbowens, @nicktrav, and @nvanbenschoten)

---

pkg/storage/pebble_mvcc_scanner.go line 558 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should this say // INVARIANT: maxItersBeforeSeek == 0 && p.itersBeforeSeek == 0.?

I removed the "INVARIANT:". I was trying to state the condition that holds at this point in the code, and not an invariant that is true at all times.

[nvanbenschoten]

Still LGTM

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jbowens and @nicktrav)

[sumeerbhola]

TestStoreMetrics flake is the same as in #93916

[sumeerbhola]

Trying again with skipping this test