fix concurrency issue

kuzudb · Aug 30, 2023 · 0991de8 · 0991de8
1 parent 89b9497
commit 0991de8
Show file tree

Hide file tree

Showing 7 changed files with 35 additions and 29 deletions.
diff --git a/src/include/storage/store/nodes_statistics_and_deleted_ids.h b/src/include/storage/store/nodes_statistics_and_deleted_ids.h
@@ -112,7 +112,7 @@ class NodesStatisticsAndDeletedIDs : public TablesStatistics {
     }
 
     inline void setNumTuplesForTable(common::table_id_t tableID, uint64_t numTuples) override {
-        initTableStatisticPerTableForWriteTrxIfNecessary();
+        initTableStatisticsForWriteTrx();
         ((NodeStatisticsAndDeletedIDs*)tablesStatisticsContentForWriteTrx
                 ->tableStatisticPerTable[tableID]
                 .get())
@@ -121,21 +121,18 @@ class NodesStatisticsAndDeletedIDs : public TablesStatistics {
 
     inline common::offset_t getMaxNodeOffset(
         transaction::Transaction* transaction, common::table_id_t tableID) {
-        return getMaxNodeOffset(transaction == nullptr || transaction->isReadOnly() ?
-                                    transaction::TransactionType::READ_ONLY :
-                                    transaction::TransactionType::WRITE,
-            tableID);
-    }
-
-    inline common::offset_t getMaxNodeOffset(
-        transaction::TransactionType transactionType, common::table_id_t tableID) {
-        return (transactionType == transaction::TransactionType::READ_ONLY ||
-                   tablesStatisticsContentForWriteTrx == nullptr) ?
-                   getNodeStatisticsAndDeletedIDs(tableID)->getMaxNodeOffset() :
-                   ((NodeStatisticsAndDeletedIDs*)tablesStatisticsContentForWriteTrx
-                           ->tableStatisticPerTable[tableID]
-                           .get())
-                       ->getMaxNodeOffset();
+        assert(transaction);
+        if (transaction->getType() == transaction::TransactionType::READ_ONLY) {
+            return getNodeStatisticsAndDeletedIDs(tableID)->getMaxNodeOffset();
+        } else {
+            std::unique_lock xLck{mtx};
+            return tablesStatisticsContentForWriteTrx == nullptr ?
+                       getNodeStatisticsAndDeletedIDs(tableID)->getMaxNodeOffset() :
+                       ((NodeStatisticsAndDeletedIDs*)tablesStatisticsContentForWriteTrx
+                               ->tableStatisticPerTable[tableID]
+                               .get())
+                           ->getMaxNodeOffset();
+        }
     }
 
     // This function is only used for testing purpose.
@@ -149,7 +146,7 @@ class NodesStatisticsAndDeletedIDs : public TablesStatistics {
     // keep the interface simple and no transaction is passed.
     common::offset_t addNode(common::table_id_t tableID) {
         lock_t lck{mtx};
-        initTableStatisticPerTableForWriteTrxIfNecessary();
+        initTableStatisticsForWriteTrxNoLock();
         return ((NodeStatisticsAndDeletedIDs*)tablesStatisticsContentForWriteTrx
                     ->tableStatisticPerTable[tableID]
                     .get())
@@ -159,7 +156,7 @@ class NodesStatisticsAndDeletedIDs : public TablesStatistics {
     // Refer to the comments for addNode.
     void deleteNode(common::table_id_t tableID, common::offset_t nodeOffset) {
         lock_t lck{mtx};
-        initTableStatisticPerTableForWriteTrxIfNecessary();
+        initTableStatisticsForWriteTrxNoLock();
         ((NodeStatisticsAndDeletedIDs*)tablesStatisticsContentForWriteTrx
                 ->tableStatisticPerTable[tableID]
                 .get())

diff --git a/src/include/storage/store/property_statistics.h b/src/include/storage/store/property_statistics.h
@@ -10,6 +10,9 @@ class PropertyStatistics {
 public:
     PropertyStatistics() = default;
     explicit PropertyStatistics(bool mayHaveNullValue) : mayHaveNullValue{mayHaveNullValue} {}
+    PropertyStatistics(const PropertyStatistics& other) {
+        this->mayHaveNullValue = other.mayHaveNullValue;
+    }
 
     inline bool mayHaveNull() const { return mayHaveNullValue; }
     PropertyStatistics(PropertyStatistics& other) = default;

diff --git a/src/include/storage/store/table_statistics.h b/src/include/storage/store/table_statistics.h
@@ -98,7 +98,7 @@ class TablesStatistics {
     }
 
     inline void addTableStatistic(catalog::TableSchema* tableSchema) {
-        initTableStatisticPerTableForWriteTrxIfNecessary();
+        initTableStatisticsForWriteTrx();
         tablesStatisticsContentForWriteTrx->tableStatisticPerTable[tableSchema->tableID] =
             constructTableStatistic(tableSchema);
     }
@@ -120,7 +120,7 @@ class TablesStatistics {
                 tablesStatisticsContentForReadOnlyTrx->tableStatisticPerTable.at(tableID).get();
             return tableStatistics->getPropertyStatistics(propertyID);
         } else {
-            initTableStatisticPerTableForWriteTrxIfNecessary();
+            initTableStatisticsForWriteTrx();
             assert(tablesStatisticsContentForWriteTrx->tableStatisticPerTable.contains(tableID));
             auto tableStatistics =
                 tablesStatisticsContentForWriteTrx->tableStatisticPerTable.at(tableID).get();
@@ -130,7 +130,7 @@ class TablesStatistics {
 
     void setPropertyStatisticsForTable(
         common::table_id_t tableID, common::property_id_t propertyID, PropertyStatistics stats) {
-        initTableStatisticPerTableForWriteTrxIfNecessary();
+        initTableStatisticsForWriteTrx();
         assert(tablesStatisticsContentForWriteTrx->tableStatisticPerTable.contains(tableID));
         auto tableStatistics =
             tablesStatisticsContentForWriteTrx->tableStatisticPerTable.at(tableID).get();
@@ -162,7 +162,8 @@ class TablesStatistics {
     void saveToFile(const std::string& directory, common::DBFileType dbFileType,
         transaction::TransactionType transactionType);
 
-    void initTableStatisticPerTableForWriteTrxIfNecessary();
+    void initTableStatisticsForWriteTrx();
+    void initTableStatisticsForWriteTrxNoLock();
 
 protected:
     std::shared_ptr<spdlog::logger> logger;

diff --git a/src/storage/store/nodes_statistics_and_deleted_ids.cpp b/src/storage/store/nodes_statistics_and_deleted_ids.cpp
@@ -157,7 +157,7 @@ NodesStatisticsAndDeletedIDs::NodesStatisticsAndDeletedIDs(
     std::unordered_map<table_id_t, std::unique_ptr<NodeStatisticsAndDeletedIDs>>&
         nodesStatisticsAndDeletedIDs)
     : TablesStatistics{} {
-    initTableStatisticPerTableForWriteTrxIfNecessary();
+    initTableStatisticsForWriteTrx();
     for (auto& nodeStatistics : nodesStatisticsAndDeletedIDs) {
         tablesStatisticsContentForReadOnlyTrx->tableStatisticPerTable[nodeStatistics.first] =
             std::make_unique<NodeStatisticsAndDeletedIDs>(
@@ -206,7 +206,7 @@ void NodesStatisticsAndDeletedIDs::setDeletedNodeOffsetsForMorsel(
 
 void NodesStatisticsAndDeletedIDs::addNodeStatisticsAndDeletedIDs(
     catalog::NodeTableSchema* tableSchema) {
-    initTableStatisticPerTableForWriteTrxIfNecessary();
+    initTableStatisticsForWriteTrx();
     tablesStatisticsContentForWriteTrx->tableStatisticPerTable[tableSchema->tableID] =
         constructTableStatistic(tableSchema);
 }

diff --git a/src/storage/store/rels_statistics.cpp b/src/storage/store/rels_statistics.cpp
@@ -8,7 +8,7 @@ namespace storage {
 // We should only call this function after we call setNumRelsPerDirectionBoundTableID.
 void RelsStatistics::setNumTuplesForTable(table_id_t relTableID, uint64_t numRels) {
     lock_t lck{mtx};
-    initTableStatisticPerTableForWriteTrxIfNecessary();
+    initTableStatisticsForWriteTrxNoLock();
     assert(tablesStatisticsContentForWriteTrx->tableStatisticPerTable.contains(relTableID));
     auto relStatistics =
         (RelStatistics*)tablesStatisticsContentForWriteTrx->tableStatisticPerTable[relTableID]
@@ -19,7 +19,7 @@ void RelsStatistics::setNumTuplesForTable(table_id_t relTableID, uint64_t numRel
 
 void RelsStatistics::updateNumRelsByValue(table_id_t relTableID, int64_t value) {
     lock_t lck{mtx};
-    initTableStatisticPerTableForWriteTrxIfNecessary();
+    initTableStatisticsForWriteTrxNoLock();
     auto relStatistics =
         (RelStatistics*)tablesStatisticsContentForWriteTrx->tableStatisticPerTable[relTableID]
             .get();

diff --git a/src/storage/store/table_statistics.cpp b/src/storage/store/table_statistics.cpp
@@ -71,7 +71,12 @@ void TablesStatistics::saveToFile(const std::string& directory, DBFileType dbFil
     logger->info("Wrote {} to {}.", getTableTypeForPrinting(), filePath);
 }
 
-void TablesStatistics::initTableStatisticPerTableForWriteTrxIfNecessary() {
+void TablesStatistics::initTableStatisticsForWriteTrx() {
+    std::unique_lock xLck{mtx};
+    initTableStatisticsForWriteTrxNoLock();
+}
+
+void TablesStatistics::initTableStatisticsForWriteTrxNoLock() {
     if (tablesStatisticsContentForWriteTrx == nullptr) {
         tablesStatisticsContentForWriteTrx = std::make_unique<TablesStatisticsContent>();
         for (auto& tableStatistic : tablesStatisticsContentForReadOnlyTrx->tableStatisticPerTable) {

diff --git a/test/runner/e2e_copy_transaction_test.cpp b/test/runner/e2e_copy_transaction_test.cpp
@@ -54,7 +54,7 @@ class TinySnbCopyCSVTransactionTest : public EmptyDBTest {
         ASSERT_EQ(getStorageManager(*database)
                       ->getNodesStore()
                       .getNodesStatisticsAndDeletedIDs()
-                      .getMaxNodeOffset(transaction::TransactionType::READ_ONLY, tableID),
+                      .getMaxNodeOffset(&transaction::DUMMY_READ_TRANSACTION, tableID),
             UINT64_MAX);
     }
 
@@ -67,7 +67,7 @@ class TinySnbCopyCSVTransactionTest : public EmptyDBTest {
         ASSERT_EQ(getStorageManager(*database)
                       ->getNodesStore()
                       .getNodesStatisticsAndDeletedIDs()
-                      .getMaxNodeOffset(transaction::TransactionType::READ_ONLY, tableID),
+                      .getMaxNodeOffset(&transaction::DUMMY_READ_TRANSACTION, tableID),
             7);
     }