Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
68 changes: 50 additions & 18 deletions include/transaction/transaction_manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,41 +7,73 @@
#define CLOUDSQL_TRANSACTION_TRANSACTION_MANAGER_HPP

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

#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 {
class Catalog;
namespace storage {
class BufferPoolManager;
}
} // namespace cloudsql

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_;
public:
explicit TransactionManager(LockManager& lock_manager,
recovery::LogManager* log_manager = nullptr);

void undo_transaction(Transaction* txn);
~TransactionManager() = default;

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) {}
// 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_;
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_;
};

} // 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
Loading