Cleanup hash index initialization #3577
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
hash-index-cleanup2
branch
2 times, most recently
from
085d492
to
f79b680
Compare
ray6080
approved these changes
Jun 4, 2024
| @@ -199,8 +188,8 @@ void OverflowFile::readFromDisk(transaction::TransactionType trxType, common::pa | |||
|
|
|||
| void OverflowFile::writePageToDisk(common::page_idx_t pageIdx, uint8_t* data) const { | |||
| if (pageIdx < numPagesOnDisk) { | |||
| // TODO: updatePage does an unnecessary read + copy. We just want to overwrite | |||
| DBFileUtils::updatePage(*getBMFileHandle(), dbFileID, pageIdx, false, *bufferManager, *wal, | |||
| DBFileUtils::updatePage(*getBMFileHandle(), dbFileID, pageIdx, | |||
There was a problem hiding this comment.
I feel we should rename the param bool isInsertingNewPage inside DBFileUtils::updatePage. The param is not intuitive. bool readPageBeforeUpdate should be better?
There was a problem hiding this comment.
Maybe. I'm hesitant to flip the meaning of the boolean though, as that's something easy to overlook. Maybe we should instead use the BufferManager::PageReadPolicy to make it more obvious what the parameter means?
benjaminwinger
force-pushed
the
hash-index-cleanup2
branch
from
f79b680
to
8e982f4
Compare
Benchmark ResultMaster commit hash:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following the rewriting of how hash index and disk array headers are created in #3557, I found that removing createEmptyHashIndexFiles was working with a few tweaks, so this makes the hash index able to initialize itself if the file provided is nonexistent/empty.
I've also removed a couple of unused functions (and functions which were only used in a couple of places and were better off written inline) and added manual zeroing of the padding in
SlotEntry<T>(which has padding if the type is smaller than 8 bytes).