[tablet] fix race in RowSetMetadata::id()

This patch fixes a race in access to the RowSetMetadata::id_ field
in the rollback scenario in the MajorCompactDeltaStoresWithColumnIds()
method of the DiskRowSet class.

Before this patch, TSAN would report warnings like below when running
the MultiThreadedHybridClockTabletTest.UpdateNoMergeCompaction scenario:
of the mt-tablet-test:

    Read of size 8 at 0x7b3400014780 by thread T30 (mutexes: write M76293278759445
  9152, write M7098002):
      #0 kudu::tablet::RowSetMetadata::id() const src/kudu/tablet/rowset_metadata.h:100:31 (libtablet.so+0x346faa)
      #1 kudu::tablet::RowSetTree::Reset(...) src/kudu/tablet/rowset_tree.cc:190:48 (libtablet.so+0x4bf666)
      #2 kudu::tablet::Tablet::ModifyRowSetTree(...) src/kudu/tablet/tablet.cc:1490:3 (libtablet.so+0x323755)
      #3 kudu::tablet::Tablet::AtomicSwapRowSetsUnlocked(...) src/kudu/tablet/tablet.cc:1504:3 (libtablet.so+0x3239bc)
      #4 kudu::tablet::Tablet::AtomicSwapRowSets(...) src/kudu/tablet/tablet.cc:1496:3 (libtablet.so+0x3238f9)
      ...

    Previous write of size 8 at 0x7b3400014780 by thread T12 (mutexes: write M625572878699880144, write M530715863088620288, write M525367769810683784):
      #0 kudu::tablet::RowSetMetadata::LoadFromPB(...) src/kudu/tablet/rowset_metadata.cc:77:7 (libtablet.so+0x4f9f03)
      #1 kudu::tablet::DiskRowSet::MajorCompactDeltaStoresWithColumnIds(...)::$_0::operator()() const src/kudu/tablet/diskrowset.cc:603:23 (libtablet.so+0x46eddf)
      #2 kudu::ScopedCleanup<kudu::tablet::DiskRowSet::MajorCompactDeltaStoresWithColumnIds(...)::$_0>::~ScopedCleanup() src/kudu/util/scoped_cleanup.h:51:7 (libtablet.so+0x46cc5a)
      #3 kudu::tablet::DiskRowSet::MajorCompactDeltaStoresWithColumnIds(...) src/kudu/tablet/diskrowset.cc:636:1 (libtablet.so+0x46c5c9)
      #4 kudu::tablet::DiskRowSet::MajorCompactDeltaStores(...) src/kudu/tablet/diskrowset.cc:570:10 (libtablet.so+0x46c013)
      ...

  SUMMARY: ThreadSanitizer: data race src/kudu/tablet/rowset_metadata.h:100:31 in kudu::tablet::RowSetMetadata::id() const

Change-Id: I4b09575616e754b7dbb24586293f128e361b9360
Reviewed-on: http://gerrit.cloudera.org:8080/21779
Reviewed-by: Mahesh Reddy <[email protected]>
Tested-by: Alexey Serbin <[email protected]>
Reviewed-by: Yingchun Lai <[email protected]>

src/kudu/tablet/cfile_set.cc

@@ -137,18 +137,16 @@ Status CFileSet::DoOpen(const IOContext* io_context) {
 
   // Lazily open the column data cfiles. Each one will be fully opened
   // later, when the first iterator seeks for the first time.
-  RowSetMetadata::ColumnIdToBlockIdMap block_map = rowset_metadata_->GetColumnBlocksById();
-  for (const RowSetMetadata::ColumnIdToBlockIdMap::value_type& e : block_map) {
-    ColumnId col_id = e.first;
-    DCHECK(!ContainsKey(readers_by_col_id_, col_id)) << "already open";
-
+  const RowSetMetadata::ColumnIdToBlockIdMap block_map = rowset_metadata_->GetColumnBlocksById();
+  for (const auto& [col_id, _] : block_map) {
     unique_ptr<CFileReader> reader;
     RETURN_NOT_OK(OpenReader(rowset_metadata_->fs_manager(),
                              cfile_reader_tracker_,
                              rowset_metadata_->column_data_block_for_col_id(col_id),
                              io_context,
                              &reader));
-    readers_by_col_id_[col_id] = std::move(reader);
+    const auto& ins_info = readers_by_col_id_.emplace(col_id, std::move(reader));
+    DCHECK(ins_info.second) << "already open";
     VLOG(1) << "Successfully opened cfile for column id " << col_id
             << " in " << rowset_metadata_->ToString();
   }

src/kudu/tablet/rowset_metadata.cc

@@ -64,7 +64,7 @@ Status RowSetMetadata::Flush() {
 }
 
 Status RowSetMetadata::InitFromPB(const RowSetDataPB& pb) {
-  CHECK(!initted_);
+  DCHECK(!initted_);
 
   LoadFromPB(pb);
 
@@ -125,9 +125,8 @@ void RowSetMetadata::LoadFromPB(const RowSetDataPB& pb) {
 }
 
 void RowSetMetadata::ToProtobuf(RowSetDataPB *pb) {
-  pb->set_id(id_);
-
   std::lock_guard l(lock_);
+  pb->set_id(id_);
 
   // Write Column Files
   for (const ColumnIdToBlockIdMap::value_type& e : blocks_by_col_id_) {
@@ -175,7 +174,12 @@ void RowSetMetadata::ToProtobuf(RowSetDataPB *pb) {
 }
 
 const std::string RowSetMetadata::ToString() const {
-  return Substitute("RowSet($0)", id_);
+  int64_t id = 0;
+  {
+    std::lock_guard l(lock_);
+    id = id_;
+  }
+  return Substitute("RowSet($0)", id);
 }
 
 void RowSetMetadata::SetColumnDataBlocks(const std::map<ColumnId, BlockId>& blocks_by_col_id) {

src/kudu/tablet/rowset_metadata.h

@@ -97,7 +97,10 @@ class RowSetMetadata {
 
   const std::string ToString() const;
 
-  int64_t id() const { return id_; }
+  int64_t id() const {
+    std::lock_guard l(lock_);
+    return id_;
+  }
 
   const SchemaPtr tablet_schema() const {
     return tablet_metadata_->schema();
@@ -266,11 +269,14 @@ class RowSetMetadata {
 
   TabletMetadata* const tablet_metadata_;
   bool initted_;
-  int64_t id_;
 
   // Protects the below mutable fields.
   mutable LockType lock_;
 
+  // Rowset identifier. Set in one of the constructors, and may be also set
+  // via LoadFromPB().
+  int64_t id_;
+
   // The min and max keys of the rowset.
   std::optional<std::string> min_encoded_key_;
   std::optional<std::string> max_encoded_key_;