Fix memtable-only iterator regression

[cbi42]

Summary: when there is a single memtable without range tombstones and no SST files in the database, DBIter should wrap memtable iterator directly. Currently we create a merging iterator on top of the memtable iterator, and have DBIter wrap around it. This causes iterator regression and this PR fixes this issue.

Test plan:

• make check
• Performance:
  • Set up: ./db_bench -benchmarks=filluniquerandom -write_buffer_size=$((1 << 30)) -num=10000
  • Benchmark: ./db_bench -benchmarks=seekrandom -use_existing_db=true -avoid_flush_during_recovery=true -write_buffer_size=$((1 << 30)) -num=10000 -threads=16 -duration=60 -seek_nexts=$seek_nexts

seek_nexts    main op/sec    #10705      RocksDB v7.6
0             5746568        5749033     5786180 
30            2411690        3006466     2837699
1000          102556         128902      124667

[facebook-github-bot]

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ajkr]

The assumptions on the caller are reasonable but adding up to be somewhat complicated IMO.

What if we add bool ignore_range_deletions to the MergeIteratorBuilder constructor, and then have a common SwitchToMergingIterator() function that does everything when needed - create the MergingIterator, add first_iter as first point key iterator, add nullptr as first range del iterator (when !ignore_range_deletions AND we aren't switching due to non-nullptr AddMemtableRangeTombstoneIterator())?

[ajkr]

Perhaps a better design is having a single AddComponentIterators() function that takes both point and range iterator pointers. It could internally decide when to upgrade to MergingIterator (upon first call when range tombstone iter is non-null or second call otherwise). It could also have some logic to only call AddRangeTombstoneIterator() upon the first non-nullptr range iterator provided - first calling AddRangeTombstoneIterator(nullptr) for all the previous components for which we have not yet added a range tombstone iterator.

[cbi42]

Thanks for the review. I agree with the complexity involved in MergeIteratorBuilder, and updated the API to AddPointAndTombstoneIterator that adds a pair of point and range tombstone iters at a time. I did not consolidate AddPointAndTombstoneIterator and AddIterator yet to separate use cases that may or may not involve range tombstones. I'm thinking to consolidate them (or decide on not to consolidate them) as a next step after this PR.

[ajkr]

I think improving the design of the MergeIteratorBuilder would be nice to have, though we can still land this if there's no easy way right now

[ajkr]

Perhaps a better design is having a single AddComponentIterators() function that takes both point and range iterator pointers. It could internally decide when to upgrade to MergingIterator (upon first call when range tombstone iter is non-null or second call otherwise). It could also have some logic to only call AddRangeTombstoneIterator() upon the first non-nullptr range iterator provided - first calling AddRangeTombstoneIterator(nullptr) for all the previous components for which we have not yet added a range tombstone iterator.

[facebook-github-bot]

@cbi42 has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ajkr]

What about delete *memtable_range_tombstone_iter_? Could *memtable_range_tombstone_iter_ have been non-null if the SV changed after the sv_number_ != cur_sv_number check picked the else branch, and the mutable memtable in the old SV had range tombstones while the mutable memtable in the new SV does not?

[cbi42]

Yes this could happen. Thanks for catching this, fixing it in #10716