From a101c9de606d9d92c8b9e192b2802ed008b77a19 Mon Sep 17 00:00:00 2001 From: Yu Zhang Date: Mon, 6 Jun 2022 14:36:22 -0700 Subject: [PATCH] Return "invalid argument" when read timestamp is too old (#10109) Summary: With this change, when a given read timestamp is smaller than the column-family's full_history_ts_low, Get(), MultiGet() and iterators APIs will return Status::InValidArgument(). Test plan ``` $COMPILE_WITH_ASAN=1 make -j24 all $./db_with_timestamp_basic_test --gtest_filter=DBBasicTestWithTimestamp.UpdateFullHistoryTsLow $ make -j24 check ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/10109 Reviewed By: riversand963 Differential Revision: D36901126 Pulled By: jowlyzhang fbshipit-source-id: 255feb1a66195351f06c1d0e42acb1ff74527f86 --- db/db_impl/compacted_db_impl.cc | 8 +++---- db/db_impl/db_impl.cc | 19 +++++++++------- db/db_impl/db_impl.h | 22 +++++++++++++++---- db/db_impl/db_impl_readonly.cc | 11 +++++----- db/db_impl/db_impl_secondary.cc | 11 +++++----- db/db_impl/db_impl_write.cc | 6 ++--- db/db_with_timestamp_basic_test.cc | 6 ++--- .../transactions/pessimistic_transaction.cc | 4 ++-- 8 files changed, 52 insertions(+), 35 deletions(-) diff --git a/db/db_impl/compacted_db_impl.cc b/db/db_impl/compacted_db_impl.cc index 482a2e1f6..a505d3e5a 100644 --- a/db/db_impl/compacted_db_impl.cc +++ b/db/db_impl/compacted_db_impl.cc @@ -49,8 +49,8 @@ Status CompactedDBImpl::Get(const ReadOptions& options, ColumnFamilyHandle*, std::string* timestamp) { assert(user_comparator_); if (options.timestamp) { - const Status s = - FailIfTsSizesMismatch(DefaultColumnFamily(), *(options.timestamp)); + const Status s = FailIfTsMismatchCf( + DefaultColumnFamily(), *(options.timestamp), /*ts_for_read=*/true); if (!s.ok()) { return s; } @@ -109,8 +109,8 @@ std::vector CompactedDBImpl::MultiGet( size_t num_keys = keys.size(); if (options.timestamp) { - Status s = - FailIfTsSizesMismatch(DefaultColumnFamily(), *(options.timestamp)); + Status s = FailIfTsMismatchCf(DefaultColumnFamily(), *(options.timestamp), + /*ts_for_read=*/true); if (!s.ok()) { return std::vector(num_keys, s); } diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 2faa22ada..4dbf89a43 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -1737,8 +1737,9 @@ Status DBImpl::GetImpl(const ReadOptions& read_options, const Slice& key, assert(get_impl_options.column_family); if (read_options.timestamp) { - const Status s = FailIfTsSizesMismatch(get_impl_options.column_family, - *(read_options.timestamp)); + const Status s = FailIfTsMismatchCf(get_impl_options.column_family, + *(read_options.timestamp), + /*ts_for_read=*/true); if (!s.ok()) { return s; } @@ -1968,8 +1969,8 @@ std::vector DBImpl::MultiGet( for (size_t i = 0; i < num_keys; ++i) { assert(column_family[i]); if (read_options.timestamp) { - stat_list[i] = - FailIfTsSizesMismatch(column_family[i], *(read_options.timestamp)); + stat_list[i] = FailIfTsMismatchCf( + column_family[i], *(read_options.timestamp), /*ts_for_read=*/true); if (!stat_list[i].ok()) { should_fail = true; } @@ -2303,7 +2304,8 @@ void DBImpl::MultiGet(const ReadOptions& read_options, const size_t num_keys, ColumnFamilyHandle* cfh = column_families[i]; assert(cfh); if (read_options.timestamp) { - statuses[i] = FailIfTsSizesMismatch(cfh, *(read_options.timestamp)); + statuses[i] = FailIfTsMismatchCf(cfh, *(read_options.timestamp), + /*ts_for_read=*/true); if (!statuses[i].ok()) { should_fail = true; } @@ -2963,8 +2965,8 @@ Iterator* DBImpl::NewIterator(const ReadOptions& read_options, assert(column_family); if (read_options.timestamp) { - const Status s = - FailIfTsSizesMismatch(column_family, *(read_options.timestamp)); + const Status s = FailIfTsMismatchCf( + column_family, *(read_options.timestamp), /*ts_for_read=*/true); if (!s.ok()) { return NewErrorIterator(s); } @@ -3105,7 +3107,8 @@ Status DBImpl::NewIterators( if (read_options.timestamp) { for (auto* cf : column_families) { assert(cf); - const Status s = FailIfTsSizesMismatch(cf, *(read_options.timestamp)); + const Status s = FailIfTsMismatchCf(cf, *(read_options.timestamp), + /*ts_for_read=*/true); if (!s.ok()) { return s; } diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h index dae0d21fb..880d30a91 100644 --- a/db/db_impl/db_impl.h +++ b/db/db_impl/db_impl.h @@ -1455,8 +1455,8 @@ class DBImpl : public DB { void SetDbSessionId(); Status FailIfCfHasTs(const ColumnFamilyHandle* column_family) const; - Status FailIfTsSizesMismatch(const ColumnFamilyHandle* column_family, - const Slice& ts) const; + Status FailIfTsMismatchCf(ColumnFamilyHandle* column_family, const Slice& ts, + bool ts_for_read) const; // recovery_ctx stores the context about version edits and // LogAndApplyForRecovery persist all those edits to new Manifest after @@ -2554,8 +2554,9 @@ inline Status DBImpl::FailIfCfHasTs( return Status::OK(); } -inline Status DBImpl::FailIfTsSizesMismatch( - const ColumnFamilyHandle* column_family, const Slice& ts) const { +inline Status DBImpl::FailIfTsMismatchCf(ColumnFamilyHandle* column_family, + const Slice& ts, + bool ts_for_read) const { if (!column_family) { return Status::InvalidArgument("column family handle cannot be null"); } @@ -2575,6 +2576,19 @@ inline Status DBImpl::FailIfTsSizesMismatch( << ts_sz << " given"; return Status::InvalidArgument(oss.str()); } + if (ts_for_read) { + auto cfh = static_cast_with_check(column_family); + auto cfd = cfh->cfd(); + std::string current_ts_low = cfd->GetFullHistoryTsLow(); + if (!current_ts_low.empty() && + ucmp->CompareTimestamp(ts, current_ts_low) < 0) { + std::stringstream oss; + oss << "Read timestamp: " << ts.ToString(true) + << " is smaller than full_history_ts_low: " + << Slice(current_ts_low).ToString(true) << std::endl; + return Status::InvalidArgument(oss.str()); + } + } return Status::OK(); } diff --git a/db/db_impl/db_impl_readonly.cc b/db/db_impl/db_impl_readonly.cc index 5be89bce2..5548ebc09 100644 --- a/db/db_impl/db_impl_readonly.cc +++ b/db/db_impl/db_impl_readonly.cc @@ -47,8 +47,8 @@ Status DBImplReadOnly::Get(const ReadOptions& read_options, assert(column_family); if (read_options.timestamp) { - const Status s = - FailIfTsSizesMismatch(column_family, *(read_options.timestamp)); + const Status s = FailIfTsMismatchCf( + column_family, *(read_options.timestamp), /*ts_for_read=*/true); if (!s.ok()) { return s; } @@ -114,8 +114,8 @@ Iterator* DBImplReadOnly::NewIterator(const ReadOptions& read_options, ColumnFamilyHandle* column_family) { assert(column_family); if (read_options.timestamp) { - const Status s = - FailIfTsSizesMismatch(column_family, *(read_options.timestamp)); + const Status s = FailIfTsMismatchCf( + column_family, *(read_options.timestamp), /*ts_for_read=*/true); if (!s.ok()) { return NewErrorIterator(s); } @@ -155,7 +155,8 @@ Status DBImplReadOnly::NewIterators( if (read_options.timestamp) { for (auto* cf : column_families) { assert(cf); - const Status s = FailIfTsSizesMismatch(cf, *(read_options.timestamp)); + const Status s = FailIfTsMismatchCf(cf, *(read_options.timestamp), + /*ts_for_read=*/true); if (!s.ok()) { return s; } diff --git a/db/db_impl/db_impl_secondary.cc b/db/db_impl/db_impl_secondary.cc index bec1fe2f7..ab75819e4 100644 --- a/db/db_impl/db_impl_secondary.cc +++ b/db/db_impl/db_impl_secondary.cc @@ -349,8 +349,8 @@ Status DBImplSecondary::GetImpl(const ReadOptions& read_options, assert(column_family); if (read_options.timestamp) { - const Status s = - FailIfTsSizesMismatch(column_family, *(read_options.timestamp)); + const Status s = FailIfTsMismatchCf( + column_family, *(read_options.timestamp), /*ts_for_read=*/true); if (!s.ok()) { return s; } @@ -442,8 +442,8 @@ Iterator* DBImplSecondary::NewIterator(const ReadOptions& read_options, assert(column_family); if (read_options.timestamp) { - const Status s = - FailIfTsSizesMismatch(column_family, *(read_options.timestamp)); + const Status s = FailIfTsMismatchCf( + column_family, *(read_options.timestamp), /*ts_for_read=*/true); if (!s.ok()) { return NewErrorIterator(s); } @@ -514,7 +514,8 @@ Status DBImplSecondary::NewIterators( if (read_options.timestamp) { for (auto* cf : column_families) { assert(cf); - const Status s = FailIfTsSizesMismatch(cf, *(read_options.timestamp)); + const Status s = FailIfTsMismatchCf(cf, *(read_options.timestamp), + /*ts_for_read=*/true); if (!s.ok()) { return s; } diff --git a/db/db_impl/db_impl_write.cc b/db/db_impl/db_impl_write.cc index 62d68bc08..fee4df87b 100644 --- a/db/db_impl/db_impl_write.cc +++ b/db/db_impl/db_impl_write.cc @@ -30,7 +30,7 @@ Status DBImpl::Put(const WriteOptions& o, ColumnFamilyHandle* column_family, Status DBImpl::Put(const WriteOptions& o, ColumnFamilyHandle* column_family, const Slice& key, const Slice& ts, const Slice& val) { - const Status s = FailIfTsSizesMismatch(column_family, ts); + const Status s = FailIfTsMismatchCf(column_family, ts, /*ts_for_read=*/false); if (!s.ok()) { return s; } @@ -63,7 +63,7 @@ Status DBImpl::Delete(const WriteOptions& write_options, Status DBImpl::Delete(const WriteOptions& write_options, ColumnFamilyHandle* column_family, const Slice& key, const Slice& ts) { - const Status s = FailIfTsSizesMismatch(column_family, ts); + const Status s = FailIfTsMismatchCf(column_family, ts, /*ts_for_read=*/false); if (!s.ok()) { return s; } @@ -83,7 +83,7 @@ Status DBImpl::SingleDelete(const WriteOptions& write_options, Status DBImpl::SingleDelete(const WriteOptions& write_options, ColumnFamilyHandle* column_family, const Slice& key, const Slice& ts) { - const Status s = FailIfTsSizesMismatch(column_family, ts); + const Status s = FailIfTsMismatchCf(column_family, ts, /*ts_for_read=*/false); if (!s.ok()) { return s; } diff --git a/db/db_with_timestamp_basic_test.cc b/db/db_with_timestamp_basic_test.cc index 473cf0ce7..57ce76f79 100644 --- a/db/db_with_timestamp_basic_test.cc +++ b/db/db_with_timestamp_basic_test.cc @@ -272,7 +272,6 @@ TEST_F(DBBasicTestWithTimestamp, UpdateFullHistoryTsLow) { } ASSERT_OK(Flush()); - // TODO return a non-ok for read ts < current_ts_low and test it. for (int i = 0; i < 10; i++) { ReadOptions read_opts; std::string ts_str = Timestamp(i, 0); @@ -280,8 +279,8 @@ TEST_F(DBBasicTestWithTimestamp, UpdateFullHistoryTsLow) { read_opts.timestamp = &ts; std::string value; Status status = db_->Get(read_opts, kKey, &value); - if (i < current_ts_low - 1) { - ASSERT_TRUE(status.IsNotFound()); + if (i < current_ts_low) { + ASSERT_TRUE(status.IsInvalidArgument()); } else { ASSERT_OK(status); ASSERT_TRUE(value.compare(Key(i)) == 0); @@ -305,7 +304,6 @@ TEST_F(DBBasicTestWithTimestamp, UpdateFullHistoryTsLow) { result_ts_low = cfd->GetFullHistoryTsLow(); ASSERT_TRUE(test_cmp.CompareTimestamp(ts_low, result_ts_low) == 0); - // TODO return a non-ok for read ts < current_ts_low and test it. for (int i = current_ts_low; i < 20; i++) { ReadOptions read_opts; std::string ts_str = Timestamp(i, 0); diff --git a/utilities/transactions/pessimistic_transaction.cc b/utilities/transactions/pessimistic_transaction.cc index c5099abad..b47317060 100644 --- a/utilities/transactions/pessimistic_transaction.cc +++ b/utilities/transactions/pessimistic_transaction.cc @@ -176,8 +176,8 @@ inline Status WriteCommittedTxn::GetForUpdateImpl( value, exclusive, do_validate); } } else { - Status s = db_impl_->FailIfTsSizesMismatch(column_family, - *(read_options.timestamp)); + Status s = db_impl_->FailIfTsMismatchCf( + column_family, *(read_options.timestamp), /*ts_for_read=*/true); if (!s.ok()) { return s; }