Skip to content

Commit

Permalink
[#9162] Fix stack overflow in filtering iterator
Browse files Browse the repository at this point in the history
Summary:
Filtering iterator could make a call to Next/Prev while checking filter.
But those methods also would check filter.
So, iterating over one skipped entry results in adding two more stack frames, one for Next() or Prev() and one more for ApplyFilter(), and the stack grows proportionally to the number of skipped entries, resulting in a potential stack overflow.

Actually we should call Next/Prev of the underlying iterator in this case, as done in this diff. This approach would only utilize a constant amount of space on the stack and not linear space based on the number of skipped entries.

Test Plan: ybd debug --gtest_filter BackupTxnTest.PointInTimeBigSkipRestore

Reviewers: bogdan, mbautin

Reviewed By: mbautin

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D12143
  • Loading branch information
spolitov committed Jul 2, 2021
1 parent 27c90b7 commit 124af42
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 2 deletions.
32 changes: 32 additions & 0 deletions src/yb/client/backup-txn-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,38 @@ TEST_F(BackupTxnTest, PointInTimeRestore) {
ASSERT_NO_FATALS(VerifyData(/* num_transactions=*/ 1, WriteOpType::INSERT));
}

TEST_F(BackupTxnTest, PointInTimeBigSkipRestore) {
constexpr int kNumWrites = RegularBuildVsSanitizers(100000, 100);
constexpr int kKey = 123;

std::vector<std::future<FlushStatus>> futures;
auto session = CreateSession();
ASSERT_OK(WriteRow(session, kKey, 0));
auto hybrid_time = cluster_->mini_tablet_server(0)->server()->Clock()->Now();
for (size_t r = 1; r <= kNumWrites; ++r) {
ASSERT_OK(WriteRow(session, kKey, r, WriteOpType::INSERT, client::Flush::kFalse));
futures.push_back(session->FlushFuture());
}

int good = 0;
for (auto& future : futures) {
if (future.get().status.ok()) {
++good;
}
}

LOG(INFO) << "Total good: " << good;

auto snapshot_id = ASSERT_RESULT(CreateSnapshot());
ASSERT_OK(VerifySnapshot(snapshot_id, SysSnapshotEntryPB::COMPLETE));

ASSERT_OK(RestoreSnapshot(snapshot_id, hybrid_time));

auto value = ASSERT_RESULT(SelectRow(session, kKey));
ASSERT_EQ(value, 0);
}


TEST_F(BackupTxnTest, Persistence) {
LOG(INFO) << "Write data";

Expand Down
4 changes: 2 additions & 2 deletions src/yb/rocksdb/table/filtering_iterator.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ class FilteringIterator : public InternalIterator {
break;
}
if (!backward) {
Next();
iterator_->Next();
} else {
Prev();
iterator_->Prev();
}
}
}
Expand Down

0 comments on commit 124af42

Please sign in to comment.