Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WritePrepared: release snapshot equal to max #4944

Closed
wants to merge 2 commits into from
Closed

WritePrepared: release snapshot equal to max #4944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f7e8fc
WritePrepared: release snapshot equal to max
Feb 4, 2019
97615ab
add comments
Feb 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions utilities/transactions/write_prepared_transaction_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1239,6 +1239,37 @@ TEST_P(WritePreparedTransactionTest, MaxCatchupWithNewSnapshot) {
db->ReleaseSnapshot(snap);
}

// Check that old_commit_map_ cleanup works correctly if the snapshot equals
// max_evicted_seq_.
TEST_P(WritePreparedTransactionTest, CleanupSnapshotEqualToMax) {
const size_t snapshot_cache_bits = 7; // same as default
const size_t commit_cache_bits = 0; // only 1 entry => frequent eviction
DestroyAndReopenWithExtraOptions(snapshot_cache_bits, commit_cache_bits);
WriteOptions woptions;
WritePreparedTxnDB* wp_db = dynamic_cast<WritePreparedTxnDB*>(db);
// Insert something to increase seq
ASSERT_OK(db->Put(woptions, "key", "value"));
auto snap = db->GetSnapshot();
auto snap_seq = snap->GetSequenceNumber();
// Another insert should trigger eviction + load snapshot from db
ASSERT_OK(db->Put(woptions, "key", "value"));
// This is the scenario that we check agaisnt
ASSERT_EQ(snap_seq, wp_db->max_evicted_seq_);
// old_commit_map_ now has some data that needs gc
ASSERT_EQ(1, wp_db->snapshots_total_);
ASSERT_EQ(1, wp_db->old_commit_map_.size());

db->ReleaseSnapshot(snap);

// Another insert should trigger eviction + load snapshot from db
ASSERT_OK(db->Put(woptions, "key", "value"));

// the snapshot and related metadata must be properly garbage collected
ASSERT_EQ(0, wp_db->snapshots_total_);
ASSERT_TRUE(wp_db->snapshots_all_.empty());
ASSERT_EQ(0, wp_db->old_commit_map_.size());
}

TEST_P(WritePreparedTransactionTest, AdvanceSeqByOne) {
auto snap = db->GetSnapshot();
auto seq1 = snap->GetSequenceNumber();
Expand Down
6 changes: 3 additions & 3 deletions utilities/transactions/write_prepared_txn_db.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,9 +710,9 @@ const std::vector<SequenceNumber> WritePreparedTxnDB::GetSnapshotListFromDB(

void WritePreparedTxnDB::ReleaseSnapshotInternal(
const SequenceNumber snap_seq) {
// relax is enough since max increases monotonically, i.e., if snap_seq <
// old_max => snap_seq < new_max as well.
if (snap_seq < max_evicted_seq_.load(std::memory_order_relaxed)) {
// TODO(myabandeh): relax should enough since the synchronizatin is already
// done by snapshots_mutex_ under which this function is called.
if (snap_seq <= max_evicted_seq_.load(std::memory_order_acquire)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't follow why we needed to change from relaxed => acquire?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithms were initially designed to allow concurrent call to AdvanceMaxEvictedSeq which has added too much unnecessary complexity. We should get rid of all that in a future refactoring.
For the moment my concern was that that if max is already advanced by another thread, and our cpu cache is not updated with that, then with memory_order_relaxed we might fail to properly cleanup the old_commit_map_ for the released snapshot. I do not think this would happen in the current code but it is easier to prove the code is correct with acquire than with relaxed.

// Then this is a rare case that transaction did not finish before max
// advances. It is expected for a few read-only backup snapshots. For such
// snapshots we might have kept around a couple of entries in the
Expand Down
1 change: 1 addition & 0 deletions utilities/transactions/write_prepared_txn_db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,7 @@ class WritePreparedTxnDB : public PessimisticTransactionDB {
WritePreparedTransactionTest_AdvanceMaxEvictedSeqWithDuplicatesTest_Test;
friend class WritePreparedTransactionTest_AdvanceSeqByOne_Test;
friend class WritePreparedTransactionTest_BasicRecoveryTest_Test;
friend class WritePreparedTransactionTest_CleanupSnapshotEqualToMax_Test;
friend class WritePreparedTransactionTest_DoubleSnapshot_Test;
friend class WritePreparedTransactionTest_IsInSnapshotEmptyMapTest_Test;
friend class WritePreparedTransactionTest_IsInSnapshotReleased_Test;
Expand Down