storage: add MVCCTimeInterval block property for range keys

[erikgrinaker]

This patch adds MVCCTimeInternal block property collection and
filtering for range keys, which allows using time-bound iterators with
range keys.

Range keys will only be written once the MVCCRangeTombstones version
gate is enabled.

Resolves #82596.

Release note: None

[cockroach-teamcity]

This change is 

[erikgrinaker]

@jbowens This doesn't work because of the in-mem arena right? I'll just let this sit until we land compaction and can enable durability then.

[jbowens]

Yeah, but even after compaction lands, I think b-c@{5,6,7,8} will all end up in the same range-key block. Overlapping 'stacks' of range keys flushed at the same time will be output together in a single block (and really, internally a single key in a single block). To get each range key to be in a separate block you can either write them with different bounds, or flush between writes.

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

[erikgrinaker]

Yeah, figured as much, thanks.

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

[erikgrinaker]

Looks like DataBlockIntervalCollector receives the encoded range key, so I'll need access to decoding functions to extract the suffix. Opened cockroachdb/pebble#1774.

[erikgrinaker]

This is ready for review now.

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

---

pkg/storage/pebble.go line 330 at r4 (raw file):

// pebbleDataBlockMVCCTimeIntervalPointCollector implements
// pebble.DataBlockIntervalCollector for point keys.
type pebbleDataBlockMVCCTimeIntervalPointCollector struct {

I split out separate implementations for point and range keys to avoid having to check the key type (point vs range) in Add(), since Pebble already checks it for us when deciding which property collector to use. We could keep a common implementation if we're not too worried about checking this twice.

---

pkg/storage/pebble_test.go line 634 at r3 (raw file):

	// Range keys [b-c)@5, [c-d)@7, [d-e)@9 in separate blocks in a single SST.
	//
	// TODO(erikgrinaker): Seems like these all end up in the same block anyway?

@jbowens: seems like range keys end up in the same block even though the don't overlap. Is this expected, given BlockSize=1?

[jbowens]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, @itsbilal, and @nicktrav)

---

pkg/storage/pebble.go line 330 at r4 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

I split out separate implementations for point and range keys to avoid having to check the key type (point vs range) in Add(), since Pebble already checks it for us when deciding which property collector to use. We could keep a common implementation if we're not too worried about checking this twice.

it's all pretty simple, so the code duplication doesn't bother me 👍

---

pkg/storage/pebble.go line 353 at r4 (raw file):

var (
	_ sstable.DataBlockIntervalCollector      = &pebbleDataBlockMVCCTimeIntervalRangeCollector{}
	_ sstable.SuffixReplaceableBlockCollector = (*pebbleDataBlockMVCCTimeIntervalRangeCollector)(nil)

nit: is there a reason to not use (*pebbleDataBlockMVCCTimeIntervalRangeCollector)(nil) for both of these interface assertions?

---

pkg/storage/pebble_test.go line 634 at r3 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

@jbowens: seems like range keys end up in the same block even though the don't overlap. Is this expected, given BlockSize=1?

I didn't expect it, but after thinking about it, yeah, it makes sense. Created cockroachdb/pebble#1788.

[erikgrinaker]

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

---

pkg/storage/pebble.go line 353 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

nit: is there a reason to not use (*pebbleDataBlockMVCCTimeIntervalRangeCollector)(nil) for both of these interface assertions?

Nah, this carried over from the previous code. Fixed, thanks.

[erikgrinaker]

CI failures are unrelated.

TFTR!

bors r=jbowens

[craig]

Build succeeded:

• GitHub CI (Cockroach)