-
Notifications
You must be signed in to change notification settings - Fork 0
Improve Stability and Achieve Zero-Warning Build #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
1fe09d7
781f414
9db9531
f0d19c4
a87e40c
3970f69
063dbdc
5c059a1
1bb00ba
095daf4
6da1b69
cf9f8b6
4f52041
b9bc812
ee83845
c524caf
41699c4
663e661
411ed55
1832684
a2fc8fc
dc8bca5
20618ed
2b5d41d
e273a01
2d6b89b
ab7bb77
e14ffd9
f7c8167
e201350
6e977e0
b806074
53e04b0
01373d4
1e35425
f31c54d
7dda2cb
94ee26c
aa6dd02
4a01b35
6f460d2
1af2b55
966cdc2
d39abf1
aaf2480
5cc33e0
2589c03
ec6cf73
db4a77d
e8c1f21
79476f6
467290a
cf58a30
ab473cf
55f363e
4a5f40a
7912e59
08fb671
40a3f45
a5c6c77
418c108
dc9e15f
3d90873
bc52bc3
132a92a
e531604
1b1851a
311b23d
8ac71f5
273317f
56c3310
f413cbd
3e81daf
f640393
8f28256
3d284ef
a5ec37e
c0ebc2d
7229d36
004062e
9a773c9
6a113c1
b520186
9943f43
2e1d485
8230821
f304e2a
b426603
a4c9d9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,74 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @file page.hpp | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @brief Base page class for the buffer pool manager | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #ifndef CLOUDSQL_STORAGE_PAGE_HPP | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #define CLOUDSQL_STORAGE_PAGE_HPP | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <cstring> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <shared_mutex> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <string> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <array> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace cloudsql::storage { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @class Page | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @brief Represents a single page in memory managed by the Buffer Pool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class Page { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static constexpr uint32_t PAGE_SIZE = 4096; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Page() { reset_memory(); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Page(const Page&) = delete; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Page& operator=(const Page&) = delete; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Page(Page&&) = delete; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Page& operator=(Page&&) = delete; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| virtual ~Page() = default; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Output raw pointer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [[nodiscard]] char* get_data() { return data_.data(); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [[nodiscard]] uint32_t get_page_id() const { return page_id_; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [[nodiscard]] int get_pin_count() const { return pin_count_; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [[nodiscard]] bool is_dirty() const { return is_dirty_; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [[nodiscard]] std::string get_file_name() const { return file_name_; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Read/Write Latch for concurrent access | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void r_lock() { rwlatch_.lock_shared(); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void r_unlock() { rwlatch_.unlock_shared(); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void w_lock() { rwlatch_.lock(); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void w_unlock() { rwlatch_.unlock(); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Get Page LSN (for recovery/WAL) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Assuming page layout reserves first 8 bytes (or 4 bytes) for PageLSN depending on implementation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // We will store LSN explicitly here to avoid parsing the header if not strictly needed, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // but typically it's written in the first 4 bytes of data_. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [[nodiscard]] int32_t get_lsn() const { return lsn_; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void set_lsn(int32_t lsn) { lsn_ = lsn; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| friend class BufferPoolManager; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void reset_memory() { data_.fill(0); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| std::array<char, PAGE_SIZE> data_{}; // Fixed size page array | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint32_t page_id_ = 0; // The logical page id within the file | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| std::string file_name_; // File this page belongs to | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int pin_count_ = 0; // Number of concurrent accesses | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool is_dirty_ = false; // Whether page has been modified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int32_t lsn_ = -1; // Page LSN, last modified operation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| std::shared_mutex rwlatch_; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+56
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check how BufferPoolManager uses reset_memory and whether it manually resets other fields
rg -n "reset_memory|page_id_\s*=|pin_count_\s*=|is_dirty_\s*=|file_name_\s*=" --type=cpp -C 3 -g '!tests/*'Repository: poyrazK/cloudSQL Length of output: 6138
While Proposed fix void reset_memory() {
data_.fill(0);
+ page_id_ = 0;
+ file_name_.clear();
+ pin_count_ = 0;
+ is_dirty_ = false;
+ lsn_ = -1;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } // namespace cloudsql::storage | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #endif // CLOUDSQL_STORAGE_PAGE_HPP | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,11 @@ | |
| #include "catalog/catalog.hpp" | ||
| #include "common/config.hpp" | ||
| #include "network/server.hpp" | ||
| #include "storage/storage_manager.hpp" | ||
| #include "parser/lexer.hpp" // Added by user instruction | ||
| #include "recovery/log_manager.hpp" // Added by user instruction | ||
| #include "recovery/recovery_manager.hpp" // Added by user instruction | ||
| #include "storage/buffer_pool_manager.hpp" | ||
| #include "storage/storage_manager.hpp" // Added by user instruction | ||
|
|
||
| namespace { | ||
|
|
||
|
|
@@ -117,18 +121,28 @@ int main(int argc, char* argv[]) { // NOLINT(bugprone-exception-escape) | |
| static_cast<void>(std::signal(SIGTERM, signal_handler)); | ||
|
|
||
| try { | ||
| /* Initialize storage manager */ | ||
| const auto storage_manager = std::make_unique<cloudsql::storage::StorageManager>(config.data_dir); | ||
|
|
||
| /* Initialize storage manager & buffer pool */ | ||
| auto disk_manager = std::make_unique<cloudsql::storage::StorageManager>(config.data_dir); | ||
| auto bpm = std::make_unique<cloudsql::storage::BufferPoolManager>(128, *disk_manager); | ||
| /* Initialize catalog */ | ||
| const auto catalog = cloudsql::Catalog::create(); | ||
| if (!catalog) { | ||
| std::cerr << "Failed to initialize catalog\n"; | ||
| return 1; | ||
| } | ||
|
|
||
| /* Initialize log manager & run recovery */ | ||
| auto log_manager = std::make_unique<cloudsql::recovery::LogManager>(config.data_dir + "/wal.log"); | ||
|
|
||
| std::cout << "Running Crash Recovery..." << std::endl; | ||
| cloudsql::recovery::RecoveryManager rm(*bpm, *catalog, *log_manager); | ||
| if (!rm.recover()) { | ||
| std::cerr << "Crash recovery failed. Restarting anyway.\n"; | ||
| } | ||
| log_manager->run_flush_thread(); | ||
|
Comment on lines
+166
to
+174
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log flush thread is started but never explicitly stopped — use-after-free / UB on shutdown.
Add an explicit stop/join of the flush thread before 🐛 Suggested teardown ordering /* Cleanup */
std::cout << "Shutting down...\n";
static_cast<void>(server->stop());
server.reset();
+
+ log_manager->stop_flush_thread(); // join before log_manager is destroyed🤖 Prompt for AI Agents |
||
|
|
||
| /* Initialize server */ | ||
| g_server = cloudsql::network::Server::create(config.port, *catalog, *storage_manager); | ||
| g_server = cloudsql::network::Server::create(config.port, *catalog, *bpm); | ||
| if (!g_server) { | ||
| std::cerr << "Failed to create server\n"; | ||
| return 1; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 86
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 1135
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 16323
🏁 Script executed:
# Check page.hpp for LSN comments and member declarations fd page.hpp -x cat -n {}Repository: poyrazK/cloudSQL
Length of output: 2789
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 6912
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 86
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 2713
🏁 Script executed:
# Look for buffer_pool or bpm files fd buffer_pool -e hpp -e cppRepository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 87
🏁 Script executed:
# List all files in include/storage ls -la include/storage/Repository: poyrazK/cloudSQL
Length of output: 433
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 87
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 1204
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 1150
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 163
🏁 Script executed:
# List what's in storage_manager.hpp cat include/storage/storage_manager.hppRepository: poyrazK/cloudSQL
Length of output: 2639
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 461
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 963
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 818
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 764
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 449
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 120
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 122
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 489
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 15134
🏁 Script executed:
# Check if there are any files without standard extensions find include/storage -type fRepository: poyrazK/cloudSQL
Length of output: 183
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
# Found it! Check buffer_pool_manager.hpp which is referenced but not in ls output cat include/storage/buffer_pool_manager.hppRepository: poyrazK/cloudSQL
Length of output: 131
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 1034
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 1034
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 24131
🏁 Script executed:
# Check btree_index.hpp line 14 more carefully head -20 include/storage/btree_index.hppRepository: poyrazK/cloudSQL
Length of output: 430
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 447
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
# Check if buffer_pool_tests.cpp exists ls -la tests/buffer_pool_tests.cppRepository: poyrazK/cloudSQL
Length of output: 134
🏁 Script executed:
Repository: poyrazK/cloudSQL
Length of output: 223
🏁 Script executed:
# Verify that buffer_pool_manager.hpp is truly missing ls -la include/storage/buffer_pool_manager.hppRepository: poyrazK/cloudSQL
Length of output: 146
🏁 Script executed:
# Check if there's a src/storage/buffer_pool_manager.cpp ls -la src/storage/Repository: poyrazK/cloudSQL
Length of output: 379
BufferPoolManager must serialize page LSN to persist it with page data.
The Page class stores
lsn_as a separate member, not as part ofdata_. Your comment correctly notes that LSNs are "typically written in the first 4 bytes ofdata_," but this implementation keeps them separate. WhenBufferPoolManagerflushes dirty pages to disk, it will write only thedata_buffer—thelsn_value will remain in memory and be lost on crash. Recovery will lack page LSNs needed to check durability of modifications.Ensure
BufferPoolManager.flush_page()serializes the currentlsn_value intodata_before callingStorageManager.write_page(), and deserializes it back on read, or document why a different recovery strategy is used.🤖 Prompt for AI Agents