dolysis
/
rocksdb
mirror of https://github.com/facebook/rocksdb.git
3
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Propagate more ReadOptions to ApproximateOffsetOf/Sizes (#12764) 

Summary:
Unknown why these would ignore options like deadline and read_tier. Setting total_order_seek=true is unnecessary because of the disable_prefix_seek (= true) parameter to NewIndexIterator. This is only used by the hash index, which uses total order seek if either the ReadOption or the parameter is true. The parameter is arguably redundant with the total_order_seek option, meaning it could be eliminated, but I think this case is exceptional (compared to e.g. no_io):
* Prefix seek is particular to user iterators, though might be usable, carefully, for other read operations.
* The historical default of total_order_seek=false in a sense is "wrong result by default" so cannot be interpreted as an intent to force prefix seek in an operation for which it might be usual or give bad results.

Also added a generic release note to cover this and related PRs.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/12764

Test Plan: existing tests

Reviewed By: hx235

Differential Revision: D58474240

Pulled By: pdillinger

fbshipit-source-id: 79014d9822ba8f09d57ce4524363aa0973017b68
This commit is contained in:
Peter Dillinger 2024-06-12 16:25:47 -07:00 committed by Facebook GitHub Bot
parent 5cf3bed00f
commit 3abcba8470
2 changed files with 5 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
table/block_based/block_based_table_reader.cc
View File

@ -633,6 +633,8 @@ Status BlockBasedTable::Open(
// From read_options, retain deadline, io_timeout, rate_limiter_priority, and // From read_options, retain deadline, io_timeout, rate_limiter_priority, and
// verify_checksums. In future, we may retain more options. // verify_checksums. In future, we may retain more options.
// TODO: audit more ReadOptions and do this in a way that brings attention
// on new ReadOptions?
ReadOptions ro; ReadOptions ro;
ro.deadline = read_options.deadline; ro.deadline = read_options.deadline;
ro.io_timeout = read_options.io_timeout; ro.io_timeout = read_options.io_timeout;
@ -2857,11 +2859,8 @@ uint64_t BlockBasedTable::ApproximateOffsetOf(const ReadOptions& read_options,
BlockCacheLookupContext context(caller); BlockCacheLookupContext context(caller);
IndexBlockIter iiter_on_stack; IndexBlockIter iiter_on_stack;
ReadOptions ro;
ro.total_order_seek = true;
ro.io_activity = read_options.io_activity;
auto index_iter = auto index_iter =
NewIndexIterator(ro, /*disable_prefix_seek=*/true, NewIndexIterator(read_options, /*disable_prefix_seek=*/true,
/*input_iter=*/&iiter_on_stack, /*get_context=*/nullptr, /*input_iter=*/&iiter_on_stack, /*get_context=*/nullptr,
/*lookup_context=*/&context); /*lookup_context=*/&context);
std::unique_ptr<InternalIteratorBase<IndexValue>> iiter_unique_ptr; std::unique_ptr<InternalIteratorBase<IndexValue>> iiter_unique_ptr;
@ -2904,11 +2903,8 @@ uint64_t BlockBasedTable::ApproximateSize(const ReadOptions& read_options,
BlockCacheLookupContext context(caller); BlockCacheLookupContext context(caller);
IndexBlockIter iiter_on_stack; IndexBlockIter iiter_on_stack;
ReadOptions ro;
ro.total_order_seek = true;
ro.io_activity = read_options.io_activity;
auto index_iter = auto index_iter =
NewIndexIterator(ro, /*disable_prefix_seek=*/true, NewIndexIterator(read_options, /*disable_prefix_seek=*/true,
/*input_iter=*/&iiter_on_stack, /*get_context=*/nullptr, /*input_iter=*/&iiter_on_stack, /*get_context=*/nullptr,
/*lookup_context=*/&context); /*lookup_context=*/&context);
std::unique_ptr<InternalIteratorBase<IndexValue>> iiter_unique_ptr; std::unique_ptr<InternalIteratorBase<IndexValue>> iiter_unique_ptr;

1
unreleased_history/bug_fixes/read_options.md Normal file
View File

@ -0,0 +1 @@
* Various read operations could ignore various ReadOptions that might be relevant. Fixed many such cases, which can result in behavior change but a better reflection of specified options.