You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Noticed during the review of #6153 (fix for #6149), which touches the adjacent accounting but deliberately did not widen its scope to this.
What is not accounted
LocalBucket.writeMultiPageRecord calls updatePageStatistics for every CONTINUATION chunk it places, but never for the HEAD chunk on the page the record spilled from:
// WRITE THE 1ST CHUNKbyteWritten = currentPage.writeNumber(newPosition, FIRST_CHUNK);
...
chunkSize = availableSpaceForFirstChunk - byteWritten - INT_SERIALIZED_SIZE - LONG_SERIALIZED_SIZE;
currentPage.writeInt(newPosition, chunkSize);
currentPage.writeByteArray(...); // <- no updatePageStatistics for this page
...
while (bufferSize > 0) {
...
updatePageStatistics(nextPage.pageId.getPageNumber(), spaceAvailableInCurrentPage, -chunkSize); // <- only here
}
The head chunk is the one that can consume the most space on the page it lands on: when the spilling record is the LAST record of its page, availableSpaceForFirstChunk is its own footprint plus the entire free tail, so a spill can take a page from kilobytes-free to zero-free without freeSpaceInPages hearing about it.
updateMultiPageRecord has the same shape: it accounts the chunks it relocates onto other pages, not the head chunk it rewrites in place.
Why it is only a hint, and why it is still worth fixing
freeSpaceInPages is a placement hint, not a correctness input: findAvailableSpace re-reads the page through getAvailableSpaceInPage/getFreeSpaceInPage and re-verifies before using a slot, so a stale entry costs a wasted candidate rather than a corrupt write. That is why this has gone unnoticed.
The cost is a wasted page probe per stale entry, and gatherPageStatistics only re-derives the map on its own cadence (MAX_TIMEOUT_GATHER_STATS, and only for pages below txPageCount - 2), so an entry for a page a spill has filled can stay wrong for a while. On a bulk load that spills a lot, the allocator keeps being pointed at pages that have nothing left.
Suggested fix
Account the head chunk where it is written, in both writeMultiPageRecord and updateMultiPageRecord, the same way #6153 accounts the bytes the in-page shift claims:
Do not double count against fix(#6149): a spilling record asks the page for the chunk header before falling back to a placeholder #6153. The spill path now calls updatePageStatistics(pageId, freeTailInPage, -slotEnlargement) for the bytes the shift claimed. Whatever is added here has to be the head chunk's footprint net of what the slot already occupied and of that enlargement, or the page's free space will be driven negative.
The delta convention is (availableSpaceBeforeTheWrite, -bytesTaken), and updatePageStatistics removes the entry when availableSpace + delta == 0 - which for a sealed page is exactly the outcome wanted, so the arithmetic has to land on 0 and not on a negative.
A test would assert through gatherPageStatistics/the allocator rather than the private map: after sealing a page with a spill (see BucketPageLayoutTestSupport.sealFirstPage), a subsequent insert must not be offered that page.
Noticed during the review of #6153 (fix for #6149), which touches the adjacent accounting but deliberately did not widen its scope to this.
What is not accounted
LocalBucket.writeMultiPageRecordcallsupdatePageStatisticsfor every CONTINUATION chunk it places, but never for the HEAD chunk on the page the record spilled from:The head chunk is the one that can consume the most space on the page it lands on: when the spilling record is the LAST record of its page,
availableSpaceForFirstChunkis its own footprint plus the entire free tail, so a spill can take a page from kilobytes-free to zero-free withoutfreeSpaceInPageshearing about it.updateMultiPageRecordhas the same shape: it accounts the chunks it relocates onto other pages, not the head chunk it rewrites in place.Why it is only a hint, and why it is still worth fixing
freeSpaceInPagesis a placement hint, not a correctness input:findAvailableSpacere-reads the page throughgetAvailableSpaceInPage/getFreeSpaceInPageand re-verifies before using a slot, so a stale entry costs a wasted candidate rather than a corrupt write. That is why this has gone unnoticed.The cost is a wasted page probe per stale entry, and
gatherPageStatisticsonly re-derives the map on its own cadence (MAX_TIMEOUT_GATHER_STATS, and only for pages belowtxPageCount - 2), so an entry for a page a spill has filled can stay wrong for a while. On a bulk load that spills a lot, the allocator keeps being pointed at pages that have nothing left.Suggested fix
Account the head chunk where it is written, in both
writeMultiPageRecordandupdateMultiPageRecord, the same way #6153 accounts the bytes the in-page shift claims:Two things to get right rather than assume:
updatePageStatistics(pageId, freeTailInPage, -slotEnlargement)for the bytes the shift claimed. Whatever is added here has to be the head chunk's footprint net of what the slot already occupied and of that enlargement, or the page's free space will be driven negative.(availableSpaceBeforeTheWrite, -bytesTaken), andupdatePageStatisticsremoves the entry whenavailableSpace + delta == 0- which for a sealed page is exactly the outcome wanted, so the arithmetic has to land on 0 and not on a negative.A test would assert through
gatherPageStatistics/the allocator rather than the private map: after sealing a page with a spill (seeBucketPageLayoutTestSupport.sealFirstPage), a subsequent insert must not be offered that page.