From 56e67fca3e5e432334989589d8c608c98cbd3a28 Mon Sep 17 00:00:00 2001 From: Tianyu Li Date: Thu, 11 Apr 2019 14:22:13 -0400 Subject: [PATCH 1/3] Retry on transient update failure now. --- src/storage/data_table.cpp | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/storage/data_table.cpp b/src/storage/data_table.cpp index 94ba9f9193..8cd0095ac0 100644 --- a/src/storage/data_table.cpp +++ b/src/storage/data_table.cpp @@ -82,7 +82,7 @@ bool DataTable::Update(transaction::TransactionContext *const txn, const TupleSl "The input buffer cannot change the reserved columns, so it should have fewer attributes."); TERRIER_ASSERT(redo.NumColumns() > 0, "The input buffer should modify at least one attribute."); UndoRecord *const undo = txn->UndoRecordForUpdate(this, slot, redo); - UndoRecord *const version_ptr = AtomicallyReadVersionPtr(slot, accessor_); + UndoRecord *version_ptr = AtomicallyReadVersionPtr(slot, accessor_); // Since we disallow write-write conflicts, the version vector pointer is essentially an implicit // write lock on the tuple. @@ -101,13 +101,17 @@ bool DataTable::Update(transaction::TransactionContext *const txn, const TupleSl // Update the next pointer of the new head of the version chain undo->Next() = version_ptr; - if (!CompareAndSwapVersionPtr(slot, accessor_, version_ptr, undo)) { - // Mark this UndoRecord as never installed by setting the table pointer to nullptr. This is inspected in the - // TransactionManager's Rollback() and GC's Unlink logic - undo->Table() = nullptr; - return false; + while (!CompareAndSwapVersionPtr(slot, accessor_, version_ptr, undo)) { + // A failed compare and swap can be either a result of conflict, or simply interference from the GC. + // We need to check to find out + version_ptr = AtomicallyReadVersionPtr(slot, accessor_); + if (HasConflict(version_ptr, txn) || !Visible(slot, accessor_)) { + undo->Table() = nullptr; + return false; + } } + // Update in place with the new value. for (uint16_t i = 0; i < redo.NumColumns(); i++) { TERRIER_ASSERT(redo.ColumnIds()[i] != VERSION_POINTER_COLUMN_ID, @@ -165,7 +169,7 @@ bool DataTable::Delete(transaction::TransactionContext *const txn, const TupleSl // Create a redo txn->StageDelete(this, slot); UndoRecord *const undo = txn->UndoRecordForDelete(this, slot); - UndoRecord *const version_ptr = AtomicallyReadVersionPtr(slot, accessor_); + UndoRecord *version_ptr = AtomicallyReadVersionPtr(slot, accessor_); // Since we disallow write-write conflicts, the version vector pointer is essentially an implicit // write lock on the tuple. if (HasConflict(version_ptr, txn) || !Visible(slot, accessor_)) { @@ -178,12 +182,16 @@ bool DataTable::Delete(transaction::TransactionContext *const txn, const TupleSl // Update the next pointer of the new head of the version chain undo->Next() = version_ptr; - if (!CompareAndSwapVersionPtr(slot, accessor_, version_ptr, undo)) { - // Mark this UndoRecord as never installed by setting the table pointer to nullptr. This is inspected in the - // TransactionManager's Rollback() and GC's Unlink logic - undo->Table() = nullptr; - return false; + while (!CompareAndSwapVersionPtr(slot, accessor_, version_ptr, undo)) { + // A failed compare and swap can be either a result of conflict, or simply interference from the GC. + // We need to check to find out + version_ptr = AtomicallyReadVersionPtr(slot, accessor_); + if (HasConflict(version_ptr, txn) || !Visible(slot, accessor_)) { + undo->Table() = nullptr; + return false; + } } + // We have the write lock. Go ahead and flip the logically deleted bit to true accessor_.SetNull(slot, VERSION_POINTER_COLUMN_ID); return true; From e123683f20c27cdf525605a6b215ce214ceccad8 Mon Sep 17 00:00:00 2001 From: Tianyu Li Date: Thu, 11 Apr 2019 15:02:11 -0400 Subject: [PATCH 2/3] Refactor to reduce duplicate code. --- src/storage/data_table.cpp | 64 +++++++------------ test/storage/large_garbage_collector_test.cpp | 4 +- 2 files changed, 26 insertions(+), 42 deletions(-) diff --git a/src/storage/data_table.cpp b/src/storage/data_table.cpp index 8cd0095ac0..92d13fe1c7 100644 --- a/src/storage/data_table.cpp +++ b/src/storage/data_table.cpp @@ -82,35 +82,26 @@ bool DataTable::Update(transaction::TransactionContext *const txn, const TupleSl "The input buffer cannot change the reserved columns, so it should have fewer attributes."); TERRIER_ASSERT(redo.NumColumns() > 0, "The input buffer should modify at least one attribute."); UndoRecord *const undo = txn->UndoRecordForUpdate(this, slot, redo); - UndoRecord *version_ptr = AtomicallyReadVersionPtr(slot, accessor_); - - // Since we disallow write-write conflicts, the version vector pointer is essentially an implicit - // write lock on the tuple. - if (HasConflict(version_ptr, txn) || !Visible(slot, accessor_)) { - // Mark this UndoRecord as never installed by setting the table pointer to nullptr. This is inspected in the - // TransactionManager's Rollback() and GC's Unlink logic - undo->Table() = nullptr; - return false; - } - - // Store before-image before making any changes or grabbing lock - for (uint16_t i = 0; i < undo->Delta()->NumColumns(); i++) { - StorageUtil::CopyAttrIntoProjection(accessor_, slot, undo->Delta(), i); - } - - // Update the next pointer of the new head of the version chain - undo->Next() = version_ptr; - - while (!CompareAndSwapVersionPtr(slot, accessor_, version_ptr, undo)) { - // A failed compare and swap can be either a result of conflict, or simply interference from the GC. - // We need to check to find out + UndoRecord *version_ptr; + do { version_ptr = AtomicallyReadVersionPtr(slot, accessor_); + + // Since we disallow write-write conflicts, the version vector pointer is essentially an implicit + // write lock on the tuple. if (HasConflict(version_ptr, txn) || !Visible(slot, accessor_)) { + // Mark this UndoRecord as never installed by setting the table pointer to nullptr. This is inspected in the + // TransactionManager's Rollback() and GC's Unlink logic undo->Table() = nullptr; return false; } - } + // Store before-image before making any changes or grabbing lock + for (uint16_t i = 0; i < undo->Delta()->NumColumns(); i++) + StorageUtil::CopyAttrIntoProjection(accessor_, slot, undo->Delta(), i); + + // Update the next pointer of the new head of the version chain + undo->Next() = version_ptr; + } while (!CompareAndSwapVersionPtr(slot, accessor_, version_ptr, undo)); // Update in place with the new value. for (uint16_t i = 0; i < redo.NumColumns(); i++) { @@ -169,28 +160,21 @@ bool DataTable::Delete(transaction::TransactionContext *const txn, const TupleSl // Create a redo txn->StageDelete(this, slot); UndoRecord *const undo = txn->UndoRecordForDelete(this, slot); - UndoRecord *version_ptr = AtomicallyReadVersionPtr(slot, accessor_); - // Since we disallow write-write conflicts, the version vector pointer is essentially an implicit - // write lock on the tuple. - if (HasConflict(version_ptr, txn) || !Visible(slot, accessor_)) { - // Mark this UndoRecord as never installed by setting the table pointer to nullptr. This is inspected in the - // TransactionManager's Rollback() and GC's Unlink logic - undo->Table() = nullptr; - return false; - } - - // Update the next pointer of the new head of the version chain - undo->Next() = version_ptr; - - while (!CompareAndSwapVersionPtr(slot, accessor_, version_ptr, undo)) { - // A failed compare and swap can be either a result of conflict, or simply interference from the GC. - // We need to check to find out + UndoRecord *version_ptr; + do { version_ptr = AtomicallyReadVersionPtr(slot, accessor_); + // Since we disallow write-write conflicts, the version vector pointer is essentially an implicit + // write lock on the tuple. if (HasConflict(version_ptr, txn) || !Visible(slot, accessor_)) { + // Mark this UndoRecord as never installed by setting the table pointer to nullptr. This is inspected in the + // TransactionManager's Rollback() and GC's Unlink logic undo->Table() = nullptr; return false; } - } + + // Update the next pointer of the new head of the version chain + undo->Next() = version_ptr; + } while (!CompareAndSwapVersionPtr(slot, accessor_, version_ptr, undo)); // We have the write lock. Go ahead and flip the logically deleted bit to true accessor_.SetNull(slot, VERSION_POINTER_COLUMN_ID); diff --git a/test/storage/large_garbage_collector_test.cpp b/test/storage/large_garbage_collector_test.cpp index 0fedb00112..776bf304f7 100644 --- a/test/storage/large_garbage_collector_test.cpp +++ b/test/storage/large_garbage_collector_test.cpp @@ -21,10 +21,10 @@ class LargeGCTests : public TerrierTest { delete gc_; } - const uint32_t num_iterations = 10; + const uint32_t num_iterations = 50; const uint16_t max_columns = 20; const uint32_t initial_table_size = 1000; - const uint32_t num_txns = 1000; + const uint32_t num_txns = 10000; const uint32_t batch_size = 100; storage::BlockStore block_store_{1000, 1000}; storage::RecordBufferSegmentPool buffer_pool_{10000, 10000}; From 965161e59e2d1b3a0d77b8985c982d4ccd966c5d Mon Sep 17 00:00:00 2001 From: Tianyu Li Date: Thu, 11 Apr 2019 15:28:43 -0400 Subject: [PATCH 3/3] Revert test change --- test/storage/large_garbage_collector_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/storage/large_garbage_collector_test.cpp b/test/storage/large_garbage_collector_test.cpp index 776bf304f7..0fedb00112 100644 --- a/test/storage/large_garbage_collector_test.cpp +++ b/test/storage/large_garbage_collector_test.cpp @@ -21,10 +21,10 @@ class LargeGCTests : public TerrierTest { delete gc_; } - const uint32_t num_iterations = 50; + const uint32_t num_iterations = 10; const uint16_t max_columns = 20; const uint32_t initial_table_size = 1000; - const uint32_t num_txns = 10000; + const uint32_t num_txns = 1000; const uint32_t batch_size = 100; storage::BlockStore block_store_{1000, 1000}; storage::RecordBufferSegmentPool buffer_pool_{10000, 10000};