diff --git a/db/wide/db_wide_basic_test.cc b/db/wide/db_wide_basic_test.cc index 15d2fdff7f..6f3bc9be7e 100644 --- a/db/wide/db_wide_basic_test.cc +++ b/db/wide/db_wide_basic_test.cc @@ -1723,6 +1723,77 @@ TEST_F(DBWideBasicTest, PutEntitySerializationError) { ASSERT_OK(db_->Write(WriteOptions(), &batch)); } +TEST_F(DBWideBasicTest, PinnableWideColumnsMove) { + Options options = GetDefaultOptions(); + + constexpr char key1[] = "foo"; + constexpr char value[] = "bar"; + ASSERT_OK(db_->Put(WriteOptions(), db_->DefaultColumnFamily(), key1, value)); + + constexpr char key2[] = "baz"; + const WideColumns columns{{"quux", "corge"}}; + ASSERT_OK(db_->PutEntity(WriteOptions(), db_->DefaultColumnFamily(), key2, + columns)); + + ASSERT_OK(db_->Flush(FlushOptions())); + + const auto test_move = [&](bool fill_cache) { + ReadOptions read_options; + read_options.fill_cache = fill_cache; + + { + const WideColumns expected_columns{{kDefaultWideColumnName, value}}; + + { + PinnableWideColumns result; + ASSERT_OK(db_->GetEntity(read_options, db_->DefaultColumnFamily(), key1, + &result)); + ASSERT_EQ(result.columns(), expected_columns); + + PinnableWideColumns move_target(std::move(result)); + ASSERT_EQ(move_target.columns(), expected_columns); + } + + { + PinnableWideColumns result; + ASSERT_OK(db_->GetEntity(read_options, db_->DefaultColumnFamily(), key1, + &result)); + ASSERT_EQ(result.columns(), expected_columns); + + PinnableWideColumns move_target; + move_target = std::move(result); + ASSERT_EQ(move_target.columns(), expected_columns); + } + } + + { + PinnableWideColumns result; + ASSERT_OK(db_->GetEntity(read_options, db_->DefaultColumnFamily(), key2, + &result)); + ASSERT_EQ(result.columns(), columns); + + PinnableWideColumns move_target(std::move(result)); + ASSERT_EQ(move_target.columns(), columns); + } + + { + PinnableWideColumns result; + ASSERT_OK(db_->GetEntity(read_options, db_->DefaultColumnFamily(), key2, + &result)); + ASSERT_EQ(result.columns(), columns); + + PinnableWideColumns move_target; + move_target = std::move(result); + ASSERT_EQ(move_target.columns(), columns); + } + }; + + // Test with and without fill_cache to cover both the case when pointers are + // invalidated during PinnableSlice's move and when they are not. + test_move(/* fill_cache*/ false); + test_move(/* fill_cache*/ true); +} + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/db/wide/wide_columns.cc b/db/wide/wide_columns.cc index b255cfe225..67b734781d 100644 --- a/db/wide/wide_columns.cc +++ b/db/wide/wide_columns.cc @@ -13,8 +13,9 @@ const Slice kDefaultWideColumnName; const WideColumns kNoWideColumns; Status PinnableWideColumns::CreateIndexForWideColumns() { - Slice value_copy = value_; + columns_.clear(); + Slice value_copy = value_; return WideColumnSerialization::Deserialize(value_copy, columns_); } diff --git a/include/rocksdb/wide_columns.h b/include/rocksdb/wide_columns.h index e677f07dab..cba44df1dd 100644 --- a/include/rocksdb/wide_columns.h +++ b/include/rocksdb/wide_columns.h @@ -105,6 +105,16 @@ extern const WideColumns kNoWideColumns; // wide-column queries. class PinnableWideColumns { public: + PinnableWideColumns() = default; + + PinnableWideColumns(const PinnableWideColumns&) = delete; + PinnableWideColumns& operator=(const PinnableWideColumns&) = delete; + + PinnableWideColumns(PinnableWideColumns&&); + PinnableWideColumns& operator=(PinnableWideColumns&&); + + ~PinnableWideColumns() = default; + const WideColumns& columns() const { return columns_; } size_t serialized_size() const { return value_.size(); } @@ -121,6 +131,7 @@ class PinnableWideColumns { void Reset(); private: + void Move(PinnableWideColumns&& other); void CopyValue(const Slice& value); void PinOrCopyValue(const Slice& value, Cleanable* cleanable); void MoveValue(PinnableSlice&& value); @@ -133,6 +144,37 @@ class PinnableWideColumns { WideColumns columns_; }; +inline void PinnableWideColumns::Move(PinnableWideColumns&& other) { + assert(columns_.empty()); + + if (other.columns_.empty()) { + return; + } + + const char* const data = other.value_.data(); + const bool is_plain_value = + other.columns_.size() == 1 && + other.columns_.front().name() == kDefaultWideColumnName && + other.columns_.front().value() == other.value_; + + MoveValue(std::move(other.value_)); + + if (value_.data() == data) { + columns_ = std::move(other.columns_); + } else { + if (is_plain_value) { + CreateIndexForPlainValue(); + } else { + const Status s = CreateIndexForWideColumns(); + assert(s.ok()); + + s.PermitUncheckedError(); + } + } + + other.Reset(); +} + inline void PinnableWideColumns::CopyValue(const Slice& value) { value_.PinSelf(value); } @@ -210,6 +252,20 @@ inline void PinnableWideColumns::Reset() { columns_.clear(); } +inline PinnableWideColumns::PinnableWideColumns(PinnableWideColumns&& other) { + Move(std::move(other)); +} + +inline PinnableWideColumns& PinnableWideColumns::operator=( + PinnableWideColumns&& other) { + if (this != &other) { + Reset(); + Move(std::move(other)); + } + + return *this; +} + inline bool operator==(const PinnableWideColumns& lhs, const PinnableWideColumns& rhs) { return lhs.columns() == rhs.columns(); @@ -220,5 +276,4 @@ inline bool operator!=(const PinnableWideColumns& lhs, return !(lhs == rhs); } - } // namespace ROCKSDB_NAMESPACE diff --git a/unreleased_history/bug_fixes/pinnable_wide_columns_move.md b/unreleased_history/bug_fixes/pinnable_wide_columns_move.md new file mode 100644 index 0000000000..8dceb277d3 --- /dev/null +++ b/unreleased_history/bug_fixes/pinnable_wide_columns_move.md @@ -0,0 +1 @@ +Correctly implemented the move semantics of `PinnableWideColumns`.