-
Notifications
You must be signed in to change notification settings - Fork 0
[ENGINE] Added wal api to Heap File + refactored heap file into several files #175
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
Conversation
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.
Pull request overview
This pull request refactors the heap file module into multiple files and adds WAL (Write-Ahead Logging) API support for atomic multi-page operations. The main changes include:
- Introduction of lock chain abstractions to manage page locking patterns
- Addition of PageCollector trait to gather modified pages for batch WAL logging
- Refactoring of heap file code into separate modules (error, fsm, lock_chains, pages, record)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
engine/src/heap_file/lock_chains.rs |
New file introducing lock chain traits and implementations for managing page locking patterns with WAL support |
engine/src/heap_file/mod.rs |
Major refactoring to use new lock chains and PageCollector; methods updated to support multi-page WAL operations |
engine/src/heap_file/record.rs |
Moved record-related types (RecordPtr, RecordTag, RecordFragment, RecordHandle) from mod.rs |
engine/src/heap_file/pages.rs |
Moved page-related types (Metadata, page headers, HeapPage) from mod.rs |
engine/src/heap_file/error.rs |
Extracted HeapFileError enum into separate error module |
engine/src/heap_file/fsm.rs |
Moved FreeSpaceMap implementation from mod.rs to dedicated module |
executor/src/statement_executor.rs |
Updated imports to reflect new module structure |
executor/src/error_factory.rs |
Updated imports to reflect new module structure |
engine/src/b_tree_node.rs |
Updated imports to reflect new module structure |
engine/src/b_tree.rs |
Updated imports to reflect new module structure |
engine/src/lib.rs |
Removed extra blank line |
Comments suppressed due to low confidence (5)
engine/src/heap_file/mod.rs:230
- The insert method only calls drop_pages if result.is_ok(), which means if an error occurs during insertion, the pages collected in the InsertPageLockChain are never dropped via drop_write_pages. These pinned write pages will be dropped individually when page_chain goes out of scope, but this may leak the pages or fail to properly notify the WAL system. Consider calling drop_pages unconditionally, or at minimum in an error case as well to ensure proper resource cleanup.
engine/src/heap_file/mod.rs:260 - The update operation creates an ExclusivePageLockChain but never calls drop_pages on it. Unlike the insert method which explicitly calls drop_pages, this method relies on Drop trait behavior which ExclusivePageLockChain does not implement. This means the pages held by the lock chain will be dropped individually instead of as a multi-page WAL operation, which breaks the intended atomicity guarantee for WAL logging. The fix is to either call page_chain.drop_pages() explicitly at the end of this method, or implement Drop trait for ExclusivePageLockChain to automatically call drop_pages.
engine/src/heap_file/mod.rs:277 - The delete operation creates an ExclusivePageLockChain but never calls drop_pages on it. Like the update method, this means pages will be dropped individually rather than as a multi-page WAL operation, breaking atomicity. This needs to either call page_chain.drop_pages() explicitly or rely on a Drop trait implementation.
engine/src/heap_file/mod.rs:340 - The add_column_migration creates a NoDroppingPageLockChain which accumulates pages throughout the entire operation but never calls drop_pages at the end. This means all the modified pages during the migration will be dropped individually rather than as a single multi-page WAL operation. This is particularly critical for schema migrations which should be atomic. The fix is to add chain.drop_pages() after the while loop completes successfully (line 339).
engine/src/heap_file/mod.rs:378 - The remove_column_migration creates a NoDroppingPageLockChain which accumulates pages throughout the entire operation but never calls drop_pages at the end. Like add_column_migration, this means all modified pages will be dropped individually instead of as a single multi-page WAL operation, breaking atomicity for schema migrations. The fix is to add chain.drop_pages() after the while loop completes successfully (line 378).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Main files that should be checked in PR:
lock_chains.rsandmod.rs, others are just moving code from one place to another