Merge pull request #1337 from pmenon/fix-delete-1336

Fix for #1336

src/codegen/deleter.cpp

@@ -6,27 +6,31 @@
 //
 // Identification: src/codegen/deleter.cpp
 //
-// Copyright (c) 2015-2017, Carnegie Mellon University Database Group
+// Copyright (c) 2015-2018, Carnegie Mellon University Database Group
 //
 //===----------------------------------------------------------------------===//
 
 #include "codegen/deleter.h"
+
 #include "codegen/transaction_runtime.h"
 #include "concurrency/transaction_manager_factory.h"
 #include "storage/data_table.h"
 
 namespace peloton {
 namespace codegen {
 
-void Deleter::Init(storage::DataTable *table,
-                   executor::ExecutorContext *executor_context) {
+Deleter::Deleter(storage::DataTable *table,
+                 executor::ExecutorContext *executor_context)
+    : table_(table), executor_context_(executor_context) {
   PELOTON_ASSERT(table != nullptr && executor_context != nullptr);
-  table_ = table;
-  executor_context_ = executor_context;
+}
+
+void Deleter::Init(Deleter &deleter, storage::DataTable *table,
+                   executor::ExecutorContext *executor_context) {
+  new (&deleter) Deleter(table, executor_context);
 }
 
 void Deleter::Delete(uint32_t tile_group_id, uint32_t tuple_offset) {
-  PELOTON_ASSERT(table_ != nullptr && executor_context_ != nullptr);
   LOG_TRACE("Deleting tuple <%u, %u> from table '%s' (db ID: %u, table ID: %u)",
             tile_group_id, tuple_offset, table_->GetName().c_str(),
             table_->GetDatabaseOid(), table_->GetOid());
@@ -36,23 +40,73 @@ void Deleter::Delete(uint32_t tile_group_id, uint32_t tuple_offset) {
   auto *tile_group_header = tile_group->GetHeader();
 
   auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
-  auto is_owner = TransactionRuntime::IsOwner(*txn, tile_group_header,
-                                              tuple_offset);
-  bool acquired_ownership = false;
-  if (is_owner == false) {
-    acquired_ownership = TransactionRuntime::AcquireOwnership(
-        *txn, tile_group_header, tuple_offset);
-    if (!acquired_ownership)
-      return;
+
+  bool is_owner = txn_manager.IsOwner(txn, tile_group_header, tuple_offset);
+  bool is_written = txn_manager.IsWritten(txn, tile_group_header, tuple_offset);
+
+  ItemPointer old_location{tile_group_id, tuple_offset};
+
+  // Did the current transaction create this version we're deleting? If so, we
+  // can perform the deletion without inserting a new empty version.
+
+  if (is_owner && is_written) {
+    LOG_TRACE("The current transaction is the owner of the tuple");
+    txn_manager.PerformDelete(txn, old_location);
+    executor_context_->num_processed++;
+    return;
   }
 
+  // We didn't create this version. In order to perform the delete, we need to
+  // acquire ownership of the version. Let's check if we can do so.
+
+  bool is_ownable =
+      is_owner || txn_manager.IsOwnable(txn, tile_group_header, tuple_offset);
+  if (!is_ownable) {
+    // Version is not own-able. The transaction should be aborted as we cannot
+    // update the latest version.
+    LOG_TRACE("Tuple [%u-%u] isn't own-able. Failing transaction.",
+              tile_group_id, tuple_offset);
+    txn_manager.SetTransactionResult(txn, ResultType::FAILURE);
+    return;
+  }
+
+  // Version is own-able. Let's grab ownership of the version.
+
+  bool acquired_ownership =
+      is_owner ||
+      txn_manager.AcquireOwnership(txn, tile_group_header, tuple_offset);
+  if (!acquired_ownership) {
+    LOG_TRACE(
+        "Failed acquiring ownership of tuple [%u-%u]. Failing transaction.",
+        tile_group_id, tuple_offset);
+    txn_manager.SetTransactionResult(txn, ResultType::FAILURE);
+    return;
+  }
+
+  // We've acquired ownership of the latest version, and it isn't locked by any
+  // other threads. Now, let's insert an empty version.
+
   ItemPointer new_location = table_->InsertEmptyVersion();
+
+  // Insertion into the table may fail. PerformDelete() should not be called if
+  // the insertion fails. At this point, we've acquired a write lock on the
+  // version, but since it is not in the write set (since we haven't yet put it
+  // into the write set), the acquired lock can't be released when the
+  // transaction is aborted. The YieldOwnership() function helps us release the
+  // acquired write lock.
+
   if (new_location.IsNull()) {
-    TransactionRuntime::YieldOwnership(*txn, tile_group_header, tuple_offset);
+    LOG_TRACE("Failed to insert new tuple. Failing transaction.");
+    if (!is_owner) {
+      // If ownership was acquired, we release it here, thus releasing the
+      // write lock.
+      txn_manager.YieldOwnership(txn, tile_group_header, tuple_offset);
+    }
+    txn_manager.SetTransactionResult(txn, ResultType::FAILURE);
     return;
   }
 
-  ItemPointer old_location(tile_group_id, tuple_offset);
+  // All is well
   txn_manager.PerformDelete(txn, old_location, new_location);
   executor_context_->num_processed++;
 }

src/include/codegen/deleter.h

@@ -38,20 +38,20 @@ namespace codegen {
 // class is initialized once (through Init()) outside the main loop.
 class Deleter {
  public:
+  // Constructor
+  Deleter(storage::DataTable *table,
+          executor::ExecutorContext *executor_context);
+
   // Initializer this deleter instance using the provided transaction and table.
   // All tuples to be deleted occur within the provided transaction are from
   // the provided table
-  void Init(storage::DataTable *table,
-            executor::ExecutorContext *executor_context);
+  static void Init(Deleter &deleter, storage::DataTable *table,
+                   executor::ExecutorContext *executor_context);
 
   // Delete the tuple within the provided tile group ID (unique) at the provided
   // offset from the start of the tile group.
   void Delete(uint32_t tile_group_id, uint32_t tuple_offset);
 
- private:
-  // Can't construct
-  Deleter() : table_(nullptr), executor_context_(nullptr) {}
-
  private:
   // The table the tuples are deleted from
   storage::DataTable *table_;