sstable: reimplement index block lazy loading in single-level and two-level iterator

[xxmplus]

This implementation addresses the data corruption issues in the previous attempt (527eebf and fe43e7b, reverted) successfully.

This reimplementation maintains the same I/O optimization benefits as the original attempt. While the behavior is transparent to users, the internal semantic is different now. The order of events in some data-driven tests have to change accordingly. See changes in checkpoint, cleaner, and event_listener.

There are also tests added to demonstrate how bloom filter can benefit from lazy loading by avoid loading index block entirely. See changes in flushable_ingest, and reader_lazy_loading.

Resolves #3248

[cockroach-teamcity]

This change is 

[RaduBerinde]

CC @jbowens - please take a look as well given that there were subtle problems with the previous iteration.

Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions

---

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

// lazyState holds the state for lazy loading of index blocks.
type lazyState struct {
	// loaded indicates if the index block has been loaded

[nit] Mention that when an error is hit during index load, loaded will be true and err will be set

---

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

type lazyState struct {
	// loaded indicates if the index block has been loaded
	loaded bool

[nit] I'd separate the "constant" fields from the fields we are updating (i.e. move loaded next to err)

---

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

	comparer      *base.Comparer
	transforms    blockiter.Transforms
	err           error

[nit] the err field could use a comment

---

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

	// Perserve context for lazy loading.
	i.indexLazyState = &lazyState{

Why does indexLazyState need to be a pointer? We need to avoid allocations in these hot paths.

---

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

		ctx:           ctx,
		reader:        r,
		indexFilterRH: i.indexFilterRH,

Do we need this to store reader, indexFilerRH, readEnv, transforms? Can't we use them directly from i? (they shouldn't change) Same for comparer which is just i.reader.Comparer

Hm.. that doesn't leave much.. Do you know specifically which parameters changing were causing problems in the previous iteration of the change?

[xxmplus]

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

---

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

Previously, RaduBerinde wrote…

[nit] Mention that when an error is hit during index load, loaded will be true and err will be set

added.

---

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

Previously, RaduBerinde wrote…

[nit] I'd separate the "constant" fields from the fields we are updating (i.e. move loaded next to err)

moved.

---

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

Previously, RaduBerinde wrote…

[nit] the err field could use a comment

added.

---

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

Previously, RaduBerinde wrote…

Do we need this to store reader, indexFilerRH, readEnv, transforms? Can't we use them directly from i? (they shouldn't change) Same for comparer which is just i.reader.Comparer

Hm.. that doesn't leave much.. Do you know specifically which parameters changing were causing problems in the previous iteration of the change?

This is actually a good point. I should verify them individually instead of preserving everything. Let me test it more.

[xxmplus]

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

---

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

Previously, RaduBerinde wrote…

Why does indexLazyState need to be a pointer? We need to avoid allocations in these hot paths.

It's in fact not required, see the latest commit.

---

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

Previously, xxmplus (Andy Xu) wrote…

This is actually a good point. I should verify them individually instead of preserving everything. Let me test it more.

Thanks for catching this. I tested these fields individually (remove one by one) and realized that none of them are required to be preserved. Like you mentioned above, none of them changes. What failed the previous attempt is in fact the way error was preserved and the loaded flag was set. Previously lazy loading error was saved in iterator's err field and got cleared. And the loaded flag was only set when index was loaded successfully. This caused problems when iterator was reset and reused.

[RaduBerinde]

Reviewable status: 0 of 12 files reviewed, 9 unresolved discussions (waiting on @jbowens)

---

sstable/data_test.go line 378 at r6 (raw file):

				fmt.Fprintf(&b, "|  index.isDataInvalidated()=%t\n", si.index.IsDataInvalidated())
			}
			if si != nil {

[nit] these should all go in a single block

---

sstable/reader_iter_test.go line 74 at r6 (raw file):

			})
			if err != nil {
				// If construction fails, that's also valid (for non-lazy implementations)

We no longer have a non-lazy implementation, no?

---

sstable/reader_iter_test.go line 77 at r6 (raw file):

				continue
			}
			// With lazy loading, the error should happen on first use

[nit] . at the end

---

sstable/reader_iter_two_lvl.go line 184 at r6 (raw file):

	}
	i.secondLevel.data.InitOnce(r.keySchema, r.Comparer, &i.secondLevel.internalValueConstructor)
	topLevelIndexH, err := r.readTopLevelIndexBlock(ctx, i.secondLevel.readEnv.Block, i.secondLevel.indexFilterRH)

Is there a benefit to lazy loading the top-level block? I think we only expect two-level indexes on L6 which doesn't normally use bloom filters. @jbowens, @sumeerbhola WDYT?

---

sstable/reader_iter_two_lvl.go line 1063 at r6 (raw file):

// Close implements internalIterator.Close, as documented in the pebble
// package.
func (i *twoLevelIterator[I, PI, D, PD]) ensureTopLevelIndexLoaded() error {

This function could set i.secondLevel.err instead of every call site doing it.

---

sstable/reader_iter_single_lvl.go line 175 at r6 (raw file):

	index I
	data  D

[nit] you can consider something like

lazyIndexLoad struct {
  enabled bool
  loaded bool
  err error
}

---

sstable/reader_iter_single_lvl.go line 178 at r6 (raw file):

	// indexLazyEnabled is set to true if the index block is lazy-loaded. This is the
	// default for single-level iterators. But it can be false for two-level iterators.
	indexLazyEnabled bool

This is a bit confusing as it suggests the type also supports eager loading. Instead, why not just set indexLoaded = true from loadSecondLevelIndexBlock()?

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

	indexLoaded bool
	// indexLoadError saves the error from the index block load operation. Note that
	// the error is not reset for reuse.

What does "is not reset for reuse" mean here?

---

sstable/reader_iter_single_lvl.go line 1626 at r6 (raw file):

}

func (i *singleLevelIterator[I, PI, D, PD]) ensureIndexLoaded() error {

This function could set i.err instead of every call site doing it.

[xxmplus]

Reviewable status: 0 of 12 files reviewed, 9 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @sumeerbhola)

---

sstable/reader_iter_single_lvl.go line 178 at r6 (raw file):

Previously, RaduBerinde wrote…

This is a bit confusing as it suggests the type also supports eager loading. Instead, why not just set indexLoaded = true from loadSecondLevelIndexBlock()?

good point, removed it to avoid confusion.

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

Previously, RaduBerinde wrote…

What does "is not reset for reuse" mean here?

corrected. it should read "Note that the error is not reset when the iterator is reused."

---

sstable/reader_iter_single_lvl.go line 1626 at r6 (raw file):

Previously, RaduBerinde wrote…

This function could set i.err instead of every call site doing it.

updated.

---

sstable/reader_iter_two_lvl.go line 1063 at r6 (raw file):

Previously, RaduBerinde wrote…

This function could set i.secondLevel.err instead of every call site doing it.

updated.

---

sstable/data_test.go line 378 at r6 (raw file):

Previously, RaduBerinde wrote…

[nit] these should all go in a single block

updated.

---

sstable/reader_iter_test.go line 74 at r6 (raw file):

Previously, RaduBerinde wrote…

We no longer have a non-lazy implementation, no?

removed to avoid confusion.

---

sstable/reader_iter_test.go line 77 at r6 (raw file):

Previously, RaduBerinde wrote…

[nit] . at the end

added.

[RaduBerinde]

Very cool!

Reviewable status: 0 of 12 files reviewed, 6 unresolved discussions (waiting on @jbowens, @sumeerbhola, and @xxmplus)

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

Previously, xxmplus (Andy Xu) wrote…

corrected. it should read "Note that the error is not reset when the iterator is reused."

[nit] It's still confusing. "Reuse" normally refers to when we put it in a pool then use it again (see .resetForReuse()). I think this wants to say that once this error is set, it is never changed for the lifetime of the iterator (and all operations that require the index block fail).

---

sstable/reader_iter_test.go line 5 at r10 (raw file):

// the LICENSE file.

//go:build invariants

This is unusual - what is the reason? Could we instead just if !invariants.Enabled { t.Skip() } the relevant test(s)?

---

sstable/reader_iter_two_lvl.go line 103 at r10 (raw file):

func (i *twoLevelIterator[I, PI, D, PD]) resolveMaybeExcluded(dir int8) intersectsResult {
	if !i.ensureTopLevelIndexLoaded() {
		return blockExcluded // Conservative choice when we can't load the index

blockIntersects would be the conservative choice here.. But we should never be calling this without having consulted an index block; I would just change it to if invariants.Enabled && !i.indexLoaded { panic() }

[RaduBerinde]

Reviewable status: 0 of 12 files reviewed, 6 unresolved discussions (waiting on @jbowens, @sumeerbhola, and @xxmplus)

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

Previously, RaduBerinde wrote…

[nit] It's still confusing. "Reuse" normally refers to when we put it in a pool then use it again (see .resetForReuse()). I think this wants to say that once this error is set, it is never changed for the lifetime of the iterator (and all operations that require the index block fail).

Actually, this kind of seems to break the iterator contract:

// InternalIterators accumulate errors encountered during operation, exposing
// them through the Error method. All of the absolute positioning methods
// reset any accumulated error before positioning. Relative positioning
// methods return without advancing if the iterator has accumulated an error.

There is likely higher-level code that relies on this. Before this change, getting an error during Init meant you have no iterator. With this change, a transient disk issue or one-off timeout could cause us to have an iterator that is essentially broken.

I think it would be safer to make all the absolute positioning methods try again. @jbowens WDYT?

[RaduBerinde]

Reviewable status: 0 of 12 files reviewed, 6 unresolved discussions (waiting on @jbowens, @sumeerbhola, and @xxmplus)

[xxmplus]

Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @sumeerbhola)

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

Previously, RaduBerinde wrote…

Actually, this kind of seems to break the iterator contract:

// InternalIterators accumulate errors encountered during operation, exposing
// them through the Error method. All of the absolute positioning methods
// reset any accumulated error before positioning. Relative positioning
// methods return without advancing if the iterator has accumulated an error.

There is likely higher-level code that relies on this. Before this change, getting an error during Init meant you have no iterator. With this change, a transient disk issue or one-off timeout could cause us to have an iterator that is essentially broken.

I think it would be safer to make all the absolute positioning methods try again. @jbowens WDYT?

oooh!! I knew nothing about this. let me revisit the code.

---

sstable/reader_iter_test.go line 5 at r10 (raw file):

Previously, RaduBerinde wrote…

This is unusual - what is the reason? Could we instead just if !invariants.Enabled { t.Skip() } the relevant test(s)?

this is unnecessary, I can remove it.

[xxmplus]

Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @sumeerbhola)

---

sstable/reader_iter_two_lvl.go line 103 at r10 (raw file):

Previously, RaduBerinde wrote…

blockIntersects would be the conservative choice here.. But we should never be calling this without having consulted an index block; I would just change it to if invariants.Enabled && !i.indexLoaded { panic() }

ack, good point.

[xxmplus]

Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @sumeerbhola)

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

Previously, xxmplus (Andy Xu) wrote…

oooh!! I knew nothing about this. let me revisit the code.

The indexLoadError and err feel clumsy. Let me try again.

---

sstable/reader_iter_two_lvl.go line 103 at r10 (raw file):

Previously, xxmplus (Andy Xu) wrote…

ack, good point.

I followed the code path again, and you are right. resolveMaybeExcluded() is called by loadSecondLevelIndexBlock() where it loads top level index block already. By the time it reaches resolveMaybeExcluded(), index block should have been loaded. Let me update the code.

[xxmplus]

Reviewable status: 0 of 12 files reviewed, 4 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @sumeerbhola)

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

Previously, xxmplus (Andy Xu) wrote…

The indexLoadError and err feel clumsy. Let me try again.

indexLoadError is removed now.

---

sstable/reader_iter_two_lvl.go line 103 at r10 (raw file):

Previously, xxmplus (Andy Xu) wrote…

I followed the code path again, and you are right. resolveMaybeExcluded() is called by loadSecondLevelIndexBlock() where it loads top level index block already. By the time it reaches resolveMaybeExcluded(), index block should have been loaded. Let me update the code.

updated.

[xxmplus]

Reviewable status: 0 of 12 files reviewed, 4 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @sumeerbhola)

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

Previously, xxmplus (Andy Xu) wrote…

indexLoadError is removed now.

All absolute positioning methods reset the err (existing code) so that retry is allowed.

[RaduBerinde]

Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @jbowens, @sumeerbhola, and @xxmplus)

---

sstable/reader_iter_single_lvl.go line 1595 at r12 (raw file):

func (i *singleLevelIterator[I, PI, D, PD]) ensureIndexLoaded() bool {
	if i.indexLoaded && i.err == nil {

Why i.err == nil here? Won't this mean that if we hit some other error, we'll reload the index? I think just doing if i.indexLoaded { return true } should work (and we can remove the indexLoaded = false below)

[jbowens]

Reviewed 2 of 10 files at r1, 3 of 7 files at r11, all commit messages.
Reviewable status: 5 of 12 files reviewed, 11 unresolved discussions (waiting on @RaduBerinde, @sumeerbhola, and @xxmplus)

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

Previously, xxmplus (Andy Xu) wrote…

All absolute positioning methods reset the err (existing code) so that retry is allowed.

i'm confused by this; why is it that we don't want this to be reset when the iterator is reset? the call to resetForReuse won't zero singleLevelIterator.index, but during Close() we'll call index.Close() which will zero its state.

---

sstable/reader_iter_single_lvl.go line 321 at r12 (raw file):

	if !i.ensureIndexLoaded() {
		return
	}

are there actually circumstances where we call initBounds and the index block is not loaded?

---

sstable/reader_iter_single_lvl.go line 351 at r12 (raw file):

	if !i.ensureIndexLoaded() {
		return
	}

are there actually circumstances where we call initBoundsForAlreadyLoadedBlock and the index block is not loaded?

---

sstable/reader_iter_single_lvl.go line 700 at r12 (raw file):

	if !i.ensureIndexLoaded() {
		return nil
	}

this is redundant with the above conditional right?

---

sstable/reader_iter_single_lvl.go line 757 at r12 (raw file):

		if !i.ensureIndexLoaded() {
			return nil
		}

is this also redundant? I don't see where we would've potentially cleared the loaded index block.

---

sstable/reader_iter_single_lvl.go line 1044 at r12 (raw file):

	var dontSeekWithinBlock bool
	if !i.ensureIndexLoaded() {

redundant with the call to ensureIndexLoaded a few lines above?

---

sstable/reader_iter_single_lvl.go line 1074 at r12 (raw file):

		// Intersects=false irrespective of the block props provided) during
		// seeks.
		if !i.ensureIndexLoaded() {

ditto, also redundant?

[xxmplus]

Reviewable status: 5 of 12 files reviewed, 11 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @sumeerbhola)

---

sstable/reader_iter_single_lvl.go line 184 at r6 (raw file):

Previously, jbowens (Jackson Owens) wrote…

i'm confused by this; why is it that we don't want this to be reset when the iterator is reset? the call to resetForReuse won't zero singleLevelIterator.index, but during Close() we'll call index.Close() which will zero its state.

ok, move indexLoaded above clearForResetBoundary so that it will be reset by resetForReuse()

---

sstable/reader_iter_single_lvl.go line 321 at r12 (raw file):

Previously, jbowens (Jackson Owens) wrote…

are there actually circumstances where we call initBounds and the index block is not loaded?

removed. and I went through the two-level iterator to remove unnecessary check as well.

---

sstable/reader_iter_single_lvl.go line 351 at r12 (raw file):

Previously, jbowens (Jackson Owens) wrote…

are there actually circumstances where we call initBoundsForAlreadyLoadedBlock and the index block is not loaded?

removed.

---

sstable/reader_iter_single_lvl.go line 700 at r12 (raw file):

Previously, jbowens (Jackson Owens) wrote…

this is redundant with the above conditional right?

removed.

---

sstable/reader_iter_single_lvl.go line 757 at r12 (raw file):

Previously, jbowens (Jackson Owens) wrote…

is this also redundant? I don't see where we would've potentially cleared the loaded index block.

removed.

---

sstable/reader_iter_single_lvl.go line 1044 at r12 (raw file):

Previously, jbowens (Jackson Owens) wrote…

redundant with the call to ensureIndexLoaded a few lines above?

removed.

---

sstable/reader_iter_single_lvl.go line 1074 at r12 (raw file):

Previously, jbowens (Jackson Owens) wrote…

ditto, also redundant?

removed.

---

sstable/reader_iter_single_lvl.go line 1595 at r12 (raw file):

Previously, RaduBerinde wrote…

Why i.err == nil here? Won't this mean that if we hit some other error, we'll reload the index? I think just doing if i.indexLoaded { return true } should work (and we can remove the indexLoaded = false below)

you are right.

[RaduBerinde]

Reviewable status: 5 of 12 files reviewed, 11 unresolved discussions (waiting on @jbowens, @sumeerbhola, and @xxmplus)

[jbowens]

Reviewable status: 5 of 12 files reviewed, 15 unresolved discussions (waiting on @RaduBerinde, @sumeerbhola, and @xxmplus)

---

sstable/reader_iter_test.go line 539 at r14 (raw file):

			}
		case 3:
			iter.Next()

It's not legal to perform a Next on an iterator that hasn't been positioned, so I think we need to adjust the test to first perform a iter.First() or something, or make the Next conditional on having already performed a seek.

---

sstable/reader_iter_single_lvl.go line 444 at r14 (raw file):

		PD(&i.data).Invalidate()
		return loadBlockFailed
	}

This should be unnecessary; the caller is required to have positioned the index iterator already so we necessarily must have already loaded the block if the caller is obeying the contract. We could make it an assertion instead:

	if !i.indexLoaded {
		i.err = errors.AssertionFailedf("index block is not loaded")
		return loadBlockFailed
	}

---

sstable/reader_iter_single_lvl.go line 1290 at r14 (raw file):

	if !i.ensureIndexLoaded() {
		return nil
	}

This is also unnecessary in the relative positioning operations (NextPrefix,Next, Prev), because the iterator must've already been seeked. Otherwise it's undefined where we end up, since there is no current position to move relative to. We can make these calls to ensureIndexLoaded in NextPrefix, Next and Prev assertion failures too:

		if !i.indexLoaded {
			i.err = errors.AssertionFailedf("index block is not loaded")
			return nil
		}

---

sstable/reader_iter_single_lvl.go line 1445 at r14 (raw file):

		if !i.ensureIndexLoaded() {
			return nil
		}

ditto here and skipForward—I think these should be assertions.

---

sstable/reader_iter_two_lvl.go line 857 at r14 (raw file):

	if !i.ensureTopLevelIndexLoaded() {
		return nil
	}

ditto for the twoLevelIterator--I think

• the relative positioning calls should assert the top-level index has already been loaded (and in this case we should lift the assertion up to above the call to i.secondLevel.NextPrefix.)
• the skipForward and skipBackward funcs can similarly assert

[xxmplus]

Reviewable status: 5 of 12 files reviewed, 15 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @sumeerbhola)

---

sstable/reader_iter_single_lvl.go line 444 at r14 (raw file):

Previously, jbowens (Jackson Owens) wrote…

This should be unnecessary; the caller is required to have positioned the index iterator already so we necessarily must have already loaded the block if the caller is obeying the contract. We could make it an assertion instead:

	if !i.indexLoaded {
		i.err = errors.AssertionFailedf("index block is not loaded")
		return loadBlockFailed
	}

updated.

---

sstable/reader_iter_single_lvl.go line 1290 at r14 (raw file):

Previously, jbowens (Jackson Owens) wrote…

This is also unnecessary in the relative positioning operations (NextPrefix,Next, Prev), because the iterator must've already been seeked. Otherwise it's undefined where we end up, since there is no current position to move relative to. We can make these calls to ensureIndexLoaded in NextPrefix, Next and Prev assertion failures too:

		if !i.indexLoaded {
			i.err = errors.AssertionFailedf("index block is not loaded")
			return nil
		}

updated.

---

sstable/reader_iter_single_lvl.go line 1445 at r14 (raw file):

Previously, jbowens (Jackson Owens) wrote…

ditto here and skipForward—I think these should be assertions.

updated.

---

sstable/reader_iter_test.go line 539 at r14 (raw file):

Previously, jbowens (Jackson Owens) wrote…

It's not legal to perform a Next on an iterator that hasn't been positioned, so I think we need to adjust the test to first perform a iter.First() or something, or make the Next conditional on having already performed a seek.

updated.

[jbowens]

@jbowens reviewed all commit messages.
Reviewable status: 5 of 13 files reviewed, 15 unresolved discussions (waiting on @RaduBerinde, @sumeerbhola, and @xxmplus)

[xxmplus]

TFTR!