db: use correct bounds for shared ingestion

[RaduBerinde]

db: remove levelIter reliance on SmallestPointKey kind

The level iterator relies on checking if SmallestPointKey is of kind
InternalKeyKindRangeDelete. But in the case of external files, we
don't know the kind and we use InternalKeyKindMax.

We modify the code to always issue a synthetic boundary at the
smallest boundary if we have a rangeDel iterator, and adjust the logic
to potentially issue two synthetic boundaries (one at
SmallestPointKey and one at Smallest).

We add an external ingestion testcase which fails against the existing
code.

db: use correct bounds for shared ingestion

The kinds in the key bounds for shared ingestion can be incorrect
because of sequence number rewriting. We loosen the bounds to use the
minimum and maximum relevant kinds, similar to external file
ingestion.

We also switch to using AssertBounds instead of
AssertUserKeyBounds which is now possible.

Fixes #3167

[cockroach-teamcity]

This change is 

[RaduBerinde]

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

---

level_iter.go line 1151 at r2 (raw file):

			// the next file.
			if *l.rangeDelIterPtr != nil && l.filteredIter != nil && l.filteredIter.MaybeFilteredKeys() {
				// Only emit this boundary if we haven't emitted it already (or

I tried a ton of variations before I had this idea of emitting both boundaries. What I don't understand is how we got away with only emitting at SmallestPointKey before, since this boundary also seems important. But I don't understand exactly how the mergingIter code that relies on this works.

[jbowens]

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @itsbilal and @RaduBerinde)

---

level_iter.go line 967 at r1 (raw file):

			return l.verify(l.skipEmptyFileBackward())
		}
		return nil, base.LazyValue{}

If we just always emit the smallest boundary (rather than both), I think we can keep this structure. In any case, I think it'd be nice to keep Prev and Next reciprocal in code structure.

---

level_iter.go line 1153 at r1 (raw file):

				// Only emit this boundary if we haven't emitted it already (or
				// emitted one for the same key in the case above).
				if l.smallestBoundary == nil && l.cmp(l.smallestBoundary.UserKey, l.iterFile.Smallest.UserKey) > 0 {

Won't this nil pointer when we access l.smallestBoundary.UserKey because l.smallestBoundary == nil? Am I parsing this correctly? If I am, it's a bit troublesome that no tests triggered it. This is a very difficult case to exercise, but I thought we had a unit test in testdata/iter_histories. Maybe we only cover the largest boundary.

---

level_iter.go line 1151 at r2 (raw file):
I think with the change you made above we no longer need this second boundary condition. If we give the same treatment to the largest boundary, I don't see why we can't remove the MaybeFilteredKeys func and related interfaces altogether.

What I don't understand is how we got away with only emitting at SmallestPointKey before, since this boundary also seems important.

It took me a while staring at it to realize this, but SmallestPointKey is the smallest range deletion and/or point key, so an iterator that's positioned at SmallestPointKey must not have any range deletions that delete smaller keys.

Only range keys can cause Smallest to be less than SmallestPointKey, and the merging iterator doesn't care about range keys at all.

[RaduBerinde]

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @itsbilal and @jbowens)

---

level_iter.go line 1153 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Won't this nil pointer when we access l.smallestBoundary.UserKey because l.smallestBoundary == nil? Am I parsing this correctly? If I am, it's a bit troublesome that no tests triggered it. This is a very difficult case to exercise, but I thought we had a unit test in testdata/iter_histories. Maybe we only cover the largest boundary.

Oh, wow, this was supposed to be an ||. So then we must never be exiting here. But I did try just with the previous boundary and got meta test failures. Maybe I didn't stress it enough.

[RaduBerinde]

level_iter.go line 1151 at r2 (raw file):

Previously, jbowens (Jackson Owens) wrote…

I think with the change you made above we no longer need this second boundary condition. If we give the same treatment to the largest boundary, I don't see why we can't remove the MaybeFilteredKeys func and related interfaces altogether.

What I don't understand is how we got away with only emitting at SmallestPointKey before, since this boundary also seems important.

It took me a while staring at it to realize this, but SmallestPointKey is the smallest range deletion and/or point key, so an iterator that's positioned at SmallestPointKey must not have any range deletions that delete smaller keys.

Only range keys can cause Smallest to be less than SmallestPointKey, and the merging iterator doesn't care about range keys at all.

Then why did we need this boundary before?

[RaduBerinde]

level_iter.go line 1153 at r1 (raw file):

Previously, RaduBerinde wrote…

Oh, wow, this was supposed to be an ||. So then we must never be exiting here. But I did try just with the previous boundary and got meta test failures. Maybe I didn't stress it enough.

I don't think it's possible to get into this path without having emitted the previous boundary (IOW smallestBoundary is never nil here), so that explains why we didn't hit a nil pointer issue. But it also means we don't actually need this. I'll keep playing with it and see what more I can find.

[RaduBerinde]

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @itsbilal and @jbowens)

---

level_iter.go line 967 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

If we just always emit the smallest boundary (rather than both), I think we can keep this structure. In any case, I think it'd be nice to keep Prev and Next reciprocal in code structure.

Done.mak

---

level_iter.go line 1153 at r1 (raw file):

Previously, RaduBerinde wrote…

I don't think it's possible to get into this path without having emitted the previous boundary (IOW smallestBoundary is never nil here), so that explains why we didn't hit a nil pointer issue. But it also means we don't actually need this. I'll keep playing with it and see what more I can find.

I simplified as suggested and it does seem to work. I'm not sure why I was seeing a failure before.

[itsbilal]

, thanks for finally resolving this! The simplification in level_iter is also a great side-effect.

Reviewed 2 of 3 files at r1, 9 of 9 files at r3, 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jbowens and @RaduBerinde)

---

ingest.go line 127 at r4 (raw file):

		// Initialize meta.{HasPointKeys,Smallest,Largest}, etc.
		//
		// NB: We create new internal keys and pass them into ExternalRangeKeyBounds

I know this isn't all your change but s/External/Extend/, and also s/Range/Point/

---

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

		// TODO(radu): we should be using AssertBounds, but it currently fails in
		// some cases (#3167).
		rangeDelIter = keyspan.AssertUserKeyBounds(

ayyy

[RaduBerinde]

TFTR!

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jbowens)

[jbowens]

Reviewed 2 of 3 files at r1, 1 of 9 files at r3, 7 of 7 files at r5, 7 of 7 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @RaduBerinde)

---

level_iter.go line 1151 at r2 (raw file):

Previously, RaduBerinde wrote…

Then why did we need this boundary before?

We needed it before because a range deletion being the smallest key in the table wasn't the only condition with which we might exhaust the point iterator before the range deletion iterator. If a block property filter excludes blocks containing keys smaller than the smallest range deletion, the point iterator could be exhausted while a range deletion in the file is still relevant to another level. Your change here to unconditionally interleave the boundary when there are any range deletions side steps the problem.

I think we should make the reciprocal change to forward iteration / the largest boundary, and then I think we'll be able to remove the entire concept of a filteredIter.

[RaduBerinde]

Makes sense, thanks! I will investigate the upper boundary separately, I hit meta test failures last time I tried it. TFTRs!

[RaduBerinde]

Reviewable status: all files reviewed, 3 unresolved discussions

---

level_iter.go line 1151 at r2 (raw file):

Previously, jbowens (Jackson Owens) wrote…

We needed it before because a range deletion being the smallest key in the table wasn't the only condition with which we might exhaust the point iterator before the range deletion iterator. If a block property filter excludes blocks containing keys smaller than the smallest range deletion, the point iterator could be exhausted while a range deletion in the file is still relevant to another level. Your change here to unconditionally interleave the boundary when there are any range deletions side steps the problem.

I think we should make the reciprocal change to forward iteration / the largest boundary, and then I think we'll be able to remove the entire concept of a filteredIter.

The corresponding change for the upper bound fails various tests with:

checker failed with error: out of order keys wjhyruuycd@4#66,SET >= wjhyruuycd@4#66,SET in L6: fileNum=000005

I tried using just the UserKey and setting the trailer to InternalKeyRangeDeleteSentinel but I get a similar error:

checker failed with error: out of order keys h#3,SET >= h#inf,RANGEDEL in L2: fileNum=000010

Not particularly surprising - if the LargestPointKey is not an exclusive sentinel, it's incorrect to emit it. If we only emit this boundary when the largest point key is an exclusive sentinel, this amounts to the current code (the only possible key type is RangeDel) and we can't really remove the other boundary case..

So I don't see how we can apply the same simplification.

[jbowens]

Reviewable status: all files reviewed, 3 unresolved discussions

---

level_iter.go line 1151 at r2 (raw file):

Previously, RaduBerinde wrote…

The corresponding change for the upper bound fails various tests with:

checker failed with error: out of order keys wjhyruuycd@4#66,SET >= wjhyruuycd@4#66,SET in L6: fileNum=000005

I tried using just the UserKey and setting the trailer to InternalKeyRangeDeleteSentinel but I get a similar error:

checker failed with error: out of order keys h#3,SET >= h#inf,RANGEDEL in L2: fileNum=000010

Not particularly surprising - if the LargestPointKey is not an exclusive sentinel, it's incorrect to emit it. If we only emit this boundary when the largest point key is an exclusive sentinel, this amounts to the current code (the only possible key type is RangeDel) and we can't really remove the other boundary case..

So I don't see how we can apply the same simplification.

I think we might need to just teach the level checker (level_checker.go) about the 'ignorable boundary keys'. I don't think it knows about them currently, and a key that's exactly equal in Trailer is unexpected.

[RaduBerinde]

level_iter.go line 1151 at r2 (raw file):

Previously, jbowens (Jackson Owens) wrote…

I think we might need to just teach the level checker (level_checker.go) about the 'ignorable boundary keys'. I don't think it knows about them currently, and a key that's exactly equal in Trailer is unexpected.

OOh interesting, I'll try that.

[RaduBerinde]

level_iter.go line 1151 at r2 (raw file):

Previously, RaduBerinde wrote…

OOh interesting, I'll try that.

I haven't been able to get back to this yet, I filed an issue so I don't lose track of it (#3334).