Skip to content

Commit f49d76b

Browse files
cbi42facebook-github-bot
cbi42
authored and
facebook-github-bot
committed
Clarify that memtable_op_scan_flush_trigger does not support tailing iterator (facebook#13586)
Summary: clarify in comments and fix one implementation under NewIterator where option `memtable_op_scan_flush_trigger` does not work correctly with tailing iterator yet. This is because tailing iterator can rebuild iterator internally which reads from a newer memtable, and DBIter's reference to active memtable needs to be refreshed. This PR clarifies that `memtable_op_scan_flush_trigger` will have no effect on tailing iterator. We can add the support in the future if needed. Pull Request resolved: facebook#13586 Test Plan: existing tests. Reviewed By: jaykorean Differential Revision: D74108099 Pulled By: cbi42 fbshipit-source-id: 7c6608485d57755abc44f3be0b3c5d82a7bc5ca9
1 parent 1428e95 commit f49d76b

File tree

7 files changed

+104
-97
lines changed

7 files changed

+104
-97
lines changed

‎db/arena_wrapped_db_iter.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ void ArenaWrappedDBIter::Init(
5555
db_iter_ = DBIter::NewIter(
5656
env, read_options_, ioptions, mutable_cf_options,
5757
ioptions.user_comparator, /*internal_iter=*/nullptr, version, sequence,
58-
read_callback, cfh, expose_blob_index, active_mem, &arena_);
58+
read_callback, active_mem, cfh, expose_blob_index, &arena_);
5959

6060
sv_number_ = version_number;
6161
allow_refresh_ = allow_refresh;

‎db/db_impl/db_impl.cc

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3886,12 +3886,14 @@ Iterator* DBImpl::NewIterator(const ReadOptions& _read_options,
38863886

38873887
auto iter = new ForwardIterator(this, read_options, cfd, sv,
38883888
/* allow_unprepared_value */ true);
3889+
// TODO(cbi): Add support for `memtable_op_scan_flush_trigger` for tailing
3890+
// iterator. This requires refreshing DBIter's pointer to active_mem when
3891+
// tailing iterator refreshes to new memtable internally.
38893892
result = DBIter::NewIter(env_, read_options, cfd->ioptions(),
38903893
sv->mutable_cf_options, cfd->user_comparator(),
38913894
iter, sv->current, kMaxSequenceNumber,
3892-
/*read_callback=*/nullptr, cfh,
3893-
/*expose_blob_index=*/false,
3894-
/*active_mem=*/sv->mem);
3895+
/*read_callback=*/nullptr, /*active_mem=*/nullptr,
3896+
cfh, /*expose_blob_index=*/false);
38953897
} else {
38963898
// Note: no need to consider the special case of
38973899
// last_seq_same_as_publish_seq_==false since NewIterator is overridden in
@@ -4095,12 +4097,12 @@ Status DBImpl::NewIterators(
40954097
auto iter = new ForwardIterator(this, read_options, cf_sv_pair.cfd,
40964098
cf_sv_pair.super_version,
40974099
/* allow_unprepared_value */ true);
4098-
iterators->push_back(
4099-
DBIter::NewIter(env_, read_options, cf_sv_pair.cfd->ioptions(),
4100-
cf_sv_pair.super_version->mutable_cf_options,
4101-
cf_sv_pair.cfd->user_comparator(), iter,
4102-
cf_sv_pair.super_version->current, kMaxSequenceNumber,
4103-
nullptr /*read_callback*/, cf_sv_pair.cfh));
4100+
iterators->push_back(DBIter::NewIter(
4101+
env_, read_options, cf_sv_pair.cfd->ioptions(),
4102+
cf_sv_pair.super_version->mutable_cf_options,
4103+
cf_sv_pair.cfd->user_comparator(), iter,
4104+
cf_sv_pair.super_version->current, kMaxSequenceNumber,
4105+
nullptr /*read_callback*/, /*active_mem=*/nullptr, cf_sv_pair.cfh));
41044106
}
41054107
} else {
41064108
for (const auto& cf_sv_pair : cf_sv_pairs) {

‎db/db_iter.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,9 @@ class DBIter final : public Iterator {
7171
InternalIterator* internal_iter,
7272
const Version* version, const SequenceNumber& sequence,
7373
ReadCallback* read_callback,
74+
ReadOnlyMemTable* active_mem,
7475
ColumnFamilyHandleImpl* cfh = nullptr,
7576
bool expose_blob_index = false,
76-
ReadOnlyMemTable* active_mem = nullptr,
7777
Arena* arena = nullptr) {
7878
void* mem = arena ? arena->AllocateAligned(sizeof(DBIter))
7979
: operator new(sizeof(DBIter));
@@ -475,9 +475,9 @@ class DBIter final : public Iterator {
475475
const size_t timestamp_size_;
476476
std::string saved_timestamp_;
477477
std::optional<std::vector<ScanOptions>> scan_opts_;
478-
ReadOnlyMemTable* active_mem_;
479-
SequenceNumber memtable_seqno_lb_;
480-
uint32_t memtable_op_scan_flush_trigger_;
478+
ReadOnlyMemTable* const active_mem_;
479+
const SequenceNumber memtable_seqno_lb_;
480+
const uint32_t memtable_op_scan_flush_trigger_;
481481
Direction direction_;
482482
bool valid_;
483483
bool current_entry_is_merged_;

‎db/db_iter_stress_test.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,7 @@ TEST_F(DBIteratorStressTest, StressTest) {
532532
env_, ropt, ImmutableOptions(options),
533533
MutableCFOptions(options), BytewiseComparator(),
534534
internal_iter, /*version=*/nullptr, sequence,
535-
nullptr /*read_callback*/));
535+
nullptr /*read_callback*/, /*active_mem=*/nullptr));
536536
}
537537

538538
// Do a random operation. It's important to do it on ref_it

0 commit comments

Comments
 (0)