storage: add time-bound iterator handling for range keys

[erikgrinaker]

We currently use table/block filters for time-bound iteration of point keys. We need this to handle range keys too. At the very least, it cannot have false negatives (skip tables/blocks with range keys that are within the range key interval). It would be great if we could rely on pebble.IterOptions.RangeKeyFilters for this, but all Pebble databases may not support it so it's possible this would require a costly data migration. In the worst case, just make sure we don't skip over range key blocks at all.

cockroach/pkg/storage/pebble_iterator.go

Lines 195 to 227 in 28e600d

	if opts.MaxTimestampHint.IsSet() {
	encodedMinTS := string(encodeMVCCTimestamp(opts.MinTimestampHint))
	encodedMaxTS := string(encodeMVCCTimestamp(opts.MaxTimestampHint))
	p.options.TableFilter = func(userProps map[string]string) bool {
	tableMinTS := userProps["crdb.ts.min"]
	if len(tableMinTS) == 0 {
	if opts.WithStats {
	p.timeBoundNumSSTables++
	}
	return true
	}
	tableMaxTS := userProps["crdb.ts.max"]
	if len(tableMaxTS) == 0 {
	if opts.WithStats {
	p.timeBoundNumSSTables++
	}
	return true
	}
	used := encodedMaxTS >= tableMinTS && encodedMinTS <= tableMaxTS
	if used && opts.WithStats {
	p.timeBoundNumSSTables++
	}
	return used
	}
	// We are given an inclusive [MinTimestampHint, MaxTimestampHint]. The
	// MVCCWAllTimeIntervalCollector has collected the WallTimes and we need
	// [min, max), i.e., exclusive on the upper bound.
	p.options.PointKeyFilters = []pebble.BlockPropertyFilter{
	sstable.NewBlockIntervalFilter(mvccWallTimeIntervalCollector,
	uint64(opts.MinTimestampHint.WallTime),
	uint64(opts.MaxTimestampHint.WallTime)+1),
	}
	}

Jira issue: CRDB-16526

Epic CRDB-2624

[blathers-crl]

cc @cockroachdb/replication

[erikgrinaker]

@jbowens Re: our previous discussion about this. Seems like we should move to block property collectors for TBIs. These appear to already support range keys.

Given that enabling range keys requires ratcheting the Pebble database version and CRDB cluster version, we can assume that once we start writing these block properties then readers will respect them. We can also stop writing table properties.

I don't think we need to do anything more here? Older SSTables that contain existing table props would be filtered like today based on point keys, and these cannot contain range keys so we're not incorrectly filtering anything here. Newer SSTables would not have table props, and thus would only be filtered on block properties, which are written accurately both for point and range keys.

Am I missing anything?

[nicktrav]

I just created #82678 to work out if / how we can go about removing table properties entirely. We've been talking about this on Storage for a while.

[erikgrinaker]

I just created #82678 to work out if / how we can go about removing table properties entirely. We've been talking about this on Storage for a while.

Way ahead of you: #82658

We'll just need to keep respecting them for as long as nodes can still have SSTs written by 21.2 or older versions. Not sure how best to ensure that though -- might need an actual migration in 23.1 or something.

[nicktrav]

might need an actual migration in 23.1 or something.

Yeah, that issue is more for the migration. I'll update it to make it more pointed.

We have some precedent for doing these two step migrations now (see #77634 and cockroachdb/pebble#1537), so hopefully we can reuse some of that functionality.