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
2 changes: 2 additions & 0 deletions include/storage/buffer_pool_manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ class BufferPoolManager {

bool close_file(const std::string& file_name) { return storage_manager_.close_file(file_name); }

bool delete_file(const std::string& file_name);

/**
* @brief Flush all pages in the pool to disk
*/
Expand Down
10 changes: 9 additions & 1 deletion include/storage/heap_table.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,16 @@ class HeapTable {
/** @brief Removes the physical heap file */
bool drop();

private:
/**
* @brief Read a page from the heap file into buffer
* @note Bypasses MVCC and cached_page_ invariants — test use only
*/
bool read_page(uint32_t page_num, char* buffer) const;

/**
* @brief Write a buffer into a page of the heap file
* @note Bypasses MVCC and cached_page_ invariants — test use only
*/
bool write_page(uint32_t page_num, const char* buffer);
};

Expand Down
7 changes: 7 additions & 0 deletions include/storage/storage_manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,13 @@ class StorageManager {
*/
bool create_dir_if_not_exists();

/**
* @brief Delete a file from storage
* @param filename The relative name of the file
* @return true if deleted, false otherwise
*/
bool delete_file(const std::string& filename);

private:
std::string data_dir_;
std::unordered_map<std::string, std::unique_ptr<std::fstream>> open_files_;
Expand Down
37 changes: 37 additions & 0 deletions src/storage/buffer_pool_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,43 @@ bool BufferPoolManager::delete_page(const std::string& file_name, uint32_t page_
return true;
}

bool BufferPoolManager::delete_file(const std::string& file_name) {
{
const std::scoped_lock<std::mutex> lock(latch_);

const uint32_t file_id = get_file_id_internal(file_name);

// Evict all pages belonging to this file
for (auto it = page_table_.begin(); it != page_table_.end();) {
if (it->first.file_id == file_id) {
const uint32_t frame_id = it->second;
Page* const page = &pages_[frame_id];
if (page->pin_count_ > 0) {
return false; // Cannot delete while pages are pinned
}
// Flush dirty pages before removing
if (page->is_dirty_) {
storage_manager_.write_page(file_name, page->page_id_, page->get_data());
page->is_dirty_ = false;
}
replacer_.pin(frame_id);
page->page_id_ = 0;
page->file_name_ = "";
page->pin_count_ = 0;
free_list_.push_back(frame_id);
it = page_table_.erase(it);
} else {
++it;
}
}

// Remove file_id mapping
file_id_map_.erase(file_name);
}

return storage_manager_.delete_file(file_name);
}
Comment on lines +230 to +265
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

Partial-eviction state leak when a pinned page is encountered mid-iteration.

If the loop has already evicted frames for earlier pages of file_id and then hits a still-pinned page, the early return false on line 242 leaves the buffer pool in an inconsistent state: those earlier frames have already been cleared (file_name_ = "", page_id_ = 0), pushed onto free_list_, and erased from page_table_, yet the file has not been deleted and callers are told the operation failed. Subsequent fetches for the still-existing file will silently re-read those pages from disk (OK), but the caller has no way to recover or know which pages were dropped, and the in-flight pinned reader on the remaining page is now operating on a file that the caller is likely about to retry deleting.

Prefer a two-pass approach: first scan page_table_ to verify no frame for file_id has pin_count_ > 0, then perform the evictions. That keeps the "fails if pinned" contract atomic with respect to buffer-pool state.

🛡️ Suggested two-pass implementation
 bool BufferPoolManager::delete_file(const std::string& file_name) {
     {
         const std::scoped_lock<std::mutex> lock(latch_);
 
         const uint32_t file_id = get_file_id_internal(file_name);
 
-        // Evict all pages belonging to this file
-        for (auto it = page_table_.begin(); it != page_table_.end();) {
-            if (it->first.file_id == file_id) {
-                const uint32_t frame_id = it->second;
-                Page* const page = &pages_[frame_id];
-                if (page->pin_count_ > 0) {
-                    return false;  // Cannot delete while pages are pinned
-                }
-                // Flush dirty pages before removing
-                if (page->is_dirty_) {
-                    storage_manager_.write_page(file_name, page->page_id_, page->get_data());
-                    page->is_dirty_ = false;
-                }
-                replacer_.pin(frame_id);
-                page->page_id_ = 0;
-                page->file_name_ = "";
-                page->pin_count_ = 0;
-                free_list_.push_back(frame_id);
-                it = page_table_.erase(it);
-            } else {
-                ++it;
-            }
-        }
+        // First pass: fail fast if any page is still pinned, leaving state untouched.
+        for (const auto& [key, frame_id] : page_table_) {
+            if (key.file_id == file_id && pages_[frame_id].pin_count_ > 0) {
+                return false;
+            }
+        }
+
+        // Second pass: evict all matching frames.
+        for (auto it = page_table_.begin(); it != page_table_.end();) {
+            if (it->first.file_id != file_id) {
+                ++it;
+                continue;
+            }
+            const uint32_t frame_id = it->second;
+            Page* const page = &pages_[frame_id];
+            // No need to flush — file is about to be deleted.
+            replacer_.pin(frame_id);
+            page->page_id_ = 0;
+            page->file_name_.clear();
+            page->pin_count_ = 0;
+            page->is_dirty_ = false;
+            free_list_.push_back(frame_id);
+            it = page_table_.erase(it);
+        }
 
         // Remove file_id mapping
         file_id_map_.erase(file_name);
     }
 
     return storage_manager_.delete_file(file_name);
 }

Note: flushing dirty pages right before deleting the file is wasted I/O — the diff above also drops the flush, matching the "is_dirty=false" asymmetry already documented in HeapTable::drop().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/storage/buffer_pool_manager.cpp` around lines 230 - 265, The delete_file
implementation (BufferPoolManager::delete_file) can leave the buffer pool
partially mutated if it returns early upon encountering a pinned page; first
scan page_table_ for any entries with matching file_id whose pages_ have
pin_count_ > 0 and return false if any are found, and only after that
second-pass do the actual evictions: iterate page_table_ again to flush (if
needed), call replacer_.pin(frame_id), clear
pages_[frame_id]->page_id_/file_name_/pin_count_, push frame_id onto free_list_,
and erase the page_table_ entries; finally erase file_id_map_ and call
storage_manager_.delete_file(file_name). Ensure no state is mutated before the
pinned-page check so the operation is atomic with respect to buffer-pool state.


void BufferPoolManager::flush_all_pages() {
const std::scoped_lock<std::mutex> lock(latch_);

Expand Down
5 changes: 4 additions & 1 deletion src/storage/heap_table.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -740,12 +740,15 @@ bool HeapTable::create() {
}

bool HeapTable::drop() {
// Unpin the insert-cached page (is_dirty=false since we're dropping the table)
// Note: this is asymmetric with the destructor which uses is_dirty=true -
// here we skip writes since the entire file will be deleted anyway.
if (cached_page_ != nullptr) {
bpm_.unpin_page_by_id(file_id_, cached_page_id_, false);
cached_page_ = nullptr;
}
static_cast<void>(bpm_.close_file(filename_));
return (std::remove(filename_.c_str()) == 0);
return bpm_.delete_file(filename_);
}

bool HeapTable::read_page(uint32_t page_num, char* buffer) const {
Expand Down
16 changes: 16 additions & 0 deletions src/storage/storage_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,22 @@ bool StorageManager::create_dir_if_not_exists() {
return true;
}

/**
* @brief Delete a file from storage
*/
bool StorageManager::delete_file(const std::string& filename) {
// Close any open handle first to avoid stale fstream issues
auto it = open_files_.find(filename);
if (it != open_files_.end()) {
if (it->second->is_open()) {
it->second->close();
}
open_files_.erase(it);
}
const std::string filepath = get_full_path(filename);
return std::remove(filepath.c_str()) == 0;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

} // namespace cloudsql::storage

/** @} */
Loading
Loading