sstable: don't skipBackward past empty blocks with hideObsolete

[itsbilal]

Currently, if we skipBackward in singleLevelIterator and come across a seemingly empty data block, we stop right there. This is not safe if hide obsolete points is true, as the block could have just been obsolete in its entirety. This change makes the iterator continue iterating backward until some other termination condition is exhausted.

This matches the behaviour for two-level iterators, as well as for skipForward in single-level iterators.

Fixes #3761.

[cockroach-teamcity]

This change is 

[itsbilal]

This really only affects shared foreign sstables because all other use cases of hideObsoletePoints use both the block property filter and the iterator transform, while for shared foreign sstables we only use the iterator transform. That's why it only popped up in the two instance metamorphic test.

[RaduBerinde]

Nice find! Do you understand why it wasn't detected before?

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

---

sstable/reader_iter_single_lvl.go line 1449 at r1 (raw file):

		}
		kv := i.data.Last()
		if kv == nil {

I don't understand the kv == nil check at all.. Other than HideObsoletePoints, is it possible to have empty blocks? And if we do, why would we just stop there?

[itsbilal]

@RaduBerinde just cuz it takes disaggregated storage and shared ingestions to trigger this, a specific set of blocks with snapshot-pinned obsolete keys (something that only happens rarely), and sometimes an excise or so.

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

---

sstable/reader_iter_single_lvl.go line 1449 at r1 (raw file):

Previously, RaduBerinde wrote…

I don't understand the kv == nil check at all.. Other than HideObsoletePoints, is it possible to have empty blocks? And if we do, why would we just stop there?

I guess I wanted this change to be as incremental as possible. But you're right that in all cases of empty-ish blocks, we should keep going for correctness reasons, but limiting that is an optimization. Should I have us continue in all cases?

[RaduBerinde]

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

---

sstable/reader_iter_single_lvl.go line 1449 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

I guess I wanted this change to be as incremental as possible. But you're right that in all cases of empty-ish blocks, we should keep going for correctness reasons, but limiting that is an optimization. Should I have us continue in all cases?

Yeah, and if hideobsoletepoints is the only case where we think this can happen, adding an invariants check that HideObsoletePoints is true

[itsbilal]

sstable/reader_iter_single_lvl.go line 1449 at r1 (raw file):

Previously, RaduBerinde wrote…

Yeah, and if hideobsoletepoints is the only case where we think this can happen, adding an invariants check that HideObsoletePoints is true

HideObsoletePoints isn't the only case where this happens, it seems. We test for (and come across) other cases where data blocks are empty if I add a panic here. I think we should unconditionally continue though; it matches skipForward behaviour. I'll make that change here.

[itsbilal]

sstable/reader_iter_single_lvl.go line 1449 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

HideObsoletePoints isn't the only case where this happens, it seems. We test for (and come across) other cases where data blocks are empty if I add a panic here. I think we should unconditionally continue though; it matches skipForward behaviour. I'll make that change here.

Done.

[RaduBerinde]

Reviewable status: 0 of 3 files reviewed, all discussions resolved

[itsbilal]

TFTR!