engine: Fix case where a pebbleBatchIterator is passed to ClearIterRange

[itsbilal]

Currently ClearIterRange expects a pebbleIterator to be passed in
as the iterator. This isn't always the case; occasionally, we see
a pebbleBatchIterator which currently errors out the method. Update
that method in pebble and pebbleBatch to accept both types of
iterators.

Release note: None

[cockroach-teamcity]

This change is 

[itsbilal]

Quick fix for an error observed on a cluster I ran for ~36h. This code path isn't in active use until #41452 and #41453 land anyway, but best to correct it soon while we're in the midst of reviewing all of these. Should probably write a simple pebble-specific test to exercise this case.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)

---

pkg/storage/engine/pebble.go, line 286 at r1 (raw file):

		pebbleIter = i.iter
	case *pebbleBatchIterator:
		pebbleIter = i.iter

The type-switch is a bit of a code smell. Perhaps add a pebbleIter() to the various pebble*Iterator types. Then you can do something like:

type pebbleIterGetter interface { pebbleIter() *pebble.Iterator }
pebbleIter := iter.(pebbleIterGetter).pebbleIter()

Or perhaps better is to add a unsafeRawKey() []byte method to the various pebble*Iterator classes as it looks like that is why you're trying to get access to the *pebble.Iterator.

---

pkg/storage/engine/pebble.go, line 306 at r1 (raw file):

		}

		err = p.db.Delete(pebbleIter.Key(), pebble.Sync)

This passed my attention last time, but synchronously deleting each key is going to be horribly slow. What we want to do is build up a batch of deletes and commit the batch when it gets too large.

---

pkg/storage/engine/pebble_batch.go, line 200 at r1 (raw file):

	}

	var pebbleIter *pebble.Iterator

This looks like a duplicate of the other clear range code. I think it could be unified.

---

pkg/storage/engine/pebble_batch.go, line 216 at r1 (raw file):

	iter.SetUpperBound(end.Key)

	for ; ; iter.Next() {

Are you missing a call to iter.Seek() here?

[itsbilal]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajkr, @petermattis, and @sumeerbhola)

---

pkg/storage/engine/pebble.go, line 286 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The type-switch is a bit of a code smell. Perhaps add a pebbleIter() to the various pebble*Iterator types. Then you can do something like:

type pebbleIterGetter interface { pebbleIter() *pebble.Iterator }
pebbleIter := iter.(pebbleIterGetter).pebbleIter()

Or perhaps better is to add a unsafeRawKey() []byte method to the various pebble*Iterator classes as it looks like that is why you're trying to get access to the *pebble.Iterator.

Did something similar, added an unsafeRawKey and an inline interface to be able to call it. Thanks!

---

pkg/storage/engine/pebble.go, line 306 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This passed my attention last time, but synchronously deleting each key is going to be horribly slow. What we want to do is build up a batch of deletes and commit the batch when it gets too large.

Done. This also lets us reuse pebbleBatch.ClearIterRange since we can just create a new batch, call ClearIterRange on it, then commit it.

---

pkg/storage/engine/pebble_batch.go, line 200 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This looks like a duplicate of the other clear range code. I think it could be unified.

Done.

---

pkg/storage/engine/pebble_batch.go, line 216 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Are you missing a call to iter.Seek() here?

Oops, good catch. Fixed.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajkr, @itsbilal, and @sumeerbhola)

---

pkg/storage/engine/pebble.go, line 285 at r2 (raw file):

	defer batch.Close()

	err := batch.ClearIterRange(iter, start, end)

Nit: if err := batch.ClearIterRange(...); err != nil { ... }

---

pkg/storage/engine/pebble_iterator.go, line 164 at r2 (raw file):

// unsafeRawKey returns the raw key from the underlying pebble.Iterator.
func (p *pebbleIterator) unsafeRawKey() []byte {
	if valid, err := p.Valid(); err != nil || !valid {

I suspect this isn't necessary and you can just call p.iter.Key(). Perhaps add a TODO to investigate.

[itsbilal]

Thanks for the review! Will merge on green CI.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajkr, @petermattis, and @sumeerbhola)

---

pkg/storage/engine/pebble_iterator.go, line 164 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I suspect this isn't necessary and you can just call p.iter.Key(). Perhaps add a TODO to investigate.

It's pretty unnecessary in this case since ClearIterRange does its own validity checks before calling this. Removed.

[itsbilal]

bors r+

[itsbilal]

PR fell off the bors list for some reason.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)