Skip to content

The HEAD chunk of a spilled record is never fed into the page free-space statistics #6154

Description

@lvca

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 CHUNK
byteWritten = 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:

updatePageStatistics(currentPage.pageId.getPageNumber(), availableSpaceForFirstChunk, -(byteWritten + INT_SERIALIZED_SIZE + LONG_SERIALIZED_SIZE + chunkSize));

Two things to get right rather than assume:

  1. 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.
  2. 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.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions