crl-release-23.2: internal/keyspan: obey Seek invariants in filteringIter

[itsbilal]

23.2 Backport of #3046.

---

Previously if filteringIter's FilterFunc mutated the passed-in span to no longer be a valid return value for a SeekLT or SeekGE call, we would still return that span even though it could be >= the seek key (for SeekLT), or less than it (for SeekGE).

This change updates filteringIter to guard for this case before returning from a seek call.

Found with #3044.

Informs cockroachdb/cockroach#113973, cockroachdb/cockroach#114056.

[cockroach-teamcity]

This change is 

[RaduBerinde]

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

---

internal/keyspan/filter.go line 60 at r1 (raw file):

	// no knowledge of the seek key) mutated the span to start at a key greater
	// than or equal to `key`. Detect this case and prev/invalidate the iter.
	if span != nil && i.cmp(span.Start, key) >= 0 {

Should this be > 0? span.Start is inclusive, no?

[RaduBerinde]

internal/keyspan/filter.go line 49 at r1 (raw file):

	// less than or equal to `key`. Detect this case and next/invalidate the iter.
	if span != nil && i.cmp(span.End, key) <= 0 {
		return i.Next()

Is it not possible for i.Next() to still be less than the key? Like if we are seeking to a0 and we have keys a1, a2 which the filter truncates to a?

[itsbilal]

TFTR!

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

---

internal/keyspan/filter.go line 49 at r1 (raw file):

Previously, RaduBerinde wrote…

Is it not possible for i.Next() to still be less than the key? Like if we are seeking to a0 and we have keys a1, a2 which the filter truncates to a?

We're relying on the invariant that the underlying iter returned a span that was the correct SeekGE/SeekLT span, but filter excluded it, so a Next would take us to something that's strictly forward. Right?

---

internal/keyspan/filter.go line 60 at r1 (raw file):

Previously, RaduBerinde wrote…

Should this be > 0? span.Start is inclusive, no?

SeekLT is supposed to return a span that covers a key strictly less than the seek key, so >= is correct to catch the case that's wrong.

[RaduBerinde]

internal/keyspan/filter.go line 49 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

We're relying on the invariant that the underlying iter returned a span that was the correct SeekGE/SeekLT span, but filter excluded it, so a Next would take us to something that's strictly forward. Right?

Can't the filter exclude the next one too? I guess I don't understand what's special about the first key returned by Seek that doesn't apply to the following key

[RaduBerinde]

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

---

internal/keyspan/filter.go line 49 at r1 (raw file):

Previously, RaduBerinde wrote…

Can't the filter exclude the next one too? I guess I don't understand what's special about the first key returned by Seek that doesn't apply to the following key

I guess it's not possible with the truncate, because only the first span can intersect the upper bound.. but I'm less clear what is the filter contract in general, w.r.t modification of the span.

[itsbilal]

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

---

internal/keyspan/filter.go line 49 at r1 (raw file):

Previously, RaduBerinde wrote…

I guess it's not possible with the truncate, because only the first span can intersect the upper bound.. but I'm less clear what is the filter contract in general, w.r.t modification of the span.

Yeah. You're right that this covers the Truncate case, but doesn't generally solve a FilterFunc case for some contracts of FilterFunc that are currently completely not spelled out. We've filed #3048 to separate the two implementations anyway. so I'm okay with a Truncate-only fix for now.

[RaduBerinde]

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

---

internal/keyspan/filter.go line 49 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Yeah. You're right that this covers the Truncate case, but doesn't generally solve a FilterFunc case for some contracts of FilterFunc that are currently completely not spelled out. We've filed #3048 to separate the two implementations anyway. so I'm okay with a Truncate-only fix for now.

Awesome! Makes sense now!

[itsbilal]

TFTR!

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