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.
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.
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.
benjaminwinger
force-pushed
the
update-functional
branch
from
fc2c35a
to
2d630d4
Compare
benjaminwinger
force-pushed
the
update-functional
branch
2 times, most recently
from
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.
Issue was passing the offsetInChunk instead of the originalPageCursor.elemPosInPage.
benjaminwinger
force-pushed
the
update-functional
branch
3 times, most recently
from
51d9874
to
fc105b5
Compare
benjaminwinger
force-pushed
the
update-functional
branch
from
fc105b5
to
2203b62
Compare
Use the
updatePagefunction 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 passisInsertingNewPageas 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.