Fix the move semantics of PinnableWideColumns (#12557)

Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/12557

Unlike for other sequence containers, the C++ standard allows moving an `std::string` to invalidate pointers/iterators/references. In practice, this happens with short strings which are stored "inline" in the `std::string` object (small string optimization). Since `PinnableSlice` uses `std::string` as its internal buffer, and `PinnableWideColumns` in turn is implemented in terms of `PinnableSlice`, this means that the default compiler-generated move operations can invalidate the column index stored in `PinnableWideColumns::columns_`. The PR fixes this by providing custom move constructor/move assignment implementations for `PinnableWideColumns` that recreate the `columns_` index upon move.

Reviewed By: jaykorean

Differential Revision: D56275054

fbshipit-source-id: e8648c003dbcf1c39ec122ad229780c28138e730

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) {

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_);
 }
 

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

unreleased_history/bug_fixes/pinnable_wide_columns_move.md

@@ -0,0 +1 @@
+Correctly implemented the move semantics of `PinnableWideColumns`.