Fix stale child pointer bug in merge_branch_with_only_child #182
Conversation
✅ Heimdall Review Status
|
BrianBland
left a comment
BrianBland
left a comment
There was a problem hiding this comment.
Thanks, just had a minor request for more consistent argument naming to improve readability.
src/storage/engine.rs
Outdated
| fn merge_with_child_on_same_page( | ||
| &self, | ||
| slotted_page: &mut SlottedPageMut<'_>, | ||
| page_index: u8, |
There was a problem hiding this comment.
Recommend renaming this to parent_cell_index for consistency with the new child_cell_index
src/storage/engine.rs
Outdated
| // Delete both nodes and insert the merged one | ||
| slotted_page.delete_value(child_cell_index)?; | ||
| slotted_page.delete_value(page_index)?; | ||
|
|
||
| let only_child_node_index = slotted_page.insert_value(&only_child_node)?; | ||
| let new_cell_index = slotted_page.insert_value(&child_node)?; | ||
|
|
||
| // If we are the root of the page, we must insert at index 0 | ||
| if page_index == 0 { | ||
| assert_eq!(only_child_node_index, page_index); | ||
| assert_eq!(new_cell_index, page_index); | ||
| } |
There was a problem hiding this comment.
This is more a comment about the existing implementation, and not a critique of this changeset:
Ideally we should not be assuming and asserting the implementation details of the slotted page, specifically on insertion. It would be preferable to delete the child and then replace the parent's slot with the new node. For example:
slotted_page.delete_value(child_cell_index)?;
slotted_page.set_value(page_index, &child_node)?;There was a problem hiding this comment.
It seems like this is addressed in this PR: https://github.com/base/triedb/pull/183/changes . To avoid conflicts, I guess it is better to leave this one as it is.
|
Thanks @BrianBland for the feedbacks. |
When merging a branch node with its only remaining child on a different page, the child node was read and cached early in `merge_branch_with_only_child`. If the child's page had < 200 free bytes, `split_page` was called which moved some grandchildren to new pages and updated the child node's pointers at index 0. However, the stale cached node was then written back, overwriting `split_page`'s pointer updates. Fix: Defer reading the child node until after `split_page` completes. The helper functions now receive only the prefix to prepend, and read the child node fresh before modifying and writing it back. This ensures any pointer updates from `split_page` are preserved.
cffls
force-pushed
the
fix_invalid_cell
branch
from
da4fd46 to
688ac71
Compare
|
@BrianBland rebased and resolved the conflicts. |
BrianBland
left a comment
BrianBland
left a comment
There was a problem hiding this comment.
It looks like this PR is currently failing some linter checks. Could you please address these issues?
3 participants
When merging a branch node with its only remaining child on a different page, the child node was read and cached early in
merge_branch_with_only_child. If the child's page had < 200 free bytes,split_pagewas called which moved some grandchildren to new pages and updated the child node's pointers at index 0. However, the stale cached node was then written back, overwritingsplit_page's pointer updates.Fix: Defer reading the child node until after
split_pagecompletes. The helper functions now receive only the prefix to prepend, and read the child node fresh before modifying and writing it back. This ensures any pointer updates fromsplit_pageare preserved.