*: Set isIgnorableBoundaryKey=true for rangedel boundaries in levelIter

[itsbilal]

Merge keys put the top level Iterator's position to iterPosNext, and
in the process of Next()ing to the next user key, one of the child
levelIters could have advanced past the sstable that contained the
Merge that is being returned.

If that sstable contained a range delete covering a key in an sstable
in a lower level, and the lower level's key was not exposed to the
mergingIter due to a bloom filter mismatch, and we do two consecutive
SeekPrefixGEs where the second one gets TrySeekUsingNext, we could
end up surfacing the deleted key in the lower sstable as the
higher level's levelIter has already moved past the sstable
containing the range deletion.

This change sets isIgnorableBoundaryKey = true when returning
largestBoundary in the levelIter to prevent the mergingIter from
next()ing past it in the first SeekPrefixGE.

Fixes #2118.

[cockroach-teamcity]

This change is 

[itsbilal]

@jbowens happy to hear your thoughts on this; maybe this is an overly-defensive disabling of the optimization when we can do something like setting isIgnorableBoundaryKey to true in the levelIter.skipEmptyFileForward case that pauses at the rangedel sentinel key. For some reason levelIter flips that flag to true in SeekPrefixGE's rangedel-sentinel pausing case but not in Next()/skipEmptyFileForward().

[jbowens]

I'm having a hard time wrapping my head around what the bug is. It looks like the unit test doesn't exercise the bug, because the test still passes after reverting iterator.go?

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

[itsbilal]

@jbowens you were right. I found the last missing piece of the puzzle; the lower level SST has to have a bloom filter mismatch in the first SeekPrefixGE, otherwise the deleted key gets surfaced to the mergingIter and we don't double-next() the higher level sstable. The test fails now if you revert the other change. I also made the isIgnorableBoundaryKey change as that passes the test too.

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

[itsbilal]

Nevermind, this trips up another test. I'll revert to the previous fix soon.

[itsbilal]

Reverted to the original fix and test improved, PTAL!

[sumeerbhola]

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

---

iterator.go line 1298 at r3 (raw file):

	}
	if lastPositioningOp == seekPrefixGELastPositioningOp && i.pos != iterPosNext {
		// iterPosNext is unsafe to optimize for TrySeekUsingNext. This is because

I'm having some difficulty with this change, both in understanding and whether this ought to be the fix. Ideally we should be able to state an invariant about the InternalIterator tree and use that to decide on the optimization here (like we do in our current optimizations in iterator.go) and not have to reason about the details about the InternalIterator implementation. I worry that with the latter we will move much closer to not being able to understand or improve these optimizations in the future.
I don't quite understand the gap in the current pausing logic in level_iter.go and merging_iter.go that is causing this -- I suspect the fix belongs there and not in iterator.go.

btw, I'm unsure about how this works in the presence of InterleavingIter. nextPointCurrentUserKey can set i.pos = iterPosNext because the range key iterator is the current position and the point iter it at the next position. I guess this is harmless from a correctness perspective, but will mean we don't use TrySeekUsingNext?

[jbowens]

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

---

iterator.go line 1298 at r3 (raw file):

Previously, sumeerbhola wrote…

I'm having some difficulty with this change, both in understanding and whether this ought to be the fix. Ideally we should be able to state an invariant about the InternalIterator tree and use that to decide on the optimization here (like we do in our current optimizations in iterator.go) and not have to reason about the details about the InternalIterator implementation. I worry that with the latter we will move much closer to not being able to understand or improve these optimizations in the future.
I don't quite understand the gap in the current pausing logic in level_iter.go and merging_iter.go that is causing this -- I suspect the fix belongs there and not in iterator.go.

btw, I'm unsure about how this works in the presence of InterleavingIter. nextPointCurrentUserKey can set i.pos = iterPosNext because the range key iterator is the current position and the point iter it at the next position. I guess this is harmless from a correctness perspective, but will mean we don't use TrySeekUsingNext?

Ditto. Is the bug that we don't do this logic when the level iter gets Next-ed during prefix iteration mode?

And currently we don't have knowledge of whether we're in prefix mode during levelIter.Next, so we can't readily just add that logic to the Next exhaustion case?

[itsbilal]

iterator.go line 1298 at r3 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Ditto. Is the bug that we don't do this logic when the level iter gets Next-ed during prefix iteration mode?

And currently we don't have knowledge of whether we're in prefix mode during levelIter.Next, so we can't readily just add that logic to the Next exhaustion case?

Just pushed a detailed example into the comments, hope that helps. Also reverted to the isIgnorableBoundaryKey fix that I had yesterday, which is in line with Sumeer's comment. It changed some other datadriven tests like I had mentioned earlier but it seems safe to me (more eyes here would help).

Not sure if levelIter needs to know about prefix iteration, but we shouldn't be double-Nexting past the sstable containing the range delete in any case (I think).

[jbowens]

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

---

level_iter.go line 960 at r4 (raw file):

				if l.boundaryContext != nil {
					l.boundaryContext.isIgnorableBoundaryKey = true
				}

This isIgnorableBoundaryKey change makes sense to me.

If I'm not mistaken, we could adjust the handling of range tombstones to always rely on isIgnorableBoundaryKey and remove mergingIter.elideRangeTombstones. We've be relying on the levelIter always setting isIgnorableBoundaryKey = true whenever it returns one of these boundary keys (including in skipEmptyFileBackward). The mergingIter would not need to perform any checks for range deletion key kinds. When using within an Iterator, the mergingIter will never return range tombstone keys, as it does today with elideRangeTombstones=true. When used in compactions, it will continue to return interleaved range deletions, just not any of the artificial boundary keys.

---

iterator.go line 1298 at r3 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Just pushed a detailed example into the comments, hope that helps. Also reverted to the isIgnorableBoundaryKey fix that I had yesterday, which is in line with Sumeer's comment. It changed some other datadriven tests like I had mentioned earlier but it seems safe to me (more eyes here would help).

Not sure if levelIter needs to know about prefix iteration, but we shouldn't be double-Nexting past the sstable containing the range delete in any case (I think).

Thanks, this example helped.

[sumeerbhola]

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

---

level_iter.go line 960 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

This isIgnorableBoundaryKey change makes sense to me.

If I'm not mistaken, we could adjust the handling of range tombstones to always rely on isIgnorableBoundaryKey and remove mergingIter.elideRangeTombstones. We've be relying on the levelIter always setting isIgnorableBoundaryKey = true whenever it returns one of these boundary keys (including in skipEmptyFileBackward). The mergingIter would not need to perform any checks for range deletion key kinds. When using within an Iterator, the mergingIter will never return range tombstone keys, as it does today with elideRangeTombstones=true. When used in compactions, it will continue to return interleaved range deletions, just not any of the artificial boundary keys.

I went and reread #1725 (comment). Is the improvement you are suggesting changing the bullet "Pausing when reach a file bound that is a rangedel: mergingIter skips these keys since it ignores based on InternalKeyKindRangeDelete"?

btw, while we are here I think we should improve the commentary around isSyntheticIterBoundsKey and isIgnorableBoundaryKey. I tend to forget why both concepts are needed, and maybe others do too.
My understanding is that the former (a) plays a role in switching directions that needs it to be a separate concept (but the details for that I have forgotten), (b) allows for stating an invariant for the whole mergingIter when the synthetic key is at the top, since the bounds that are reached apply to the whole iterator heap (in comparison isIgnorableBoundaryKey is a local pause for a level). (b) is why we can break here

pebble/merging_iter.go

Lines 687 to 688 in 1bda21f

	if m.levels[item.index].isSyntheticIterBoundsKey {
	break

while we can't do so for isIgnorableBoundaryKey

[jbowens]

I went and reread #1725 (comment). Is the improvement you are suggesting changing the bullet "Pausing when reach a file bound that is a rangedel: mergingIter skips these keys since it ignores based on InternalKeyKindRangeDelete"?

Yeah. Instead of skipping them based on InternalKeyKindRangeDelete, it'd skip them based on isIgnorableBoundaryKey.

My understanding is that isIgnorableBoundaryKey is set to indicate that a key is an artificial boundary key that exists only to prevent the level iterator from proceeding to the next file before the merging iterator is done with its range deletions. The merging iterator skips these ignorable boundary keys. Previously, before the introduction of isIgnorableBoundaryKey, we would always use Kind() == InternalKeyKindRangeDelete to determine whether a way as a boundary key that could be skipped. Currently, on master, we use both, inconsistently.

---

I'm having a hard time recalling when isSyntheticIterBoundsKey (879f3bf) is necessary.

[itsbilal]

My understanding is that isIgnorableBoundaryKey is set to indicate that a key is an artificial boundary key that exists only to prevent the level iterator from proceeding to the next file before the merging iterator is done with its range deletions. The merging iterator skips these ignorable boundary keys. Previously, before the introduction of isIgnorableBoundaryKey, we would always use Kind() == InternalKeyKindRangeDelete to determine whether a way as a boundary key that could be skipped. Currently, on master, we use both, inconsistently.

There's one additional use of isIgnorableBoundaryKey and it's in mergingIter.findNextEntry where we check if we're in prefix iteration and at an ignorable boundary key that's at a different prefix than the prefix we're searching for. See:

pebble/merging_iter.go

Line 722 in 1bda21f

if m.prefix != nil && (reseeked || m.levels[item.index].isIgnorableBoundaryKey) {

This is the case we want to go into to avoid this bug, but weren't before this change.

I'll make the change to remove elideRangeTombstones in a separate commit just to keep the bugfix isolated in history.

[itsbilal]

Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @sumeerbhola)

---

level_iter.go line 960 at r4 (raw file):

Previously, sumeerbhola wrote…

I went and reread #1725 (comment). Is the improvement you are suggesting changing the bullet "Pausing when reach a file bound that is a rangedel: mergingIter skips these keys since it ignores based on InternalKeyKindRangeDelete"?

btw, while we are here I think we should improve the commentary around isSyntheticIterBoundsKey and isIgnorableBoundaryKey. I tend to forget why both concepts are needed, and maybe others do too.
My understanding is that the former (a) plays a role in switching directions that needs it to be a separate concept (but the details for that I have forgotten), (b) allows for stating an invariant for the whole mergingIter when the synthetic key is at the top, since the bounds that are reached apply to the whole iterator heap (in comparison isIgnorableBoundaryKey is a local pause for a level). (b) is why we can break here

pebble/merging_iter.go

Lines 687 to 688 in 1bda21f

	if m.levels[item.index].isSyntheticIterBoundsKey {
	break

while we can't do so for isIgnorableBoundaryKey

@jbowens made the change, PTAL!

[jbowens]

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

[jbowens]

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

[itsbilal]

TFTR @jbowens !

Should we backport the first commit to 22.2? I think we should.

[sumeerbhola]

Reviewed 1 of 4 files at r2, 1 of 4 files at r4, 1 of 1 files at r5, 6 of 7 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions

---

merging_iter.go line 736 at r7 (raw file):

		if item.key.Visible(m.snapshot, m.batchSnapshot) &&
			(!m.levels[item.index].isIgnorableBoundaryKey) &&
			(item.key.Kind() != InternalKeyKindRangeDelete || !m.elideRangeTombstones) {

liking the simplification!