-
Notifications
You must be signed in to change notification settings - Fork 94
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
Use updatePage function for write operations #2599
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2599 +/- ##
=======================================
Coverage 93.41% 93.41%
=======================================
Files 1089 1088 -1
Lines 42095 42080 -15
=======================================
- Hits 39322 39311 -11
+ Misses 2773 2769 -4 ☔ View full report in Codecov by Sentry. |
TypeUtils::encodeOverflowPtr(diskDstString.overflowPtr, | ||
updatedPageInfoAndWALPageFrame.originalPageIdx, updatedPageInfoAndWALPageFrame.posInPage); | ||
bool insertingNewPage = false; | ||
if (originalPageCursor.pageIdx >= fileHandle->getNumPages()) { |
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.
Isn't the logic here serving similar functionality with addNewPageIfNecessaryWithoutLock(len);
? Why separate them in different places?
Also, I think you also need to check if the originalPageCursor.elemPosInPage + len
is exceeding 4KB.
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.
Yes, indeed. I was trying to keep things as close as possible to the old code since it wasn't working, however we may as well clean up that too.
I think the existing checks are sufficient. It's checking that the length is no greater than 4KB, and if originalPageCursor.elemPosInPage + len
is greater than 4KB addNewPageIfNecessaryWithoutLock
will add a new page.
fc2c35a
to
2d630d4
Compare
817b044
to
002f467
Compare
src/storage/store/column.cpp
Outdated
originalPageCursor.pageIdx, insertingNewPage, *dataFH, dbFileID, *bufferManager, *wal); | ||
return {walPageIdxAndFrame, originalPageCursor.elemPosInPage}; | ||
DBFileUtils::updatePage(*dataFH, dbFileID, originalPageCursor.pageIdx, insertingNewPage, | ||
*bufferManager, *wal, [&](auto frame) { writeOp(frame, offsetInChunk); }); |
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.
Issue was passing the offsetInChunk instead of the originalPageCursor.elemPosInPage
.
51d9874
to
fc105b5
Compare
fc105b5
to
2203b62
Compare
Use the
updatePage
function for write operations instead of manually handling pinning and unpinning.Following up #2592, particularly #2592 (review).
I was mistaken about insertNewPage being redundant. It's doing some redundant work, however we need to call at least
fileHandle.addNewPage()
andwal.logPageInsertRecord(...)
as far as I understand.One option could be to use insertNewPage's insertOp so that when inserting we use that exclusively instead of
updatePage
, though I'm mildly concerned that we're not actually pinning the desired page, but rather the added one, which isn't necessarily the same. Otherwise, maybe we should just add a stripped-down version of the function, or have updatePage do that automatically when you passisInsertingNewPage
as true (or even move the check to determine that intoupdatePage
).There is one test which fails about half the time with these changes. I'm not really sure why, as this should be identical to the original; I must have made a mistake somewhere. But I thought I'd open this so it isn't forgotten and in case I'm missing something obvious.