Merge free pages after rollback #5921
Conversation
Benchmark ResultMaster commit hash:
Other queries
|
|
Closing this as the root issue should be addressed by #5922 |
|
This also fixes the fragmentation issue after rollbacks though. Do we not want to handle that case? |
Hmm yes we probably should since otherwise there would be quite a bit of unnecessary fragmentation in this case. There's some minor stuff I'd like to change design wise but sure let's still do this change. |
src/storage/optimistic_allocator.cpp
Outdated
| @@ -22,6 +22,7 @@ void OptimisticAllocator::rollback() { | |||
| for (const auto& entry : optimisticallyAllocatedPages) { | |||
| pageManager.freeImmediatelyRewritablePageRange(pageManager.getDataFH(), entry); | |||
| } | |||
| pageManager.mergeFreePages(pageManager.getDataFH()); | |||
There was a problem hiding this comment.
Let's do this in LocalStorage::rollback() instead. There's still some overhead to merging the free pages (since we resort + reinsert all the entries in the FSM) so let's call this function fewer times if we can.
benjaminwinger
force-pushed
the
merge-free-pages-after-rollback
branch
from
3222c43 to
1581cf9
Compare
Previously only happened during a checkpoint But if a rollback occurs after data has been written, there would be a bunch of small page ranges in the free page cache from data appended to the end of the file, which can be merged and truncated
benjaminwinger
force-pushed
the
merge-free-pages-after-rollback
branch
from
1581cf9 to
95aac1c
Compare
| @@ -201,6 +202,10 @@ void FreeSpaceManager::finalizeCheckpoint(FileHandle* fileHandle) { | |||
| uncheckpointedFreePageRanges.clear(); | |||
| } | |||
|
|
|||
| void FreeSpaceManager::mergeFreePages(FileHandle* fileHandle) { | |||
| mergePageRanges(std::move(uncheckpointedFreePageRanges), fileHandle); | |||
There was a problem hiding this comment.
Let's rename mergeFreePages to something like finalizeRollback (this in the page manager too)? I'd like the use case of this function to be very specific since in general we shouldn't we worrying about the structure of the FSM in other modules.
I'd also like to change this to only merge the existing page ranges. I don't want this function to truncate the data file (or modify uncheckpointedFreePageRanges, although in practice it should always be empty in this case) since I'd like to maintain the assumption that we don't touch the data file or anything persistent outside of checkpoint. Maybe you can just refactor mergePageRanges() into two functions and just call the one that only does the merging.
There was a problem hiding this comment.
Let's rename mergeFreePages to something like finalizeRollback? I'd like the use case of this function to be very specific since in general we shouldn't we worrying about the structure of the FSM in other modules.
That sounds like a good idea to me
I don't want this function to truncate the data file ... since I'd like to maintain the assumption that we don't touch the data file or anything persistent outside of checkpoint.
What about for copies where we already touch the data file outside of checkpoints? E.g. in CopyNodeAfterPKErrorRollbackFlushedGroups the rollback occurs after we've written some of the data (even if none of the data is written successfully, we currently write to the node table before updating the index, where the primary key insertion fails in that test). I would think that particularly for duplicate PK errors (e.g. if you accidentally copy the same input twice) we wouldn't want to keep the newly added pages, since the user might not want to retry.
Though isn't this only being used when we've touched the data file? We usually delay allocating pages until we're ready to write them.
There was a problem hiding this comment.
I think it’s fine to leave the data as garbage, it should be either overwritten or truncated after the next checkpoint. Since we rollback local storage even if there is no COPY (e.g. after just issuing a rollback statement) I’d still like to keep those cases from touching disk if possible.
There was a problem hiding this comment.
maybe only calling finalizerollback if we detect changes in the fsm works (we already have the versioning in place). That might also be unnecessarily complicating things though, I’ll leave this decision to you.
Previously only happened during a checkpoint.
But if a rollback occurs after data has been written, there would be a bunch of small page ranges in the free page cache from data appended to the end of the file, which can be merged and truncated.
This works around an issue I was running into on my segmentation branch (
CopyNodeAfterPKErrorRollbackFlushedGroups; maybe related to the issue withNODE_GROUP_SIZE_LOG2=12in the FIXME, but I couldn't reproduce that), so I suspect that might be related to free space management (which probably hits this edge case by segmentation increasing the number of data chunks we're dealing with). I don't think it fixes that issue though.