Fix handling of out-of-range scan option#13995
Conversation
db/version_set.cc
Outdated
| file_to_arg.second.io_coalesce_threshold = so->io_coalesce_threshold; | ||
| file_to_arg.second.max_prefetch_size = so->max_prefetch_size; | ||
| file_to_arg.second.use_async_io = so->use_async_io; | ||
| file_to_arg.second.require_file_overlap = true; |
There was a problem hiding this comment.
Is it possible to group the properties that needs to propagate down to each file together? In addition, since all of them are const, maybe we could just use a const pointer to it? Is there any lifetime concern?
struct MultiScanOptions {
io_coalesce_threshold;
max_prefetch_size;
};
struct MultiScanArgs {
MultiScanOptions options;
};
for (auto& file_to_arg : *file_to_scan_opts_) {
file_to_arg.options = so->options;
}
include/rocksdb/options.h
Outdated
| // Internal use only. | ||
| // Fail the Prepare() on a file if a scan range does not overlap | ||
| // with the file. | ||
| bool require_file_overlap = false; |
There was a problem hiding this comment.
I guess we expect this value is always set to false when caller pass it to RocksDB external API? Meantime, since this is exposed to caller, caller could set it by mistake. Probably, we should set it to false, when RocksDB received it. However, there might be many places we need to update, which is hard to maintain and enforce. Maybe it is better to move it to private and declare a the classes that need to access it as friends.
There was a problem hiding this comment.
Update to private field.
| ; | ||
| // If require_file_overlap is set, then the scan ranges for this file | ||
| // must intersect with the file. Otherwise, allow empty intersection. | ||
| if (multiscan_opts->require_file_overlap) { |
There was a problem hiding this comment.
If the only usage of multiscan_opts is to get require_file_overlap, why not just pass require_file_overlap itself?
There was a problem hiding this comment.
Other fields of multiscan_opts are also used. To pass require_file_overlap by itself, I'll have to change Prepare() signature for the iterator interface which is a bigger change.
There was a problem hiding this comment.
I think his comment is just about CollectBlockHandles
There was a problem hiding this comment.
Updated CollectBlockHandles()
anand1976
left a comment
anand1976
left a comment
There was a problem hiding this comment.
LGTM. Would be good to add a test with L0/non-L0 files to verify the end to end works as expected.
Thanks for the review, I've added a test with both L0 and non-L0 files which fails before this PR. |
Summary: currently BlockBasedTableIterator::Prepare() fails the iterator with non-ok status if an out-of-range scan option is detected. This is due to the interaction between LevelIterator and BlockBasedTableIterator, see added comment above BlockBasedTableIterator::Prepare(). This can fail stress test for L0 files since it doesn't use LevelIterator and scan options are not pruned. This PR fixes this by adding an internal option to MultiScanArgs that enables this check. Pull Request resolved: facebook#13995 Test Plan: - new unit test - stress test that fails before this pr: `python3 -u ./tools/db_crashtest.py whitebox --iterpercent=60 --prefix_size=-1 --prefixpercent=0 --readpercent=0 --test_batches_snapshots=0 --use_multiscan=1 --read_fault_one_in=0 --kill_random_test=88888 --interval=60 --multiscan_use_async_io=0 --mmap_read=0 --level0_file_num_compaction_trigger=20` Reviewed By: anand1976 Differential Revision: D83166088 Pulled By: cbi42 fbshipit-source-id: 241a7d43c8c00d9a98eea0cabb03d2174d51aae5
4 participants
Summary: currently BlockBasedTableIterator::Prepare() fails the iterator with non-ok status if an out-of-range scan option is detected. This is due to the interaction between LevelIterator and BlockBasedTableIterator, see added comment above BlockBasedTableIterator::Prepare(). This can fail stress test for L0 files since it doesn't use LevelIterator and scan options are not pruned. This PR fixes this by adding an internal option to MultiScanArgs that enables this check.
Test plan:
python3 -u ./tools/db_crashtest.py whitebox --iterpercent=60 --prefix_size=-1 --prefixpercent=0 --readpercent=0 --test_batches_snapshots=0 --use_multiscan=1 --read_fault_one_in=0 --kill_random_test=88888 --interval=60 --multiscan_use_async_io=0 --mmap_read=0 --level0_file_num_compaction_trigger=20