storage: use SetOptions() when reusing Pebble iterators

[erikgrinaker]

Note to reviewers: this is for the mvcc-range-tombstones feature branch.

---

storage: remove MVCCIterator.SetUpperBound and tweak ClearIterRange

This patch removes the MVCCIterator.SetUpperBound() method, which was
only used by MVCCIterator.ClearIterRange().

ClearIterRange now also creates the iterator itself, instead of having
it passed in. This ensures the iterator is created with appropriate
bounds and options, which becomes even more important with the
introduction of range keys. If further flexibility is needed, we can
consider passing an IterOptions struct instead of a key span.

Release note: None

storage: handle multiple iterators in pebbleReadOnly and pebbleBatch

Previously, pebbleReadOnly and pebbleBatch only supported a single
concurrent iterator of a given type. Attempts to create an additional
iterator would panic with "iterator already in use".

This is particularly problematic because a subsequent change will make
more iterators reusable (specifically TBI iterators), such that e.g.
MVCCIncrementalIterator will be creating two concurrent reusable
iterators and trigger this panic.

This patch supports multiple iterators by creating new cloned iterators
if the existing iterator is in use. This preserves the pinned engine
state for the cloned iterators too.

Release note: None

storage: use SetOptions() when reusing Pebble iterators

Iterator reuse now relies on Pebble.SetOptions() to configure the
reused Pebble iterator. This allows a wider range of iterators to be
reused, since previously only the bounds could be changed on existing
iterators.

Release note: None

Jira issue: CRDB-14701

[cockroach-teamcity]

This change is 

[erikgrinaker]

@jbowens Note the removed pebbleIterator.setBounds() optimization that avoided calling Iterator.SetBounds() if the bounds did not change. I think we'll need to move this kind of optimization down into SetOptions() -- some options changes may not need the iterator to be repositioned or invalidated, but only Pebble can control that since we're not able to change individual options.

[erikgrinaker]

@jbowens I'm getting a test failure here (TestEngineTimeBound) that I think is caused by SetOptions() not replacing TableFilter. I'm essentially creating an iterator with a TableFilter set, and then cloning this iterator and calling SetOptions() on it with a new TableFilter. But it seems like the old TableFilter closure gets called on the cloned iterator. Does that check out?

[nicktrav]

Did a pass and . Would be good for either Jackson or Sumeer to give it a thumbs-up too.

Reviewed 16 of 16 files at r1, 3 of 3 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aliher1911, @jbowens, and @sumeerbhola)

[jbowens]

@jbowens Note the removed pebbleIterator.setBounds() optimization that avoided calling Iterator.SetBounds() if the bounds did not change. I think we'll need to move this kind of optimization down into SetOptions() -- some options changes may not need the iterator to be repositioned or invalidated, but only Pebble can control that since we're not able to change individual options.

All changes require the Iterator to be repositioned (including changes to masking, key types, bounds). I don't think it's possible to move this optimization down into Pebble. Check out this comment: https://github.com/cockroachdb/pebble/blob/c8e7d4149ada9f4a06d8ae44b04b25618019bc4e/iterator.go#L1672-L1681

@jbowens I'm getting a test failure here (TestEngineTimeBound) that I think is caused by SetOptions() not replacing TableFilter. I'm essentially creating an iterator with a TableFilter set, and then cloning this iterator and calling SetOptions() on it with a new TableFilter. But it seems like the old TableFilter closure gets called on the cloned iterator. Does that check out?

Sorry, this is my mistake. I overlooked this. Filed (cockroachdb/pebble#1621) I'm not sure what we can do for the TableFilter since it's just a closure, and we cannot determine whether or not the closure changed. We can (and should) detect whether the block property filters changed. Now that we have block property filters in 22.1, do we need to use TableFilter? Block property filters are strictly more powerful and also allow filtering out whole tables. Maybe we can't because sstables were created in 21.2 that still exist in 22.2 would not be subject to filtering, since they wouldn't have block property annotations.

Reviewed 1 of 16 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aliher1911 and @sumeerbhola)

[erikgrinaker]

All changes require the Iterator to be repositioned (including changes to masking, key types, bounds). I don't think it's possible to move this optimization down into Pebble. Check out this comment: https://github.com/cockroachdb/pebble/blob/c8e7d4149ada9f4a06d8ae44b04b25618019bc4e/iterator.go#L1672-L1681

I see, thanks. I'll run some preliminary benchmarks to see what effect this has, and what can be done about it.

Sorry, this is my mistake. I overlooked this. Filed (cockroachdb/pebble#1621) I'm not sure what we can do for the TableFilter since it's just a closure, and we cannot determine whether or not the closure changed.

Why do we need to compare? Can't we unconditionally replace the existing table filter? If the concern is tearing down the iterator stacks then I suppose a nil check would suffice? In the majority of cases this will be nil.

Now that we have block property filters in 22.1, do we need to use TableFilter? Block property filters are strictly more powerful and also allow filtering out whole tables. Maybe we can't because sstables were created in 21.2 that still exist in 22.2 would not be subject to filtering, since they wouldn't have block property annotations.

I suppose the only option here would be a migration that rewrites the properties. I guess it's too late in the game to get that into 22.1? Seems unfortunate to have to maintain different code paths in 22.2 for this.

Worst case, we can just prevent reuse of time-bound iterators -- that's what we do today anyway.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aliher1911 and @sumeerbhola)

[jbowens]

Love the code deletion. I'm not sure how important the set bounds optimization is, but we should make sure to circle back on that (thanks for recording the TODO).

If the concern is tearing down the iterator stacks then I suppose a nil check would suffice? In the majority of cases this will be nil.

Sounds good. SetOptions will now [just merged in Pebble] replace the iterator stack if filters are non-nil before or after the SetOptions call.

I suppose the only option here would be a migration that rewrites the properties. I guess it's too late in the game to get that into 22.1? Seems unfortunate to have to maintain different code paths in 22.2 for this.

Yeah, a migration would also be intense. Block property filters are new in 22.1, so this migration would effectively need to rewrite every sstable in the store. I don't think we'd want to block 22.1 cluster version finalization on that, in which case we'd need to support the different code path in 22.2 anyways. We could do the blocking migration in 22.2 with the expectation that most of the sstables within a store should've been written by 22.1. A quick 21.2->22.1->22.2 upgrade sequence would still need to rewrite almost the entirety of its stores though.

Reviewed 3 of 16 files at r1, 1 of 3 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @aliher1911 and @sumeerbhola)

[erikgrinaker]

If the concern is tearing down the iterator stacks then I suppose a nil check would suffice? In the majority of cases this will be nil.

Sounds good. SetOptions will now [just merged in Pebble] replace the iterator stack if filters are non-nil before or after the SetOptions call.

Awesome, thanks for the quick turnaround! Tests pass now, but I'm going to run a few benchmarks before merging.

Yeah, a migration would also be intense. Block property filters are new in 22.1, so this migration would effectively need to rewrite every sstable in the store. I don't think we'd want to block 22.1 cluster version finalization on that, in which case we'd need to support the different code path in 22.2 anyways. We could do the blocking migration in 22.2 with the expectation that most of the sstables within a store should've been written by 22.1. A quick 21.2->22.1->22.2 upgrade sequence would still need to rewrite almost the entirety of its stores though.

Good point, probably better to wait for 22.2 (or later). This will work for now.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @aliher1911, @jbowens, @nicktrav, and @sumeerbhola)

[erikgrinaker]

I'm going to run a few benchmarks before merging.

We're taking an ~8% hit on MVCC gets, using a batch which clones the batch iterator on every get. This is almost entirely due to the removed setOptions() optimization. It's probably more than we can get away with merging to master, so I'll see if I can claw it back somehow.

name                                        old time/op    new time/op    delta
MVCCGet_Pebble/versions=1/valueSize=8-24      2.60µs ± 1%    2.83µs ± 1%  +8.91%  (p=0.000 n=9+10)
MVCCGet_Pebble/versions=10/valueSize=8-24     3.90µs ± 0%    4.23µs ± 1%  +8.46%  (p=0.000 n=7+9)
MVCCGet_Pebble/versions=100/valueSize=8-24    10.2µs ± 4%    10.5µs ± 2%  +3.33%  (p=0.003 n=10+10)

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @aliher1911, @jbowens, @nicktrav, and @sumeerbhola)

[erikgrinaker]

Extended the setBounds() check to cover all options, which didn't turn out too bad. Got the performance diff down to about 2%:

name                                        old time/op    new time/op    delta
MVCCGet_Pebble/versions=1/valueSize=8-24      2.59µs ± 1%    2.63µs ± 1%  +1.52%  (p=0.000 n=18+20)
MVCCGet_Pebble/versions=10/valueSize=8-24     3.91µs ± 1%    4.00µs ± 1%  +2.42%  (p=0.000 n=18+19)
MVCCGet_Pebble/versions=100/valueSize=8-24    9.77µs ± 5%    9.95µs ± 5%  +1.78%  (p=0.012 n=19+19)

I think that's acceptable -- this is in the tens of nanoseconds, so I don't think it will show up on end-to-end benchmarks. We'll have to do another optimization pass once MVCC range tombstones are fully implemented anyway.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @aliher1911, @jbowens, @nicktrav, and @sumeerbhola)

[jbowens]

Nice

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @aliher1911, @jbowens, @nicktrav, and @sumeerbhola)

[nicktrav]

Reviewed the latest set of changes -

Reviewed 3 of 8 files at r4, 1 of 3 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, and @sumeerbhola)

---

pkg/storage/pebble_iterator.go, line 149 at r6 (raw file):

	// We don't generate a pebble.Options for comparison, because we want to reuse
	// the byte slices of the existing p.options for any key encoding.
	optsChanged := p.options.OnlyReadGuaranteedDurable != (durability == GuaranteedDurability) ||

Is there a risk of introducing "brittleness" here in that we add some new options and they aren't taken into account here? Given this is just a performance optimization rather than a correctness issue, it's probably fine, just wanted to mention it.

[erikgrinaker]

Closed the performance gap by rearranging the predicates.

name                                        old time/op    new time/op    delta
MVCCGet_Pebble/versions=1/valueSize=8-24      2.59µs ± 1%    2.60µs ± 1%  +0.42%  (p=0.013 n=18+19)
MVCCGet_Pebble/versions=10/valueSize=8-24     3.91µs ± 1%    3.91µs ± 1%    ~     (p=0.597 n=18+20)
MVCCGet_Pebble/versions=100/valueSize=8-24    9.77µs ± 5%    9.41µs ± 4%  -3.77%  (p=0.000 n=19+20)

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @aliher1911, @nicktrav, and @sumeerbhola)

---

pkg/storage/pebble_iterator.go, line 149 at r6 (raw file):

Previously, nicktrav (Nick Travers) wrote…

Is there a risk of introducing "brittleness" here in that we add some new options and they aren't taken into account here? Given this is just a performance optimization rather than a correctness issue, it's probably fine, just wanted to mention it.

Yes, that's a very real risk here. And it's true that this is "just" a performance optimization, but forgetting to handle a new option here can cause that option to not take effect if we take the fast path.

However, every option that's passed down to a Pebble iterator must have logic in this function to translate from a CRDB IterOptions to a Pebble IterOptions, so the hope is that someone adding logic for an option here will see this condition and update it. That's probably sufficient?

[nicktrav]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @aliher1911, @erikgrinaker, and @sumeerbhola)

---

pkg/storage/pebble_iterator.go, line 149 at r6 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

Yes, that's a very real risk here. And it's true that this is "just" a performance optimization, but forgetting to handle a new option here can cause that option to not take effect if we take the fast path.

However, every option that's passed down to a Pebble iterator must have logic in this function to translate from a CRDB IterOptions to a Pebble IterOptions, so the hope is that someone adding logic for an option here will see this condition and update it. That's probably sufficient?

👍

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

---

-- commits, line 25 at r5:
Can you elaborate on this statement about MVCCIncrementalIterator? Won't we continue to have one tbi that sees both point and range keys and another regular iter that also sees both kinds of keys?

[erikgrinaker]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

---

-- commits, line 25 at r5:

Previously, sumeerbhola wrote…

Can you elaborate on this statement about MVCCIncrementalIterator? Won't we continue to have one tbi that sees both point and range keys and another regular iter that also sees both kinds of keys?

Yes, that's right. There will be no changes to MVCCIncrementalIterator in that regard.

This comment refers to the subsequent SetOptions() commit in this PR. Previously, TBIs were not reusable because we had no way to change the TableFilter on an existing iterator. But now that we have SetOptions() we can reuse iterators for TBIs.

However, pebbleBatch and pebbleReadOnly did not support creating multiple concurrent reusable iterators, and would panic with "iterator already in use" because of this check here:

https://github.com/cockroachdb/cockroach/blob/master/pkg/storage/pebble_batch.go#L231-L233

MVCCIncrementalIterator needs to create two concurrent iterators (one regular and one TBI). This used to work as a side-effect of TBIs not being reusable, since it bypassed that check in pebbleBatch and created an entirely new iterator. But now that TBIs are reusable, it triggers that assertion.

This patch removes this artificial restriction of only allowing a single concurrent iterator, by cloning a new iterator if the primary one is already in use.