Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,15 @@ jobs:
./transaction_manager_tests
./statement_tests
./recovery_tests
./recovery_manager_tests
./buffer_pool_tests

- name: Generate Coverage Report
if: matrix.sanitizer == 'address' && matrix.compiler == 'clang++'
run: |
cd build
lcov --directory . --capture --output-file coverage.info --gcov-tool "$GITHUB_WORKSPACE/.github/scripts/llvm-gcov.sh" --ignore-errors inconsistent
lcov --remove coverage.info '/usr/*' '*/tests/*' '*/CMakeFiles/*' --output-file filtered_coverage.info --ignore-errors inconsistent,unused
lcov --remove coverage.info '/usr/*' '*/tests/*' '*/CMakeFiles/*' '*/_deps/*' --output-file filtered_coverage.info --ignore-errors inconsistent,unused
genhtml filtered_coverage.info --output-directory out_coverage --ignore-errors inconsistent

- name: Upload Coverage
Expand Down
8 changes: 7 additions & 1 deletion include/storage/buffer_pool_manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#define CLOUDSQL_STORAGE_BUFFER_POOL_MANAGER_HPP

#include <list>
#include <memory>
#include <mutex>
#include <string>
#include <unordered_map>
Expand Down Expand Up @@ -95,6 +96,11 @@ class BufferPoolManager {
*/
void flush_all_pages();

/**
* @brief Get pointer to log manager
*/
[[nodiscard]] recovery::LogManager* get_log_manager() const { return log_manager_; }

private:
/**
* @brief Generates a unique string key for file and page mapping
Expand All @@ -111,7 +117,7 @@ class BufferPoolManager {
std::mutex latch_;

// The actual array of pages
std::vector<Page> pages_;
std::unique_ptr<Page[]> pages_;

// Replacer instance
LRUReplacer replacer_;
Expand Down
12 changes: 12 additions & 0 deletions include/storage/storage_manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,18 @@ class StorageManager {
*/
bool write_page(const std::string& filename, uint32_t page_num, const char* buffer);

/**
* @brief Allocate a new page in the database file
* @param filename Name of the database file
* @return index of the newly allocated page
*/
uint32_t allocate_page(const std::string& filename);

/**
* @brief Deallocate a page (stub for future use)
*/
static void deallocate_page(const std::string& filename, uint32_t page_num);

Comment on lines +83 to +87
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

deallocate_page currently exposes a misleading contract.

At Line 86, deallocate_page is static void, while the current implementation is a stub/no-op (src/storage/storage_manager.cpp Lines 184-187). Callers cannot detect failure, and persistent pages are never actually reclaimed.

💡 Suggested API direction
-    static void deallocate_page(const std::string& filename, uint32_t page_num);
+    bool deallocate_page(const std::string& filename, uint32_t page_num);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* @brief Deallocate a page (stub for future use)
*/
static void deallocate_page(const std::string& filename, uint32_t page_num);
/**
* `@brief` Deallocate a page (stub for future use)
*/
static bool deallocate_page(const std::string& filename, uint32_t page_num);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/storage/storage_manager.hpp` around lines 83 - 87, The
deallocate_page declaration exposes a misleading void/no-op contract; change the
API and implementation so callers can detect failure and persistent pages are
actually reclaimed: update the declaration of deallocate_page in
storage_manager.hpp (and corresponding implementation in
src/storage/storage_manager.cpp) from static void deallocate_page(const
std::string& filename, uint32_t page_num) to return a status (e.g., bool or an
error code/Status type) and implement real deallocation logic that
removes/reclaims the page and returns success/failure; update all callers of
StorageManager::deallocate_page to check the returned status and handle/report
errors accordingly.

/**
* @brief Check if a file exists
*/
Expand Down
4 changes: 2 additions & 2 deletions include/transaction/transaction.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,11 @@ class Transaction {
exclusive_locks_.insert(rid);
}

[[nodiscard]] const std::unordered_set<std::string>& get_shared_locks() {
[[nodiscard]] std::unordered_set<std::string> get_shared_lock_set() {
const std::scoped_lock<std::mutex> lock(lock_set_mutex_);
return shared_locks_;
}
[[nodiscard]] const std::unordered_set<std::string>& get_exclusive_locks() {
[[nodiscard]] std::unordered_set<std::string> get_exclusive_lock_set() {
const std::scoped_lock<std::mutex> lock(lock_set_mutex_);
return exclusive_locks_;
}
Expand Down
66 changes: 51 additions & 15 deletions include/transaction/transaction_manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,41 +7,77 @@
#define CLOUDSQL_TRANSACTION_TRANSACTION_MANAGER_HPP

#include <atomic>
#include <deque>
#include <memory>
#include <mutex>
#include <unordered_map>
#include <vector>

#include "catalog/catalog.hpp"
#include "recovery/log_manager.hpp"
#include "storage/buffer_pool_manager.hpp"
#include "transaction/lock_manager.hpp"
#include "transaction/transaction.hpp"

namespace cloudsql::transaction {

/**
* @brief Manages the lifecycle of transactions
*/
class TransactionManager {
private:
std::atomic<txn_id_t> next_txn_id_{1};
std::unordered_map<txn_id_t, std::unique_ptr<Transaction>> active_transactions_;
std::unordered_map<txn_id_t, std::unique_ptr<Transaction>> completed_transactions_;
LockManager& lock_manager_;
Catalog& catalog_;
storage::BufferPoolManager& bpm_;
recovery::LogManager* log_manager_;
std::mutex manager_latch_;

void undo_transaction(Transaction* txn);

public:
explicit TransactionManager(LockManager& lock_manager, Catalog& catalog,
storage::BufferPoolManager& bpm,
recovery::LogManager* log_manager = nullptr)
: lock_manager_(lock_manager), catalog_(catalog), bpm_(bpm), log_manager_(log_manager) {}
recovery::LogManager* log_manager = nullptr);

~TransactionManager() = default;

// Disable copy/move
TransactionManager(const TransactionManager&) = delete;
TransactionManager& operator=(const TransactionManager&) = delete;
TransactionManager(TransactionManager&&) = delete;
TransactionManager& operator=(TransactionManager&&) = delete;

/**
* @brief Start a new transaction
* @param level Isolation level
* @return Pointer to the new transaction
*/
Transaction* begin(IsolationLevel level = IsolationLevel::REPEATABLE_READ);

/**
* @brief Commit a transaction
*/
void commit(Transaction* txn);

/**
* @brief Abort a transaction
*/
void abort(Transaction* txn);

/**
* @brief Get transaction by ID
*/
Transaction* get_transaction(txn_id_t txn_id);

private:
LockManager& lock_manager_;
Catalog& catalog_;
storage::BufferPoolManager& bpm_;
recovery::LogManager* log_manager_;

std::atomic<txn_id_t> next_txn_id_{1};
std::mutex manager_latch_;

// All active transactions
std::unordered_map<txn_id_t, std::unique_ptr<Transaction>> active_transactions_;

// Transactions that have recently finished (for cleanup/safety)
std::deque<std::unique_ptr<Transaction>> completed_transactions_;

/**
* @brief Undo changes made by a transaction
*/
void undo_transaction(Transaction* txn);
};

} // namespace cloudsql::transaction
Expand Down
14 changes: 13 additions & 1 deletion src/catalog/catalog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <iostream>
#include <memory>
#include <optional>
#include <stdexcept>
#include <string>
#include <utility>
#include <vector>
Expand Down Expand Up @@ -70,6 +71,10 @@ bool Catalog::save(const std::string& filename) const {
* @brief Create a new table
*/
oid_t Catalog::create_table(const std::string& table_name, std::vector<ColumnInfo> columns) {
if (table_exists_by_name(table_name)) {
throw std::runtime_error("Table already exists: " + table_name);
}

auto table = std::make_unique<TableInfo>();
table->table_id = next_oid_++;
table->name = table_name;
Expand Down Expand Up @@ -139,6 +144,13 @@ oid_t Catalog::create_index(const std::string& index_name, oid_t table_id,
return 0;
}

auto& table = *table_opt.value();
for (const auto& existing_idx : table.indexes) {
if (existing_idx.name == index_name) {
throw std::runtime_error("Index already exists: " + index_name);
}
}

IndexInfo index;
index.index_id = next_oid_++;
index.name = index_name;
Expand All @@ -148,7 +160,7 @@ oid_t Catalog::create_index(const std::string& index_name, oid_t table_id,
index.is_unique = is_unique;

const oid_t id = index.index_id;
(*table_opt)->indexes.push_back(std::move(index));
table.indexes.push_back(std::move(index));
return id;
}

Expand Down
Loading