Support larger data updates in TablePage using free space#61
Support larger data updates in TablePage using free space#61google-labs-jules[bot] wants to merge 1 commit intomainfrom
Conversation
This change modifies `TablePage::update_record` to support updates where the new data is larger than the existing slot allocation. Instead of returning an error, it now attempts to allocate new space at the end of the page (fragmentation). It updates the slot offset and length to point to the new location and updates the free space pointer. Existing tests were updated to reflect this capability.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on f800c40..2dafe08
| Severity | Location | Issue | Delete |
|---|---|---|---|
 |
src/core/storage/engine/heap/table_page.rs:427 | Truncating cast causes data corruption |  |
✅ Files analyzed, no issues (1)
• src/core/storage/engine/heap/tests/table_page_tests.rs
| // Since we are updating, num_records stays the same. | ||
| let end_of_slot_array = | ||
| SLOTS_ARRAY_DATA_OFFSET + (num_records as usize * Slot::SERIALIZED_SIZE); | ||
| let record_data_write_offset = (end_of_slot_array as u16).max(current_free_space_ptr); |
There was a problem hiding this comment.
Unsafe truncating cast from usize to u16 can cause silent data corruption. The calculation end_of_slot_array = SLOTS_ARRAY_DATA_OFFSET + (num_records as usize * Slot::SERIALIZED_SIZE) can exceed u16::MAX (65535) when num_records is large. For example, if num_records=16384, then end_of_slot_array=65540, which truncates to 4 when cast to u16. This causes record_data_write_offset to be incorrectly small, leading to writing data at the wrong page offset and overwriting existing slot metadata or record data. The subsequent bounds check at line 438 only validates the END of the write is within page bounds, not that the START offset is correct. This can occur with corrupted page metadata or bugs in record counting logic elsewhere. The old code (removed by this PR) would have failed with an error for larger updates; this new code can silently corrupt data. Fix: Validate that end_of_slot_array fits in u16 before casting, or use checked conversion with try_into().
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Implemented logic in
TablePage::update_recordto handle cases where the new data is larger than the old data.Previously, this would return an error. Now, it checks for available free space in the page.
If space is available, it writes the new data to the free space area, updates the slot to point to the new location, and updates the free space pointer.
This effectively appends the new version of the record, leaving the old space as "garbage" (fragmentation) which is a standard heap file behavior without immediate compaction.
Updated
test_update_record_larger_size_errortotest_update_record_larger_size_successintable_page_tests.rs.PR created automatically by Jules for task 17215662096110040170 started by @ryancinsight
High-level PR Summary
This PR enhances the
TablePage::update_recordmethod to support updating records with larger data by utilizing available free space in the page. Previously, attempting to update a record with larger data would return an error. The new implementation writes the larger data to the free space area at the end of the page, updates the slot pointer to the new location, and adjusts the free space pointer accordingly. The old data remains in place as fragmentation, which is standard heap file behavior. The corresponding test has been updated from expecting an error to validating successful updates with larger data.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
src/core/storage/engine/heap/tests/table_page_tests.rssrc/core/storage/engine/heap/table_page.rs