From 942f7c3e336300f9b122d823f873fc37053a364d Mon Sep 17 00:00:00 2001 From: Xu Date: Fri, 11 Aug 2023 22:37:44 +0800 Subject: [PATCH 1/3] feat(todo): change raw pointer to unique pointer for member variables in bustub_instance.h (#589) feat: change raw pointer to unique pointer for member variables in bustub_instance.h --- src/common/bustub_instance.cpp | 63 +++++++++++++--------------- src/include/common/bustub_instance.h | 19 +++++---- 2 files changed, 39 insertions(+), 43 deletions(-) diff --git a/src/common/bustub_instance.cpp b/src/common/bustub_instance.cpp index 723b510c1..53cb952a1 100644 --- a/src/common/bustub_instance.cpp +++ b/src/common/bustub_instance.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -15,7 +16,6 @@ #include "catalog/schema.h" #include "catalog/table_generator.h" #include "common/bustub_instance.h" -#include "common/enums/statement_type.h" #include "common/exception.h" #include "common/util/string_util.h" #include "concurrency/lock_manager.h" @@ -39,87 +39,90 @@ namespace bustub { auto BustubInstance::MakeExecutorContext(Transaction *txn, bool is_modify) -> std::unique_ptr { - return std::make_unique(txn, catalog_, buffer_pool_manager_, txn_manager_, lock_manager_, is_modify); + return std::make_unique(txn, catalog_.get(), buffer_pool_manager_.get(), txn_manager_.get(), + lock_manager_.get(), is_modify); } BustubInstance::BustubInstance(const std::string &db_file_name) { enable_logging = false; // Storage related. - disk_manager_ = new DiskManager(db_file_name); + disk_manager_ = std::make_unique(db_file_name); // Log related. - log_manager_ = new LogManager(disk_manager_); + log_manager_ = std::make_unique(disk_manager_.get()); // We need more frames for GenerateTestTable to work. Therefore, we use 128 instead of the default // buffer pool size specified in `config.h`. try { - buffer_pool_manager_ = new BufferPoolManager(128, disk_manager_, LRUK_REPLACER_K, log_manager_); + buffer_pool_manager_ = + std::make_unique(128, disk_manager_.get(), LRUK_REPLACER_K, log_manager_.get()); } catch (NotImplementedException &e) { std::cerr << "BufferPoolManager is not implemented, only mock tables are supported." << std::endl; buffer_pool_manager_ = nullptr; } // Transaction (txn) related. + lock_manager_ = std::make_unique(); - lock_manager_ = new LockManager(); + txn_manager_ = std::make_unique(lock_manager_.get(), log_manager_.get()); - txn_manager_ = new TransactionManager(lock_manager_, log_manager_); - - lock_manager_->txn_manager_ = txn_manager_; + lock_manager_->txn_manager_ = txn_manager_.get(); #ifndef __EMSCRIPTEN__ lock_manager_->StartDeadlockDetection(); #endif // Checkpoint related. - checkpoint_manager_ = new CheckpointManager(txn_manager_, log_manager_, buffer_pool_manager_); + checkpoint_manager_ = + std::make_unique(txn_manager_.get(), log_manager_.get(), buffer_pool_manager_.get()); - // Catalog. - catalog_ = new Catalog(buffer_pool_manager_, lock_manager_, log_manager_); + // Catalog related. + catalog_ = std::make_unique(buffer_pool_manager_.get(), lock_manager_.get(), log_manager_.get()); - // Execution engine. - execution_engine_ = new ExecutionEngine(buffer_pool_manager_, txn_manager_, catalog_); + // Execution engine related. + execution_engine_ = std::make_unique(buffer_pool_manager_.get(), txn_manager_.get(), catalog_.get()); } BustubInstance::BustubInstance() { enable_logging = false; // Storage related. - disk_manager_ = new DiskManagerUnlimitedMemory(); + disk_manager_ = std::make_unique(); // Log related. - log_manager_ = new LogManager(disk_manager_); + log_manager_ = std::make_unique(disk_manager_.get()); // We need more frames for GenerateTestTable to work. Therefore, we use 128 instead of the default // buffer pool size specified in `config.h`. try { - buffer_pool_manager_ = new BufferPoolManager(128, disk_manager_, LRUK_REPLACER_K, log_manager_); + buffer_pool_manager_ = + std::make_unique(128, disk_manager_.get(), LRUK_REPLACER_K, log_manager_.get()); } catch (NotImplementedException &e) { std::cerr << "BufferPoolManager is not implemented, only mock tables are supported." << std::endl; buffer_pool_manager_ = nullptr; } // Transaction (txn) related. + lock_manager_ = std::make_unique(); - lock_manager_ = new LockManager(); - - txn_manager_ = new TransactionManager(lock_manager_, log_manager_); + txn_manager_ = std::make_unique(lock_manager_.get(), log_manager_.get()); - lock_manager_->txn_manager_ = txn_manager_; + lock_manager_->txn_manager_ = txn_manager_.get(); #ifndef __EMSCRIPTEN__ lock_manager_->StartDeadlockDetection(); #endif // Checkpoint related. - checkpoint_manager_ = new CheckpointManager(txn_manager_, log_manager_, buffer_pool_manager_); + checkpoint_manager_ = + std::make_unique(txn_manager_.get(), log_manager_.get(), buffer_pool_manager_.get()); - // Catalog. - catalog_ = new Catalog(buffer_pool_manager_, lock_manager_, log_manager_); + // Catalog related. + catalog_ = std::make_unique(buffer_pool_manager_.get(), lock_manager_.get(), log_manager_.get()); - // Execution engine. - execution_engine_ = new ExecutionEngine(buffer_pool_manager_, txn_manager_, catalog_); + // Execution engine related. + execution_engine_ = std::make_unique(buffer_pool_manager_.get(), txn_manager_.get(), catalog_.get()); } void BustubInstance::CmdDisplayTables(ResultWriter &writer) { @@ -354,14 +357,6 @@ BustubInstance::~BustubInstance() { if (enable_logging) { log_manager_->StopFlushThread(); } - delete execution_engine_; - delete catalog_; - delete checkpoint_manager_; - delete log_manager_; - delete buffer_pool_manager_; - delete lock_manager_; - delete txn_manager_; - delete disk_manager_; } } // namespace bustub diff --git a/src/include/common/bustub_instance.h b/src/include/common/bustub_instance.h index 6a2b8fa68..7d660c04d 100644 --- a/src/include/common/bustub_instance.h +++ b/src/include/common/bustub_instance.h @@ -250,17 +250,18 @@ class BustubInstance { */ void GenerateMockTable(); - // TODO(chi): change to unique_ptr. Currently they're directly referenced by recovery test, so + // Currently the followings are directly referenced by recovery test, so // we cannot do anything on them until someone decides to refactor the recovery test. - DiskManager *disk_manager_; - BufferPoolManager *buffer_pool_manager_; - LockManager *lock_manager_; - TransactionManager *txn_manager_; - LogManager *log_manager_; - CheckpointManager *checkpoint_manager_; - Catalog *catalog_; - ExecutionEngine *execution_engine_; + std::unique_ptr disk_manager_; + std::unique_ptr buffer_pool_manager_; + std::unique_ptr lock_manager_; + std::unique_ptr txn_manager_; + std::unique_ptr log_manager_; + std::unique_ptr checkpoint_manager_; + std::unique_ptr catalog_; + std::unique_ptr execution_engine_; + /** Coordination for catalog */ std::shared_mutex catalog_lock_; auto GetSessionVariable(const std::string &key) -> std::string { From 50726c289762119ba4635ffa2aa28018d953782b Mon Sep 17 00:00:00 2001 From: Xu Date: Fri, 11 Aug 2023 22:38:01 +0800 Subject: [PATCH 2/3] chore(docs): improve overall format & resolve todo related to transaction part (#588) docs: improve overall format & resolve todo related to transaction part --- src/concurrency/transaction_manager.cpp | 1 + src/include/concurrency/transaction.h | 53 +++++++++++++++---- src/include/concurrency/transaction_manager.h | 5 +- 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/src/concurrency/transaction_manager.cpp b/src/concurrency/transaction_manager.cpp index da1c8e8bb..b1b06d057 100644 --- a/src/concurrency/transaction_manager.cpp +++ b/src/concurrency/transaction_manager.cpp @@ -20,6 +20,7 @@ #include "catalog/catalog.h" #include "common/macros.h" #include "storage/table/table_heap.h" + namespace bustub { void TransactionManager::Commit(Transaction *txn) { diff --git a/src/include/concurrency/transaction.h b/src/include/concurrency/transaction.h index df3a10fce..ab7e81a1c 100644 --- a/src/include/concurrency/transaction.h +++ b/src/include/concurrency/transaction.h @@ -30,19 +30,26 @@ namespace bustub { /** * Transaction states for 2PL: + * Running transactions could be aborted during either `GROWING` or `SHRINKING` stage. * * _________________________ + * | | * | v * GROWING -> SHRINKING -> COMMITTED ABORTED - * |__________|________________________^ + * | | ^ + * |___________|________________________| * * Transaction states for Non-2PL: + * Running transactions could only be aborted during `GROWING` stage, since there is no `SHRINKING` stage. + * * __________ + * | | * | v * GROWING -> COMMITTED ABORTED - * |_________________________^ + * | ^ + * |_________________________| * - **/ + */ enum class TransactionState { GROWING, SHRINKING, COMMITTED, ABORTED }; /** @@ -126,14 +133,17 @@ enum class AbortReason { * TransactionAbortException is thrown when state of a transaction is changed to ABORTED */ class TransactionAbortException : public std::exception { - txn_id_t txn_id_; - AbortReason abort_reason_; - public: explicit TransactionAbortException(txn_id_t txn_id, AbortReason abort_reason) : txn_id_(txn_id), abort_reason_(abort_reason) {} + + /** @return this transaction id */ auto GetTransactionId() -> txn_id_t { return txn_id_; } + + /** @return the abort reason for this transaction */ auto GetAbortReason() -> AbortReason { return abort_reason_; } + + /** @return the detailed information of abort reason */ auto GetInfo() -> std::string { switch (abort_reason_) { case AbortReason::LOCK_ON_SHRINKING: @@ -155,10 +165,17 @@ class TransactionAbortException : public std::exception { return "Transaction " + std::to_string(txn_id_) + " aborted because attempted lock upgrade is incompatible\n"; case AbortReason::ATTEMPTED_UNLOCK_BUT_NO_LOCK_HELD: return "Transaction " + std::to_string(txn_id_) + " aborted because attempted to unlock but no lock held \n"; + default: + // Unknown AbortReason + throw bustub::Exception("Unknown abort reason for transaction " + std::to_string(txn_id_)); } - // Todo: Should fail with unreachable. - return ""; + // This is impossible + assert(false); } + + private: + txn_id_t txn_id_; + AbortReason abort_reason_; }; /** @@ -248,22 +265,30 @@ class Transaction { return x_row_lock_set_; } - /** @return the set of resources under a shared lock */ + /** @return the set of table resources under a shared lock */ inline auto GetSharedTableLockSet() -> std::shared_ptr> { return s_table_lock_set_; } + + /** @return the set of table resources under a exclusive lock */ inline auto GetExclusiveTableLockSet() -> std::shared_ptr> { return x_table_lock_set_; } + + /** @return the set of table resources under a intention shared lock */ inline auto GetIntentionSharedTableLockSet() -> std::shared_ptr> { return is_table_lock_set_; } + + /** @return the set of table resources under a intention exclusive lock */ inline auto GetIntentionExclusiveTableLockSet() -> std::shared_ptr> { return ix_table_lock_set_; } + + /** @return the set of table resources under a shared intention exclusive lock */ inline auto GetSharedIntentionExclusiveTableLockSet() -> std::shared_ptr> { return six_table_lock_set_; } - /** @return true if rid (belong to table oid) is shared locked by this transaction */ + /** @return true if the specified row (belong to table oid) is shared locked by this transaction */ auto IsRowSharedLocked(const table_oid_t &oid, const RID &rid) -> bool { auto row_lock_set = s_row_lock_set_->find(oid); if (row_lock_set == s_row_lock_set_->end()) { @@ -272,7 +297,7 @@ class Transaction { return row_lock_set->second.find(rid) != row_lock_set->second.end(); } - /** @return true if rid (belong to table oid) is exclusive locked by this transaction */ + /** @return true if the specified row (belong to table oid) is exclusive locked by this transaction */ auto IsRowExclusiveLocked(const table_oid_t &oid, const RID &rid) -> bool { auto row_lock_set = x_row_lock_set_->find(oid); if (row_lock_set == x_row_lock_set_->end()) { @@ -281,22 +306,27 @@ class Transaction { return row_lock_set->second.find(rid) != row_lock_set->second.end(); } + /** @return true if the table (specified by oid) is intention shared locked by this transaction */ auto IsTableIntentionSharedLocked(const table_oid_t &oid) -> bool { return is_table_lock_set_->find(oid) != is_table_lock_set_->end(); } + /** @return true if the table (specified by oid) is shared locked by this transaction */ auto IsTableSharedLocked(const table_oid_t &oid) -> bool { return s_table_lock_set_->find(oid) != s_table_lock_set_->end(); } + /** @return true if the table (specified by oid) is intention exclusive locked by this transaction */ auto IsTableIntentionExclusiveLocked(const table_oid_t &oid) -> bool { return ix_table_lock_set_->find(oid) != ix_table_lock_set_->end(); } + /** @return true if the table (specified by oid) is exclusive locked by this transaction */ auto IsTableExclusiveLocked(const table_oid_t &oid) -> bool { return x_table_lock_set_->find(oid) != x_table_lock_set_->end(); } + /** @return true if the table (specified by oid) is shared intention exclusive locked by this transaction */ auto IsTableSharedIntentionExclusiveLocked(const table_oid_t &oid) -> bool { return six_table_lock_set_->find(oid) != six_table_lock_set_->end(); } @@ -340,6 +370,7 @@ class Transaction { /** The LSN of the last record written by the transaction. */ lsn_t prev_lsn_; + /** The latch for this transaction */ std::mutex latch_; /** Concurrent index: the pages that were latched during index operation. */ diff --git a/src/include/concurrency/transaction_manager.h b/src/include/concurrency/transaction_manager.h index 67ffbf952..ed7431b5d 100644 --- a/src/include/concurrency/transaction_manager.h +++ b/src/include/concurrency/transaction_manager.h @@ -71,11 +71,10 @@ class TransactionManager { void Abort(Transaction *txn); /** - * Global list of running transactions + * The transaction map is a global list of all the running transactions in the system. */ - - /** The transaction map is a global list of all the running transactions in the system. */ std::unordered_map txn_map_; + /** Coordination for the transaction map */ std::shared_mutex txn_map_mutex_; /** From df2976dfd895ae0f646260373bc8c35c171ae8a4 Mon Sep 17 00:00:00 2001 From: Xu Date: Fri, 11 Aug 2023 22:38:51 +0800 Subject: [PATCH 3/3] fix(test): ensure compatibility std::uniform_int_distribution for binary data generation & improve overall format (#587) fix: ensure compatibility std::uniform_int_distribution for binary data generation & improve overall format --- test/buffer/buffer_pool_manager_test.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/test/buffer/buffer_pool_manager_test.cpp b/test/buffer/buffer_pool_manager_test.cpp index 2d080ed1e..440c85cb9 100644 --- a/test/buffer/buffer_pool_manager_test.cpp +++ b/test/buffer/buffer_pool_manager_test.cpp @@ -13,6 +13,7 @@ #include "buffer/buffer_pool_manager.h" #include +#include #include #include @@ -29,7 +30,12 @@ TEST(BufferPoolManagerTest, DISABLED_BinaryDataTest) { std::random_device r; std::default_random_engine rng(r()); - std::uniform_int_distribution uniform_dist(0); + + constexpr int lower_bound = static_cast(std::numeric_limits::min()); + constexpr int upper_bound = static_cast(std::numeric_limits::max()); + // No matter if `char` is signed or unsigned by default, this constraint must be met + static_assert(upper_bound - lower_bound == 255); + std::uniform_int_distribution uniform_dist(lower_bound, upper_bound); auto *disk_manager = new DiskManager(db_name); auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager, k); @@ -44,7 +50,7 @@ TEST(BufferPoolManagerTest, DISABLED_BinaryDataTest) { char random_binary_data[BUSTUB_PAGE_SIZE]; // Generate random binary data for (char &i : random_binary_data) { - i = uniform_dist(rng); + i = static_cast(uniform_dist(rng)); } // Insert terminal characters both in the middle and at end @@ -65,17 +71,20 @@ TEST(BufferPoolManagerTest, DISABLED_BinaryDataTest) { EXPECT_EQ(nullptr, bpm->NewPage(&page_id_temp)); } - // Scenario: After unpinning pages {0, 1, 2, 3, 4} we should be able to create 5 new pages + // Scenario: After unpinning pages {0, 1, 2, 3, 4}, we should be able to create 5 new pages for (int i = 0; i < 5; ++i) { EXPECT_EQ(true, bpm->UnpinPage(i, true)); bpm->FlushPage(i); } for (int i = 0; i < 5; ++i) { EXPECT_NE(nullptr, bpm->NewPage(&page_id_temp)); + // Unpin the page here to allow future fetching bpm->UnpinPage(page_id_temp, false); } + // Scenario: We should be able to fetch the data we wrote a while ago. page0 = bpm->FetchPage(0); + ASSERT_NE(nullptr, page0); EXPECT_EQ(0, memcmp(page0->GetData(), random_binary_data, BUSTUB_PAGE_SIZE)); EXPECT_EQ(true, bpm->UnpinPage(0, true)); @@ -128,10 +137,11 @@ TEST(BufferPoolManagerTest, DISABLED_SampleTest) { // Scenario: We should be able to fetch the data we wrote a while ago. page0 = bpm->FetchPage(0); + ASSERT_NE(nullptr, page0); EXPECT_EQ(0, strcmp(page0->GetData(), "Hello")); // Scenario: If we unpin page 0 and then make a new page, all the buffer pages should - // now be pinned. Fetching page 0 should fail. + // now be pinned. Fetching page 0 again should fail. EXPECT_EQ(true, bpm->UnpinPage(0, true)); EXPECT_NE(nullptr, bpm->NewPage(&page_id_temp)); EXPECT_EQ(nullptr, bpm->FetchPage(0));