Skip to content
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

bug: misues of INTERNAL_PAGE_SIZE in BPlusTreeInternalPage #562

Open
Roscky opened this issue Apr 16, 2023 · 5 comments
Open

bug: misues of INTERNAL_PAGE_SIZE in BPlusTreeInternalPage #562

Roscky opened this issue Apr 16, 2023 · 5 comments

Comments

@Roscky
Copy link

Roscky commented Apr 16, 2023

The definition of INTERNAL_PAGE_SIZE in BPlusTreeInternalPage is correct, but passing INTERNAL_PAGE_SIZE directly in b_plus_tree.h is incorrect because ValueType in BPlusTree is not the same as the ValueType corresponding to BPlusTreeInternalPage. Therefore, the macro definition of INTERNAL_PAGE_SIZE here leads to incorrect calculation results, which can cause wasted space in internal nodes.

The macro definition of INTERNAL_PAGE_SIZE in b_plus_internal_tree.h.

#define INTERNAL_PAGE_SIZE ((BUSTUB_PAGE_SIZE - INTERNAL_PAGE_HEADER_SIZE) / (sizeof(MappingType)))

INTERNAL_PAGE_SIZE is used in b_plus_tree.h

explicit BPlusTree(std::string name, page_id_t header_page_id, BufferPoolManager *buffer_pool_manager,
                     const KeyComparator &comparator, int leaf_max_size = LEAF_PAGE_SIZE,
                     int internal_max_size = INTERNAL_PAGE_SIZE);

Since the ValueType of MappingType in b_plus_tree.h is RID, which is incorrect for InternalPage, the INTERNAL_PAGE_SIZE will be miscalculated.

@infdahai

This comment was marked as spam.

@Roscky Roscky changed the title INTERNAL_PAGE_SIZE of BPlusTreeInternalPage bug: misues of INTERNAL_PAGE_SIZE in BPlusTreeInternalPage Apr 17, 2023
@infdahai
Copy link

infdahai commented Apr 18, 2023

[bustub-private/src/storage/index/b_plus_tree.cpp:22:BPlusTree] INFO  - Value_size:8
PageId_size:4
INTERNAL_PAGE_SIZE:254

[bustub-private/src/storage/page/b_plus_tree_leaf_page.cpp:38:Init] INFO  - Value_size:8
Mapping_size:16
LEAF_PAGE_SIZE:254

[bustub-private/src/storage/page/b_plus_tree_internal_page.cpp:37:Init] INFO  - Value_size:4
Mapping_size:12
INTERNAL_PAGE_SIZE:339

I add print statements and find that INTERNAL_PAGE_SIZE is right in b_plus_tree_internal_page.cpp but wrong in b_plus_tree.h . so the error occurs that causes INTERNAL_PAGE_SIZE is same to LEAF_PAGE_SIZE.

we can try to fix this in #517 and #547

@skyzh
Copy link
Member

skyzh commented Apr 18, 2023

Actually 254 should be the correct max size, because we need to align MappingType. The computation is wrong, but accidentally gets the correct result.

@Roscky Roscky closed this as completed Apr 19, 2023
@Roscky Roscky reopened this Apr 19, 2023
@Roscky
Copy link
Author

Roscky commented Apr 19, 2023

When the KeyType takes 8 bytes, the sizeof(MappingType) == 16 is correct. However, when the KeyType takes 1/2/4 bytes, the MappingType of internal node should align 4 bytes since the sizeof(KeyType) <= 4 and the sizeof(ValueType) == 4, so the sizeof(MappingType) == 8 and the internal_max_size should be more than 254.

@skyzh
Copy link
Member

skyzh commented Apr 19, 2023

Yes, that’s something we should fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants