kvstorage: speed up scan for range descriptors

[RaduBerinde]

On startup we currently scan through all /Local/Range/ keys in order
to find the range descriptors. In some cases, there can be a lot of
/Local/Range/Transaction records so we end up reading through a lot
of data.

This change switches to using SeekGE:

• when we encounter a key that has a suffix before rdsc like
  /Local/Range/foo/prbe, we seek directly to the corresponding
  descriptor key /Local/Range/foo/rdsc;
• when we encounter a key that has a suffix after rdsc, we
  seek beyond all keys for this range, e.g. /Local/Range/fop/.

Note that inside Pebble, seeks to a key that is very close to the
current position are optimized to try to iterate through the next few
keys before doing a real seek. So this change should not be very
detrimental even when there aren't a lot of keys to skip.

Fixes: #109740

[cockroach-teamcity]

This change is 

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/kv/kvserver/kvstorage/init.go line 329 at r1 (raw file):

				return errors.AssertionFailedf("range key has intent with no timestamp")
			}
			// Seek to the latest value below the intent timestamp.

This code is well exercised by loqrecovery.TestRetrieveApplyStatus, which I have run with --stress

[andrewbaptist]

A few nits, but otherwise changes look good.

I'm not familiar enough with the code and testing in this area, so having a storage review as well would be good.

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

---

-- commits line 17 at r1:
is -> are

---

pkg/kv/kvserver/kvstorage/init.go line 281 at r1 (raw file):

		}

		const reportPeriod = 10 * time.Second

nit: This would be more idiomatic if you used log.EveryN and then use call log.ShouldLog.

---

pkg/kv/kvserver/kvstorage/init.go line 316 at r1 (raw file):

		}

		if key.Timestamp.IsEmpty() {

I'm confused how we get here. Didn't we just check the suffixCmp above so we know this is a LocalRangeDescriptorSuffix? Or does this check mean its a "RangeDescriptor intent"? I'm a little unclear why we need to iterate in Inconsistent mode at all here, so maybe just a comment which justifies that would help.

---

pkg/kv/kvserver/kvstorage/init.go line 342 at r1 (raw file):

			// versions of this key.
			tombstoneCount++
			iter.NextKey()

Couldn't we call
iter.SeekGE(storage.MVCCKey{Key: nextRangeDescKey}) instead of NextKey.

This probably doesn't normally matter in the scheme of things but might be important if we had a large table delete and had not run an MVCCGC yet.

---

pkg/kv/kvserver/kvstorage/init.go line 357 at r1 (raw file):

				key.Key, desc.StartKey)
		}
		if !desc.IsInitialized() {

I see this check in the previous commit as well, but what is the rational for failing on an uninitialized replica. I don't fully understand the lifecycle here, but all replicas pass through this state, but maybe the reason is that we neve persiste them to disk? This is more for my understanding of persisted RangeDescriptors but consider adding a comment why this is an assertion error or make the message cleaner.

---

pkg/kv/kvserver/kvstorage/init.go line 377 at r1 (raw file):

}

func IterateRangeDescriptorsFromDiskOld(

It would be cleaner to delete the old code rather than leaving it here.

[RaduBerinde]

TFTR!

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

---

-- commits line 17 at r1:

Previously, andrewbaptist (Andrew Baptist) wrote…

is -> are

Done.

---

pkg/kv/kvserver/kvstorage/init.go line 281 at r1 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

nit: This would be more idiomatic if you used log.EveryN and then use call log.ShouldLog.

With log.EveryN we would have an undesirable first message at the very start.

---

pkg/kv/kvserver/kvstorage/init.go line 316 at r1 (raw file):
We check above that the key starts with /Local/Range/start-key/rdsc/. Normally we would have only one or more MVCC values like /Local/Range/start-key/rdsc/some-timestamp. For a transasction that was in progress when the node was shut down, we will have an unresolved intent, which is just the "bare" key /Local/Range/start-key/rdsc/ without a timestamp.

I don't know all the details around why we do this in inconsistent mode (but note that the handling would be more complicated otherwise). @bdarnell mentioned the following:

There is one special rule for rdsc that makes it a little easier: the transaction record for any modification to a rdsc record must be located on that range, and the intent is resolved synchronously with the commit/abort of the transaction.

---

pkg/kv/kvserver/kvstorage/init.go line 342 at r1 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

Couldn't we call
iter.SeekGE(storage.MVCCKey{Key: nextRangeDescKey}) instead of NextKey.

This probably doesn't normally matter in the scheme of things but might be important if we had a large table delete and had not run an MVCCGC yet.

We don't have a nextRangeDescKey here..

---

pkg/kv/kvserver/kvstorage/init.go line 357 at r1 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

I see this check in the previous commit as well, but what is the rational for failing on an uninitialized replica. I don't fully understand the lifecycle here, but all replicas pass through this state, but maybe the reason is that we neve persiste them to disk? This is more for my understanding of persisted RangeDescriptors but consider adding a comment why this is an assertion error or make the message cleaner.

This is the descriptor we just unmarshalled. We should never be writing out an uninitialized descriptor (hence the assertion error).

---

pkg/kv/kvserver/kvstorage/init.go line 377 at r1 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

It would be cleaner to delete the old code rather than leaving it here.

Done, I left it accidentally.

[bdarnell]

Reviewed 1 of 7 files at r1, 6 of 6 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andrewbaptist, @jbowens, @nvanbenschoten, and @RaduBerinde)

---

pkg/kv/kvserver/kvstorage/init.go line 316 at r1 (raw file):

but note that the handling would be more complicated otherwise

Not just more complicated, but it would no longer be possible to resolve locally. We'd have to look up the transaction record, which may be on another node, and until then we couldn't be sure whether we should be using the RangeDescriptor value from the intent or the last committed value. And this ambiguity can have cascading effects - if a split or merge was left uncommitted, this need to be resolved before we can use the neighboring range as well. We can't bring a cluster up in the face of this ambiguity (Can't remember whether this is something that is definitely impossible or just something we'd rather not deal with), so we jump through hoops to ensure that intents on range descriptors can be resolved locally and we iterate inconsistently here.

---

pkg/kv/kvserver/kvstorage/init.go line 301 at r3 (raw file):

				iter.SeekGE(storage.MVCCKey{Key: keys.RangeDescriptorKey(keys.MustAddr(startKey))})
			} else {
				// This case should be rare in practice: we have a key that isn't

Can it ever happen? Should we log a warning (or panic?) if we see one?

[RaduBerinde]

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

---

pkg/kv/kvserver/kvstorage/init.go line 301 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Can it ever happen? Should we log a warning (or panic?) if we see one?

I am not sure exactly how things happen when we move a range to another node. We have to update the descriptor and range-delete these keys, do these operations happen together atomically?

If we think it shouldn't happen, I can add a warning and panic in crdb_test mode.

[bdarnell]

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

---

pkg/kv/kvserver/kvstorage/init.go line 301 at r3 (raw file):

Previously, RaduBerinde wrote…

I am not sure exactly how things happen when we move a range to another node. We have to update the descriptor and range-delete these keys, do these operations happen together atomically?

If we think it shouldn't happen, I can add a warning and panic in crdb_test mode.

I would think it would all be atomic but I can't say for sure. If it's not atomic I don't think there's anything else that would asynchronously clean it up so we'd have some kind of leak. I'd be comfortable with at least putting in something like a panic in debug/test mode.

[RaduBerinde]

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

---

pkg/kv/kvserver/kvstorage/init.go line 301 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I would think it would all be atomic but I can't say for sure. If it's not atomic I don't think there's anything else that would asynchronously clean it up so we'd have some kind of leak. I'd be comfortable with at least putting in something like a panic in debug/test mode.

Done.

[RaduBerinde]

CI failure is #124197

bors r+

[craig]

Build succeeded:

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

[RaduBerinde]

How do folks feel about backporting this to 24.1.x (after it bakes on master for a while)?