Fix check failure for concurrent create and drop table queries

yutiansut · Jul 8, 2022 · 35ba992 · 35ba992
1 parent 29a6aaf
commit 35ba992
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 24 deletions.
diff --git a/LockMgr/LockMgr.h b/LockMgr/LockMgr.h
@@ -125,9 +125,9 @@ class TableSchemaLockContainer<ReadLock>
                                      const std::string& table_name,
                                      const bool populate_fragmenter = true) {
     VLOG(1) << "Acquiring Table Schema Read Lock for table: " << table_name;
+    auto lock = TableSchemaLockMgr::getReadLockForTable(cat, table_name);
     auto ret = TableSchemaLockContainer<ReadLock>(
-        cat.getMetadataForTable(table_name, populate_fragmenter),
-        TableSchemaLockMgr::getReadLockForTable(cat, table_name));
+        cat.getMetadataForTable(table_name, populate_fragmenter), std::move(lock));
     validate_table_descriptor_after_lock(ret(), cat, table_name, populate_fragmenter);
     return ret;
   }
@@ -156,9 +156,9 @@ class TableSchemaLockContainer<WriteLock>
                                      const std::string& table_name,
                                      const bool populate_fragmenter = true) {
     VLOG(1) << "Acquiring Table Schema Write Lock for table: " << table_name;
+    auto lock = TableSchemaLockMgr::getWriteLockForTable(cat, table_name);
     auto ret = TableSchemaLockContainer<WriteLock>(
-        cat.getMetadataForTable(table_name, populate_fragmenter),
-        TableSchemaLockMgr::getWriteLockForTable(cat, table_name));
+        cat.getMetadataForTable(table_name, populate_fragmenter), std::move(lock));
     validate_table_descriptor_after_lock(ret(), cat, table_name, populate_fragmenter);
     return ret;
   }

diff --git a/LockMgr/LockMgrImpl.h b/LockMgr/LockMgrImpl.h
@@ -224,35 +224,25 @@ class TableLockMgrImpl {
 
   static WriteLock getWriteLockForTable(Catalog_Namespace::Catalog& cat,
                                         const std::string& table_name) {
-    if (const auto tdp = cat.getMetadataForTable(table_name, false)) {
-      ChunkKey chunk_key{cat.getCurrentDB().dbId, tdp->tableId};
-      auto& table_lock_mgr = T::instance();
-      MutexTracker* tracker = table_lock_mgr.getTableMutex(chunk_key);
-      CHECK(tracker);
-      return WriteLock(tracker);
-    } else {
-      throw std::runtime_error("Table/View " + table_name + " for catalog " +
-                               cat.getCurrentDB().dbName + " does not exist");
-    }
+    auto lock = WriteLock(getMutexTracker(cat, table_name));
+    // Ensure table still exists after lock is acquired.
+    validateExistingTable(cat, table_name);
+    return std::move(lock);
   }
+
   static WriteLock getWriteLockForTable(const ChunkKey table_key) {
     auto& table_lock_mgr = T::instance();
     return WriteLock(table_lock_mgr.getTableMutex(table_key));
   }
 
   static ReadLock getReadLockForTable(Catalog_Namespace::Catalog& cat,
                                       const std::string& table_name) {
-    if (const auto tdp = cat.getMetadataForTable(table_name, false)) {
-      ChunkKey chunk_key{cat.getCurrentDB().dbId, tdp->tableId};
-      auto& table_lock_mgr = T::instance();
-      MutexTracker* tracker = table_lock_mgr.getTableMutex(chunk_key);
-      CHECK(tracker);
-      return ReadLock(tracker);
-    } else {
-      throw std::runtime_error("Table/View " + table_name + " for catalog " +
-                               cat.getCurrentDB().dbName + " does not exist");
-    }
+    auto lock = ReadLock(getMutexTracker(cat, table_name));
+    // Ensure table still exists after lock is acquired.
+    validateAndGetExistingTableId(cat, table_name);
+    return std::move(lock);
   }
+
   static ReadLock getReadLockForTable(const ChunkKey table_key) {
     auto& table_lock_mgr = T::instance();
     return ReadLock(table_lock_mgr.getTableMutex(table_key));
@@ -362,6 +352,32 @@ class TableLockMgrImpl {
 
   mutable std::mutex map_mutex_;
   std::map<ChunkKey, std::unique_ptr<MutexTracker>> table_mutex_map_;
+
+ private:
+  static MutexTracker* getMutexTracker(Catalog_Namespace::Catalog& catalog,
+                                       const std::string& table_name) {
+    ChunkKey chunk_key{catalog.getDatabaseId(),
+                       validateAndGetExistingTableId(catalog, table_name)};
+    auto& table_lock_mgr = T::instance();
+    MutexTracker* tracker = table_lock_mgr.getTableMutex(chunk_key);
+    CHECK(tracker);
+    return tracker;
+  }
+
+  static void validateExistingTable(Catalog_Namespace::Catalog& catalog,
+                                    const std::string& table_name) {
+    validateAndGetExistingTableId(catalog, table_name);
+  }
+
+  static int32_t validateAndGetExistingTableId(Catalog_Namespace::Catalog& catalog,
+                                               const std::string& table_name) {
+    auto table_id = catalog.getTableId(table_name);
+    if (!table_id.has_value()) {
+      throw std::runtime_error("Table/View " + table_name + " for catalog " +
+                               catalog.name() + " does not exist");
+    }
+    return table_id.value();
+  }
 };
 
 template <typename T>

diff --git a/java/utility/src/main/java/com/mapd/tests/CatalogConcurrencyTest.java b/java/utility/src/main/java/com/mapd/tests/CatalogConcurrencyTest.java
@@ -31,6 +31,7 @@ public static void main(String[] args) throws Exception {
   private void run_test(
           HeavyDBTestClient dba, HeavyDBTestClient user, String prefix, int max)
           throws Exception {
+    final String sharedTableName = "table_shared";
     for (int i = 0; i < max; i++) {
       String tableName = "table_" + prefix + "_" + i;
       String viewName = "view_" + prefix + "_" + i;
@@ -71,6 +72,14 @@ private void run_test(
       logger.info("[" + tid + "]"
               + "DROP " + tableName);
       dba.runSql("DROP TABLE " + tableName + ";");
+
+      logger.info("[" + tid + "]"
+              + "CREATE IF NOT EXISTS " + sharedTableName);
+      dba.runSql("CREATE TABLE IF NOT EXISTS " + sharedTableName + " (id INTEGER);");
+
+      logger.info("[" + tid + "]"
+              + "DROP IF EXISTS " + sharedTableName);
+      dba.runSql("DROP TABLE IF EXISTS " + sharedTableName + ";");
     }
   }