sstable: Add bounds checking for virtual sstable iterators

[raggar]

This pr adds bounds checking (global and block) for singleLevelIterator when returning a value.

Fixes: #2593
Release note: None

[cockroach-teamcity]

This change is 

[bananabrick]

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

---

-- commits line 4 at r1:
Should we also add for the twoLevelIterator? While the twoLevelIterator uses a single level iterator, there are certain positioning calls which will entirely skip any calls to the singleLevelIterator.

---

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

// helper function to check if keys returned from iterator are within block and global bounds
func (i *singleLevelIterator) CheckKeyWithinBounds(iKey *InternalKey) {

nit: Could call this MaybeCheckKeyWithinBounds.

---

sstable/reader.go line 470 at r1 (raw file):

func (i *singleLevelIterator) CheckKeyWithinBounds(iKey *InternalKey) {
	// used for virtual sstable iterators
	if invariants.Enabled && i.vState != nil {

I think there could be a bug in the virtual sstable logic for these iterators such that we don't set the upper, blockUpper, lower, blockLower fields appropriately. We should also check against the vState.{lower, upper}. What do you think?

---

sstable/reader.go line 472 at r1 (raw file):

	if invariants.Enabled && i.vState != nil {
		key := iKey.UserKey
		withinUpper := i.cmp(key, i.upper) <= 0 && i.cmp(key, i.blockUpper) <= 0

Note that the upper bound can be exclusive. You can use i.endKeyInclusive to determine if it is or not.

[raggar]

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

---

sstable/reader.go line 472 at r1 (raw file):

Previously, bananabrick (Arjun Nair) wrote…

Note that the upper bound can be exclusive. You can use i.endKeyInclusive to determine if it is or not.

the comment for endInclusive says that it is used (set true) for virtual sstable iterators, so would it be an error if vState != nil and i.endKeyInclusive == False?

[raggar]

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

---

sstable/reader.go line 470 at r1 (raw file):

Previously, bananabrick (Arjun Nair) wrote…

I think there could be a bug in the virtual sstable logic for these iterators such that we don't set the upper, blockUpper, lower, blockLower fields appropriately. We should also check against the vState.{lower, upper}. What do you think?

yeah we can check these bounds as well

[raggar]

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

---

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

Previously, bananabrick (Arjun Nair) wrote…

nit: Could call this MaybeCheckKeyWithinBounds.

yup!

[raggar]

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

---

-- commits line 4 at r1:

Previously, bananabrick (Arjun Nair) wrote…

Should we also add for the twoLevelIterator? While the twoLevelIterator uses a single level iterator, there are certain positioning calls which will entirely skip any calls to the singleLevelIterator.

yeah we can add checks in there as well

[raggar]

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

---

sstable/reader.go line 470 at r1 (raw file):

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

yeah we can check these bounds as well

if a field such as blockUpper is not set is that an issue or should we just skip performing the check on that bound?

[jbowens]

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

---

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

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

yup!

nit: we should take in both (ikey *InternalKey, value []byte) and return them as well. Call sites can simply wrap the internal function returning them, eg

return i.MaybeCheckKeyWithinBounds(i.lastInternal())

[bananabrick]

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

---

-- commits line 4 at r4:
We should either add the twoLevelIterator checks here, or remove the fixes line.

---

sstable/reader.go line 470 at r1 (raw file):

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

if a field such as blockUpper is not set is that an issue or should we just skip performing the check on that bound?

I think we should just use virtual sstable bounds for this checking. The block bounds/upper and lower will be set using virtual sstable bounds anyway.

---

sstable/reader.go line 472 at r1 (raw file):

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

the comment for endInclusive says that it is used (set true) for virtual sstable iterators, so would it be an error if vState != nil and i.endKeyInclusive == False?

I think i wrote a bad comment. We should check endKeyInclusive in this function.

---

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

}

// helper function to check if keys returned from iterator are within block and global bounds

nit: We usually use full sentences as comments. Like "Helper function to check if keys returned from iterator are within block and global bounds." Also applies to the other comments here.

[bananabrick]

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

---

sstable/reader.go line 470 at r1 (raw file):

Previously, bananabrick (Arjun Nair) wrote…

I think we should just use virtual sstable bounds for this checking. The block bounds/upper and lower will be set using virtual sstable bounds anyway.

Correction: upper/lower can be within the virtual bounds, if there's a call to SetBounds. I think it's worthwhile to check upper/lower. I don't think there's additional benefit to checking blockUpper/blockLower.

[itsbilal]

Looking mostly good! Just some minor comments in addition to Arjun's comment about endKeyInclusive.

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @bananabrick, @jbowens, and @RahulAggarwal1016)

---

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

// helper function to check if keys returned from iterator are within block and global bounds
func (i *singleLevelIterator) MaybeCheckKeyWithinBounds(

this doesn't need to be exported, can be lowercase eg. just verifyKey.

---

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

	}
	// NB: skipForward checks whether exhaustedBounds is already +1.
	return i.singleLevelIterator.MaybeCheckKeyWithinBounds(i.skipForward())

We should call this methid inside skipForward/skipBackward instead of doing that here.

---

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

		if result == loadBlockOK {
			if ikey, val := i.singleLevelIterator.firstInternal(); ikey != nil {
				return ikey, val

The check should happen here

[raggar]

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

---

-- commits line 4 at r4:

Previously, bananabrick (Arjun Nair) wrote…

We should either add the twoLevelIterator checks here, or remove the fixes line.

I added the checks into some of the twoLevelIterators functions such as Prev and NextPrefix.

[raggar]

Reviewable status: 2 of 3 files reviewed, 8 unresolved discussions (waiting on @bananabrick, @itsbilal, and @jbowens)

---

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

Previously, bananabrick (Arjun Nair) wrote…

nit: We usually use full sentences as comments. Like "Helper function to check if keys returned from iterator are within block and global bounds." Also applies to the other comments here.

Done

---

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

Previously, itsbilal (Bilal Akhtar) wrote…

The check should happen here

i added the check inside of firstInternal

[raggar]

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @bananabrick, @itsbilal, and @jbowens)

---

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

Previously, jbowens (Jackson Owens) wrote…

nit: we should take in both (ikey *InternalKey, value []byte) and return them as well. Call sites can simply wrap the internal function returning them, eg

return i.MaybeCheckKeyWithinBounds(i.lastInternal())

Done

[raggar]

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @bananabrick, @itsbilal, and @jbowens)

---

sstable/reader.go line 470 at r1 (raw file):

Previously, bananabrick (Arjun Nair) wrote…

Correction: upper/lower can be within the virtual bounds, if there's a call to SetBounds. I think it's worthwhile to check upper/lower. I don't think there's additional benefit to checking blockUpper/blockLower.

Done

---

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

Previously, itsbilal (Bilal Akhtar) wrote…

this doesn't need to be exported, can be lowercase eg. just verifyKey.

Done

[raggar]

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @bananabrick, @itsbilal, and @jbowens)

---

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

Previously, itsbilal (Bilal Akhtar) wrote…

We should call this methid inside skipForward/skipBackward instead of doing that here.

Done

[raggar]

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @bananabrick, @itsbilal, and @jbowens)

---

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

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

i added the check inside of firstInternal

Correction: updated it to be added outside

[RaduBerinde]

If I understand correctly, this checks that all keys fall within the bounds in the metadata. But we're not checking that the bounds are "tight", correct? I would leave the issue open until we are able to check that too.

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @bananabrick, @itsbilal, @jbowens, and @RahulAggarwal1016)

[bananabrick]

Reviewable status: 2 of 3 files reviewed, 8 unresolved discussions (waiting on @itsbilal, @jbowens, and @RahulAggarwal1016)

---

sstable/reader.go line 478 at r6 (raw file):

		lc, vlc := i.cmp(key, i.lower), i.cmp(key, i.vState.lower.UserKey)

		if (!i.endKeyInclusive && (uc == 0 || vuc == 0)) || (uc > 0 || vuc > 0 || lc < 0 || vlc < 0) {

I think endKeyInclusive applies to lower/upper. Basically, you could create an iterator with virtual sstable bounds [a, d]. In this case end key is inclusive for virtual sstables. lower, upper will be a, d and endKeyInclusive will be true.

Then you could have a call to SetBounds(a, c). In this case, lower, upper will be a,c and endKeyInclusive will be false.

So, the virtual sstable bounds, and the lower, upper bounds might have different values for whether their last key is inclusive or exclusive.

I think this won't cause correctness issues, but it might be better to use !i.vState.Upper.IsExclusiveSentinel() to determine if virtual sstable bounds are inclusive.

[bananabrick]

Reviewable status: 2 of 3 files reviewed, 8 unresolved discussions (waiting on @itsbilal, @jbowens, and @RahulAggarwal1016)

---

sstable/reader.go line 472 at r1 (raw file):

Previously, bananabrick (Arjun Nair) wrote…

I think i wrote a bad comment. We should check endKeyInclusive in this function.

nit: We should fix the comment.

[raggar]

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @bananabrick, @itsbilal, and @jbowens)

---

sstable/reader.go line 472 at r1 (raw file):

Previously, bananabrick (Arjun Nair) wrote…

nit: We should fix the comment.

done

[raggar]

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @bananabrick, @itsbilal, and @jbowens)

---

sstable/reader.go line 478 at r6 (raw file):

Previously, bananabrick (Arjun Nair) wrote…

I think endKeyInclusive applies to lower/upper. Basically, you could create an iterator with virtual sstable bounds [a, d]. In this case end key is inclusive for virtual sstables. lower, upper will be a, d and endKeyInclusive will be true.

Then you could have a call to SetBounds(a, c). In this case, lower, upper will be a,c and endKeyInclusive will be false.

So, the virtual sstable bounds, and the lower, upper bounds might have different values for whether their last key is inclusive or exclusive.

I think this won't cause correctness issues, but it might be better to use !i.vState.Upper.IsExclusiveSentinel() to determine if virtual sstable bounds are inclusive.

done

[raggar]

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @bananabrick, @itsbilal, and @jbowens)

---

sstable/reader.go line 478 at r6 (raw file):

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

done

correction: should it be just i.vState.Upper.IsExclusiveSentinel() I believe the previous function was checking if it was inclusive

[itsbilal]

@RaduBerinde That's right. We don't have a check here for whether the bounds are right. I'm good with accepting this as-is if we reopen the issue and make it just about the bounds being tight (i.e. there being a user key at each file bound). that check would likely have to happen at virtual sstable creation time, maybe in ValidateVirtual, so it'll be a pretty different change from this.

Reviewed 1 of 1 files at r7, 1 of 1 files at r8.
Reviewable status: all files reviewed (commit messages unreviewed), 7 unresolved discussions (waiting on @bananabrick and @jbowens)

[itsbilal]

Oops, didn't mark that as an "approving" review.

[bananabrick]

Reviewable status: all files reviewed (commit messages unreviewed), 5 unresolved discussions (waiting on @itsbilal, @jbowens, @RaduBerinde, and @RahulAggarwal1016)

---

sstable/reader.go line 478 at r6 (raw file):

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

correction: should it be just i.vState.Upper.IsExclusiveSentinel() I believe the previous function was checking if it was inclusive

I think we should use !upperBoundInclusive for uc, and IsExclusiveSentinel for vuc.

[raggar]

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

---

sstable/reader.go line 478 at r6 (raw file):

Previously, bananabrick (Arjun Nair) wrote…

I think we should use !upperBoundInclusive for uc, and IsExclusiveSentinel for vuc.

Done

[bananabrick]

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @itsbilal, @jbowens, and @RaduBerinde)