*: Implement iterators and seqnum substitution for foreign SSTs

[itsbilal]

When sstables written to by other stores in shared storage are
read, we need to collapse range keys/range dels/point keys within
each SST and then return keys at fixed sequence numbers reserved
for that level. This change implements that path for all sstables
that are in shared storage according to objstorage.Provider
but have a creator ID that doesn't match ours.

First commit is #2357 .

[cockroach-teamcity]

This change is 

[itsbilal]

Not sure what would be the best way to test foreign sstable iterator creation. I will give a shot at a test that forces some SSTs to be foreign by mutating their creator IDs, but the other way to write a test is to just wait for foreign sstable ingestion to be implemented (next PR) and test it more end-to-end.

[RaduBerinde]

Not sure what would be the best way to test foreign sstable iterator creation. I will give a shot at a test that forces some SSTs to be foreign by mutating their creator IDs, but the other way to write a test is to just wait for foreign sstable ingestion to be implemented (next PR) and test it more end-to-end.

We'll want to have the more end-to-end tests anyway, so unless you think we'd be covering something extra, I'd skip the hackier tests.

[RaduBerinde]

Some minor comment. I can't help much with most of the code but I'll try to understand as much as I can to educate myself.

Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions (waiting on @bananabrick and @itsbilal)

---

range_keys.go line 66 at r1 (raw file):

	iter := current.RangeKeyLevels[0].Iter()
	for f := iter.Last(); f != nil; f = iter.Prev() {
		spanIterOpts := &keyspan.SpanIterOptions{

[nit] consider making a constructor to which you have to pass the level.

---

objstorage/objstorage.go line 125 at r1 (raw file):

// ID that doesn't match ours. The creator ID of the current Provider must
// be passed in.
func (meta *ObjectMetadata) IsForeign(creatorID CreatorID) bool {

[nit] I think this should be a method on the provider (we end up having to do GetCreatorID() against the provider anyway).

[RaduBerinde]

Do we expect this code to have significant overhead? If yes, then in the progressive restore we may want the "hydration" to happen at a higher level, so that the seqnums are fixed inside the hydrated objects? (that would throw a wrench in the current plans of doing it at the objstorage level)

Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions (waiting on @bananabrick and @itsbilal)

[itsbilal]

We won't need to do point collapsing for the progressive restore case, as those sstables will be guaranteed to have one internal key per user key and also their seqnums won't be going into the reserved sequence numbers ranges. That said we would need to apply prefix rewrite rules on them, as discussed elsewhere, but those are going to look very different than anything on here.

TFTR!

Reviewable status: 0 of 19 files reviewed, all discussions resolved (waiting on @bananabrick)

---

range_keys.go line 66 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] consider making a constructor to which you have to pass the level.

Done.

---

objstorage/objstorage.go line 125 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] I think this should be a method on the provider (we end up having to do GetCreatorID() against the provider anyway).

Done.

[RaduBerinde]

Reviewable status: 0 of 19 files reviewed, 4 unresolved discussions (waiting on @bananabrick and @itsbilal)

---

scan_internal.go line 226 at r2 (raw file):

// start of the first user key we are interested in.
func (p *pointCollapsingIterator) findNextEntry() (*base.InternalKey, base.LazyValue) {
	finishAndReturnMerge := func() (*base.InternalKey, base.LazyValue) {

This could be a method on pointCollapsingIterator, no? We have two (almost) copies of it

---

internal/rangekey/coalesce.go line 401 at r2 (raw file):

	dst.Keys = dst.Keys[:0]
	for i := range keys {
		switch keys[i].Kind() {

[nit] this code can be shorter if the switch just sets a seqNum variable and then a single piece of code appends the key.

---

internal/rangekey/coalesce.go line 406 at r2 (raw file):

				panic("pebble: keys unexpectedly not in ascending suffix order")
			}
			dst.Keys = append(dst.Keys, keys[i])

[nit] cleaner to just set it to the right value directly:

append(dst.Keys, Key{
  Trailer: base.MakeTrailer(),
  Suffix: keys[i].Suffix,
  Value: keys[i].Value,
})

---

internal/rangekey/coalesce.go line 416 at r2 (raw file):

			}
		case base.InternalKeyKindRangeKeyUnset:
			if invariants.Enabled && len(dst.Keys) > 0 && cmp(dst.Keys[len(dst.Keys)-1].Suffix, keys[i].Suffix) > 0 {

Is this invariant not expected for InternalKeyKindRangeDelete? Feels like we're just checking that KeysBySuffix does what it says.. We could do that outside the switch or in a separate loop on keys

[itsbilal]

TFTR!

Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion (waiting on @bananabrick and @RaduBerinde)

---

scan_internal.go line 226 at r2 (raw file):

Previously, RaduBerinde wrote…

This could be a method on pointCollapsingIterator, no? We have two (almost) copies of it

Done.

---

internal/rangekey/coalesce.go line 401 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] this code can be shorter if the switch just sets a seqNum variable and then a single piece of code appends the key.

Done.

---

internal/rangekey/coalesce.go line 406 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] cleaner to just set it to the right value directly:

append(dst.Keys, Key{
  Trailer: base.MakeTrailer(),
  Suffix: keys[i].Suffix,
  Value: keys[i].Value,
})

Done.

---

internal/rangekey/coalesce.go line 416 at r2 (raw file):

Previously, RaduBerinde wrote…

Is this invariant not expected for InternalKeyKindRangeDelete? Feels like we're just checking that KeysBySuffix does what it says.. We could do that outside the switch or in a separate loop on keys

It's not because InternalKeyKindRangeDelete doesn't have any prefixes. This logic is the same as UserIteratorConfig.Transform higher up in this file btw.

[RaduBerinde]

Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion (waiting on @bananabrick and @itsbilal)

---

internal/rangekey/coalesce.go line 416 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

It's not because InternalKeyKindRangeDelete doesn't have any prefixes. This logic is the same as UserIteratorConfig.Transform higher up in this file btw.

Got it, thanks, I see now the logic in coalesce() where we add the range-delete key after sorting.

[itsbilal]

Rebased after the virtual sstable changed. PTAL!

[sumeerbhola]

Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions (waiting on @bananabrick and @itsbilal)

---

scan_internal.go line 100 at r4 (raw file):

// pointCollapsingSSTIterator implements sstable.Iterator while composing
// pointCollapsingIterator.

how does the code in this PR handle range keys?

[itsbilal]

Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion (waiting on @bananabrick and @sumeerbhola)

---

scan_internal.go line 100 at r4 (raw file):

Previously, sumeerbhola wrote…

how does the code in this PR handle range keys?

We don't need pointCollapsingSSTIterator for range keys as only point keys flow through it. Range keys are instead passed through a keyspan.TransformIter that applies a ForeignSSTTransformer on the raw range key iter from the underlying SST. The foreign sst transformer 1) collapses sets/unsets/dels within the SST, similar to UserIteratorConfig and 2) exposes the remaining sets at the designated Set sequence number, and the remaining unsets/dels at the appropriate sequence number if L5 (for L6 we skip unsets/dels). Range key masking is handled by the top-level Iterator as always.

You can see the code in table_cache.go where we create the transformIter.

[jbowens]

Reviewed 2 of 19 files at r1, all commit messages.
Reviewable status: 2 of 19 files reviewed, 9 unresolved discussions (waiting on @bananabrick, @itsbilal, and @sumeerbhola)

---

options.go line 227 at r4 (raw file):

// SpanIterOptions creates a SpanIterOptions from this IterOptions.
func (o *IterOptions) SpanIterOptions(level manifest.Level) *keyspan.SpanIterOptions {

Should we pass this around as a struct, instead of a pointer? Otherwise, this allocs per level right?

---

scan_internal.go line 161 at r4 (raw file):

	// If fixedSeqNum is non-zero, all emitted points have this fixed sequence
	// number.
	fixedSeqNum uint64

when is this zero?

---

scan_internal.go line 214 at r4 (raw file):

func (p *pointCollapsingIterator) subSeqNum(key *base.InternalKey) *base.InternalKey {
	if p.fixedSeqNum == 0 || key == nil || key.Kind() == InternalKeyKindRangeDelete {

i'm missing something — how does range delete sequence number substitution work?

---

scan_internal.go line 241 at r4 (raw file):

	if saveValue {
		p.savedValue = val
	}

the saveValue parameter is a little confusing; should the callers that need to save the value save it themselves?

---

scan_internal.go line 356 at r4 (raw file):

				p.iterKey, p.value = p.iter.Next()
				p.saveKey()
				continue

can you explain this logic?

---

table_cache.go line 517 at r4 (raw file):

		// the sequence number in this iter, as these range deletes will not be
		// exposed to anything other than the interleaving iter and
		// pointCollapsingIter.

how is this used? i'm confused as to how this is used and where keys shadowed by range deletes are dropped. it seems like this could be done using a span mask, but it looks like we pass a nil mask down below.

---

table_cache.go line 527 at r4 (raw file):

		}
		pcIter.iter.Init(dbOpts.opts.Comparer, iter, rangeDelIter, nil /* mask */, opts.LowerBound, opts.UpperBound)
		iter = &pointCollapsingSSTIterator{pointCollapsingIterator: pcIter, childIter: iter}

i think we'll need to pool these to avoid allocations in practice

---

sstable/reader.go line 3171 at r4 (raw file):

	}
	i := &fragmentBlockIter{}
	if err := i.blockIter.initHandle(r.Compare, h, seqNum); err != nil {

i think this has a problem in that it can surface multiple RANGEDEL fragments with identical sequence numbers (a violation of the contract). we don't have this problem for ingested sstables since they cannot contain overlapping RANGEDEL fragments.

[itsbilal]

TFTR!

Reviewable status: 1 of 24 files reviewed, 9 unresolved discussions (waiting on @bananabrick, @itsbilal, @jbowens, and @sumeerbhola)

---

options.go line 227 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Should we pass this around as a struct, instead of a pointer? Otherwise, this allocs per level right?

Done, though I had to change the TableNewSpanIter contract for this.

---

scan_internal.go line 161 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

when is this zero?

Shouldn't happen in our uses of it. Should I assert on that?

---

scan_internal.go line 214 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

i'm missing something — how does range delete sequence number substitution work?

We don't expose range deletes through this iterator when in foreign sstable mode. We use the interleaving iterator to apply local deletions but we throw away the rangedel keys (because elideRangeDels = true). Instead they get exposed through NewFixedSeqnumRangeDelIter, see the place in newIters in table_cache.go where we create that iterator.

---

scan_internal.go line 241 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

the saveValue parameter is a little confusing; should the callers that need to save the value save it themselves?

I could make that change, yeah. It's just that it was easiest to follow the convention of findPrevEntry setting savedValue (and findNextEntry not doing that) and also be able to just do return p.finishAndReturnMerge()

---

scan_internal.go line 356 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

can you explain this logic?

Sure. We use the interleavingIter to keep the range del iter in tandem with the point iter. If p.elideRangeDeletes is enabled (which it is for all cases except when we use ScanInternal for generating internal-key sstables for replication), we want to ignore the range del points generated by the interleavingIter, but we still want to be able to peek at the iter.Span() to confirm if the current key is locally deleted.

---

table_cache.go line 517 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

how is this used? i'm confused as to how this is used and where keys shadowed by range deletes are dropped. it seems like this could be done using a span mask, but it looks like we pass a nil mask down below.

My understanding is that using a span mask will also delete points that have a higher sequence number than the range del, as that's how range key masking works. It's why I manually interleave the rangedels and handle deletion in the findNextEntry/findPrevEntry calls to the point collapsing iterator (see the s.CoversAt calls there).

---

table_cache.go line 527 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

i think we'll need to pool these to avoid allocations in practice

Done.

---

sstable/reader.go line 3171 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

i think this has a problem in that it can surface multiple RANGEDEL fragments with identical sequence numbers (a violation of the contract). we don't have this problem for ingested sstables since they cannot contain overlapping RANGEDEL fragments.

This is a good point. Let me address this.

[itsbilal]

TFTR! I'll crank out a cleanup of some of the point-collapsing iter tomorrow, but otherwise this is ready for review.

Reviewable status: 9 of 26 files reviewed, 9 unresolved discussions (waiting on @bananabrick, @jbowens, and @sumeerbhola)

---

scan_internal.go line 161 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

i think so 👍

Ah, the scanInternalIterator use of pointCollapsingIterator does not (and should not) set fixedSeqNum.

---

scan_internal.go line 170 at r6 (raw file):

Previously, jbowens (Jackson Owens) wrote…

nit: should we call this iterValue to parallel iterKey and make it a little more obvious that it's not a iterator-owned copy?

Done.

---

scan_internal.go line 426 at r6 (raw file):

Previously, jbowens (Jackson Owens) wrote…

I'm a bit confused about the contract surrounding saveKey. If this is the first iteration, we already saved the key before we entered the for loop. Or if we just skipped a key deleted by range deletion, it was just saved by the above for loop. But then here we save again. Down below I see other instances where p.iterKey is updated and we proceed to the next iteration without saving the key. I think it would be helpful to document (and enforce) some invariants, and maybe to split the key fields into i.iterKey (always points to the i.iter-owned buffer), i.key (always backed by i.savedKeyBuf) and i.savedKeyBuf that's just a buffer used to back i.key.

I incorporated your suggestion to have us not set p.iterKey to p.savedKey and to always have it point to the child iterator's key. It simplified some uses of both variables in this method. I didn't rename p.savedKey to p.key though, to keep it clearer that there will be some cases where we will continue to pass through p.iterKey instead of saving it, as an optimization. I'll give more thought to how I can enforce invariants tomorrow (and if so, which ones) and where I should be adding comments, but currently this works.

---

scan_internal.go line 445 at r6 (raw file):

Previously, jbowens (Jackson Owens) wrote…

at one point we thought we didn't need to support MERGEs, right? did we conclude that we do? or is it for expediency since timeseries data may be in the same sstable as ordinary mvcc data today?

That's a good point. We handled it just in case we wanted to ScanInternal over them, and the code for it was less complex than for SingleDelete. But the reverse case won't be exercised by ScanInternal anyway. I'll rip this out.

---

sstable/reader.go line 3171 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

the plan is still to use #2465 to avoid the need for point-collapsing in production, right? if so, I think it would be nice to avoid intertwining this implementation too closely with the fragmentBlockIter, since we anticipate needing to untangle it.

When we do #2465 we'll still need some special logic to 1) substitute sequence numbers, and 2) exclude the locally-deleted key kinds, right? So I can see more changes along these lines where we just push the logic down to block / sstable iters, instead of doing it higher up in a pointCollapsingIter of some sorts. And this change is looking similar to the 2) case above.

or maybe I'm not understanding your point? Either way I'm happy to pull this out to a separate iterator, it's just that it'll have to be another wrapping iterator as rangedel iterators are otherwise just straight up fragmentBlockIters I believe.

[jbowens]

looks good, sorry for the slow review!

Reviewed 2 of 19 files at r1, 1 of 8 files at r4, 2 of 15 files at r5, 1 of 2 files at r6, 11 of 13 files at r7, all commit messages.
Reviewable status: 24 of 26 files reviewed, 3 unresolved discussions (waiting on @bananabrick, @itsbilal, and @sumeerbhola)

---

scan_internal.go line 426 at r6 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

I incorporated your suggestion to have us not set p.iterKey to p.savedKey and to always have it point to the child iterator's key. It simplified some uses of both variables in this method. I didn't rename p.savedKey to p.key though, to keep it clearer that there will be some cases where we will continue to pass through p.iterKey instead of saving it, as an optimization. I'll give more thought to how I can enforce invariants tomorrow (and if so, which ones) and where I should be adding comments, but currently this works.

I think we still have some duplicate key saves here (eg, saveKey before entering the for loop, and then a saveKey on this line in the first case block. These are just optimizations though, so maybe just leave a TODO in case this code does end up in use in the release.

---

sstable/reader.go line 3171 at r4 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

When we do #2465 we'll still need some special logic to 1) substitute sequence numbers, and 2) exclude the locally-deleted key kinds, right? So I can see more changes along these lines where we just push the logic down to block / sstable iters, instead of doing it higher up in a pointCollapsingIter of some sorts. And this change is looking similar to the 2) case above.

or maybe I'm not understanding your point? Either way I'm happy to pull this out to a separate iterator, it's just that it'll have to be another wrapping iterator as rangedel iterators are otherwise just straight up fragmentBlockIters I believe.

good point

[itsbilal]

thanks a ton and and no worries! apologies for the slow response from me too

Reviewable status: 24 of 26 files reviewed, 3 unresolved discussions (waiting on @bananabrick, @jbowens, and @sumeerbhola)

---

scan_internal.go line 426 at r6 (raw file):

Previously, jbowens (Jackson Owens) wrote…

I think we still have some duplicate key saves here (eg, saveKey before entering the for loop, and then a saveKey on this line in the first case block. These are just optimizations though, so maybe just leave a TODO in case this code does end up in use in the release.

I just removed that duplication. Turns out there's an optimization where we can saveKey by just storing the trailer (as we're already guaranteed to be at the same user key here).