[2/3] kudu git commit: [mvcc] Fix watermark advancement in the absence of committed transactions

[al...@apache.org]

[mvcc] Fix watermark advancement in the absence of committed transactions

MvccSnapshot's 'none_committed_at_or_after_' (ncaoa) watermark
indicates the point after which no transactions are committed.
This is used to cull redo deltas that cannot contain any
committed transactions under a snapshot, thus reducing work when
applying deltas.

By definition, this watermark is never supposed to be lower than
the 'all_committed_before_' watermark. If this invariant is broken
delta selection might be wrong by skipping whole delta files.

Normally the ncaoa watermark is advanced when transactions are
marked as committed, in which case this is done correctly. However
there is problem when the 'all_committed_before_' watermark
is advanced and there are no committed or in flight transactions.
In this case the ncaoa watermark might be left behind, manifesting
as incorrect delta application/skipping or even crashes.

This patch includes a fix and a very directed test that makes sure
it is correct. A follow up patch includes a new test in fuzz-itest
that would trigger a crash without the fix.

Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Reviewed-on: http://gerrit.cloudera.org:8080/8964
Tested-by: David Ribeiro Alves <da...@gmail.com>
Reviewed-by: Todd Lipcon <to...@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/ad3b6a63
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/ad3b6a63
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/ad3b6a63

Branch: refs/heads/master
Commit: ad3b6a63a41c5b3d2f9907654f505bf76dba14ab
Parents: 8b873a1
Author: David Alves <dr...@apache.org>
Authored: Mon Jan 8 12:36:35 2018 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Tue Jan 9 20:31:02 2018 +0000

----------------------------------------------------------------------
 src/kudu/tablet/mvcc-test.cc | 42 +++++++++++++++++++++++++++++++++++++++
 src/kudu/tablet/mvcc.cc      |  7 +++++++
 src/kudu/tablet/mvcc.h       |  2 +-
 3 files changed, 50 insertions(+), 1 deletion(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/ad3b6a63/src/kudu/tablet/mvcc-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/mvcc-test.cc b/src/kudu/tablet/mvcc-test.cc
index 9bcea5e..8458922 100644
--- a/src/kudu/tablet/mvcc-test.cc
+++ b/src/kudu/tablet/mvcc-test.cc
@@ -740,5 +740,47 @@ TEST_F(MvccTest, TestWaitUntilCleanDeadline) {
   ASSERT_TRUE(s.IsTimedOut()) << s.ToString();
 }
 
+// Test for a bug related to the initialization of the MvccManager without
+// any pending transactions, i.e. when there are only calls to AdvanceSafeTime().
+// Prior to the fix we would advance safe/clean time but not the
+// 'none_committed_at_or_after_' watermark, meaning the latter would become lower
+// than safe/clean time. This had the effect on compaction of culling delta files
+// even though they shouldn't be culled.
+// This test makes sure that watermarks are advanced correctly and that delta
+// files are culled correctly.
+TEST_F(MvccTest, TestCorrectInitWithNoTxns) {
+  MvccManager mgr;
+
+  MvccSnapshot snap;
+  mgr.TakeSnapshot(&snap);
+  EXPECT_EQ(snap.all_committed_before_, Timestamp::kInitialTimestamp);
+  EXPECT_EQ(snap.none_committed_at_or_after_, Timestamp::kInitialTimestamp);
+  EXPECT_EQ(snap.committed_timestamps_.size(), 0);
+
+  // Read the clock a few times to advance the timestamp
+  for (int i = 0; i < 10; i++) clock_->Now();
+
+  // Advance the safe timestamp.
+  Timestamp new_safe_time = clock_->Now();
+  mgr.AdjustSafeTime(new_safe_time);
+
+  // Test that the snapshot reports that a timestamp lower than the safe time
+  // may have be committed transaction before that timestamp.
+  // Conversely, test that the snapshot reports that a timestamp greater
+  // than the safe time (and thus greater than none_committed_at_or_after_'
+  // as there are no other transactions committed) cannot have any committed
+  // transactions after that timestamp.
+  MvccSnapshot snap2;
+  mgr.TakeSnapshot(&snap2);
+  Timestamp before_safe_time(new_safe_time.value() - 1);
+  Timestamp after_safe_time(new_safe_time.value() + 1);
+  EXPECT_TRUE(snap2.MayHaveCommittedTransactionsAtOrAfter(before_safe_time));
+  EXPECT_FALSE(snap2.MayHaveCommittedTransactionsAtOrAfter(after_safe_time));
+
+  EXPECT_EQ(snap2.all_committed_before_, new_safe_time);
+  EXPECT_EQ(snap2.none_committed_at_or_after_, new_safe_time);
+  EXPECT_EQ(snap2.committed_timestamps_.size(), 0);
+}
+
 } // namespace tablet
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/ad3b6a63/src/kudu/tablet/mvcc.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/mvcc.cc b/src/kudu/tablet/mvcc.cc
index 87786b9..62d3d43 100644
--- a/src/kudu/tablet/mvcc.cc
+++ b/src/kudu/tablet/mvcc.cc
@@ -238,6 +238,13 @@ void MvccManager::AdjustCleanTime() {
   // Filter out any committed timestamps that now fall below the watermark
   FilterTimestamps(&cur_snap_.committed_timestamps_, cur_snap_.all_committed_before_.value());
 
+  // If the current snapshot doesn't have any committed timestamps, then make sure we still
+  // advance the 'none_committed_at_or_after_' watermark so that it never falls below
+  // 'all_committed_before_'.
+  if (cur_snap_.committed_timestamps_.empty()) {
+    cur_snap_.none_committed_at_or_after_ = cur_snap_.all_committed_before_;
+  }
+
   // it may also have unblocked some waiters.
   // Check if someone is waiting for transactions to be committed.
   if (PREDICT_FALSE(!waiters_.empty())) {

http://git-wip-us.apache.org/repos/asf/kudu/blob/ad3b6a63/src/kudu/tablet/mvcc.h
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/mvcc.h b/src/kudu/tablet/mvcc.h
index f6b5baa..24af2b3 100644
--- a/src/kudu/tablet/mvcc.h
+++ b/src/kudu/tablet/mvcc.h
@@ -116,6 +116,7 @@ class MvccSnapshot {
   FRIEND_TEST(MvccTest, TestMayHaveCommittedTransactionsAtOrAfter);
   FRIEND_TEST(MvccTest, TestMayHaveUncommittedTransactionsBefore);
   FRIEND_TEST(MvccTest, TestWaitUntilAllCommitted_SnapAtTimestampWithInFlights);
+  FRIEND_TEST(MvccTest, TestCorrectInitWithNoTxns);
 
   bool IsCommittedFallback(const Timestamp& timestamp) const;
 
@@ -430,4 +431,3 @@ class ScopedTransaction {
 
 } // namespace tablet
 } // namespace kudu
-