sstable: add (*sstable.Writer).RangeKey{Set,Unset,Delete} methods #1445

nicktrav · 2022-01-07T18:55:03Z

Users of sstable.Writer other than Pebble itself (e.g. Cockroach)
require a means of adding range keys to an sstable that is slightly more
flexible than the existing AddRangeKey method in the way in which key
spans can be added to the table.

Specifically, AddRangeKey requires that spans be sorted (start / end
key ascending, sequence number and key kind descending), in addition to
fragmented and coalesced (i.e. multiple suffixes within the same span
are coalesced into a single value).

Add the RangeKey{Set,Unset,Delete} methods on the sstable.Writer
that allow adding spans in order of start key. These methods have no
requirement that the spans be fragmented or coalesced. Instead, the
implementation handles the fragmenting of spans as keys are added. A
keyspan.Fragmenter emits completed spans into a rangekey.coalescer,
which aggregates the fragments for the same span into a
rangekey.CoalescedSpan, which can then be used to write the individual
RangeKey{Set,Unset,Delete} key / value pairs into the rang key block
in the sstable.

The new RangeKey{Set,Unset,Delete} methods have similar semantics to
the existing Set,Delete,DeleteRange,Merge methods that write the key /
value pairs at sequence number zero. In the range key case, these new
methods are intended to be used by external callers when preparing
sstables for ingestion.

Add a data-driven test specific to the new range key methods. These
tests are separate to the existing data driven test cases for the
AddRangeKey method, which has stricter requirements on how keys are
added (i.e. already ordered, fragmented and coalesced).

Rename AddInternalRangeKey to AddRangeKey to better match the
existing naming of (*sstable.Writer).Add.

Add new utility methods to the rangekey package to aid in encoding
range key values.

Related to #1339.

Some open questions:

Should the API allow for passing in multiple suffix / value pairs with the same start / end key - currently the caller would need to make multiple calls.
Safety guarantees of passing byte slices to the new functions. Specifically, we only perform a copy when a span is finalized. Depending on how the caller re-uses any byte slices, the underlying values in the fragmenter may change.

cockroach-teamcity · 2022-01-07T18:55:10Z

This change is

nicktrav

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions

internal/rangekey/rangekey.go, line 73 at r1 (raw file):

// SuffixValues, when encoded for a RangeKeySet. It may be used to construct a
// buffer of the appropriate size before encoding.
func EncodedSetSuffixValuesLen(suffixValues []SuffixValue) int {

I found myself doing a lot of gymnastics with just the suffix values (i.e. the value without the end key encoded). I split these functions out.

sstable/writer.go, line 398 at r1 (raw file):

// not required to be fragmented.
func (w *Writer) RangeKeySet(start, end, suffix, value []byte) error {
	// FIXME(travers): Should the API allow for adding multiple (suffix, value)

Open question: is this sufficient? Or should we support allowing the caller to pass in multiple suffix / value pairs. Same for RangeKeyUnset.

sstable/writer.go, line 401 at r1 (raw file):

	// pairs at the same time?
	startKey := base.MakeInternalKey(start, 0, base.InternalKeyKindRangeKeySet)
	return w.addRangeKeySpan(startKey, end, suffix, value)

Open question: there's some subtlety in tracking the lifetimes of the byte slices that are passed to these methods. As we only perform a copy when the fragmented span is coalesced and finalized, it's possible that the caller could alter the underlying data before we add to the block. Do we want to preemptively copy?

jbowens

Looks great!

Reviewed 2 of 6 files at r1.
Reviewable status: 2 of 6 files reviewed, 11 unresolved discussions (waiting on @nicktrav and @sumeerbhola)

internal/rangekey/rangekey.go, line 180 at r1 (raw file):

}

// EncodeUnsetSuffixValues encodes a slice of SuffixValues for a RangeKeyUnset

nit: s/slice of SuffixValues/slice of suffixes/

internal/rangekey/rangekey.go, line 182 at r1 (raw file):

// EncodeUnsetSuffixValues encodes a slice of SuffixValues for a RangeKeyUnset
// into dst. The length of dst must be greater than or equal to
// EncodedSetSuffixValuesLen. EncodeSetSuffixValues returns the number of bytes

nit: s/EncodeSetSuffixValues/EncodeUnsetSuffixes/

internal/rangekey/rangekey.go, line 185 at r1 (raw file):

// written, which should always equal the EncodedSetValueLen with the same
// arguments.
func EncodeUnsetSuffixValues(dst []byte, suffixes [][]byte) int {

s/EncodeUnsetSuffixValues/EncodeUnsetSuffixes/

sstable/writer.go, line 398 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

Open question: is this sufficient? Or should we support allowing the caller to pass in multiple suffix / value pairs. Same for RangeKeyUnset.

I think this is sufficient. 👍

sstable/writer.go, line 401 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

Open question: there's some subtlety in tracking the lifetimes of the byte slices that are passed to these methods. As we only perform a copy when the fragmented span is coalesced and finalized, it's possible that the caller could alter the underlying data before we add to the block. Do we want to preemptively copy?

I vote preemptively copy to err towards guarding against misuse, since we expect range keys to be rare in CockroachDB. We can walk that decision back if we need to during performance testing.

We could share a bytes.Buffer for all range key buffered data (including the encoded suffix-values tuples) to reduce the allocations..

sstable/writer.go, line 451 at r1 (raw file):

	case base.InternalKeyKindRangeKeyUnset:
		suffixes := [][]byte{suffix}
		buf := make([]byte, rangekey.EncodedUnsetSuffixValuesLen(suffixes))

I think it might be worthwhile to use a shared bytes.Buffer on the Writer for these encoded Span.Values to reduce allocations.

sstable/writer.go, line 468 at r1 (raw file):

	// only flush when we've moved past the previous key.
	if w.compare(span.Start.UserKey, w.prevRangeKeyUserKey) > 0 {
		w.fragmenter.TruncateAndFlushTo(w.prevRangeKeyUserKey)

Is this necessary? I think Fragmenter.Add will flush if necessary. I think we can Add each key span and call Finish when finishing the sstable, and that should be enough to ensure everything is flushed.

sstable/writer.go, line 496 at r1 (raw file):

}

func (w *Writer) addCoalescedSpan(s rangekey.CoalescedSpan) {

Can you add a comment here stating that this method is used as the emit function of the Coalescer?

sstable/testdata/writer_range_keys, line 1 at r1 (raw file):

# NOTE: The operations SET, UNSET, and DEL in this test file are aliases for

These test cases are great (and so easy to read!)

sstable/testdata/writer_range_keys, line 85 at r1 (raw file):

d.RANGEKEYSET.0: e [(@9=v9)]
d.RANGEKEYDEL.0: e
f.RANGEKEYSET.0: i [(@4=v4),(@5=v5),(@7=v7),(@8=v8),(@9=v9)]

nit: The ordering of these suffix-values is opposite, because the test uses base.DefaultComparer which compares suffixes as byte slices. I think it'd be worthwhile to use testkeys.Comparer and reverse the order here to avoid confusion.

nicktrav

Dismissed @jbowens from a discussion.
Reviewable status: 1 of 6 files reviewed, 4 unresolved discussions (waiting on @jbowens and @sumeerbhola)

internal/rangekey/rangekey.go, line 185 at r1 (raw file):