Re: [PATCH] Check dead heap items before marking them unused

Thanks for looking at this.

I agree that I do not have a normal SQL-level reproducer for this state.
The motivation was not a known reachable SQL bug, but that the code is
about to perform an irreversible page change based on an invariant that
is only checked in assert builds.

On the checksum point, I think checksums only cover one subset of the
possible failures here. They would catch many on-disk page corruptions
when the page is read back, but they would not catch an internal bug that
puts the wrong offset into the dead-items store, a future pruning/tracking
bug, an in-memory page state problem, or systems running without
checksums. In those cases the existing Assert documents the invariant,
but production builds would proceed to ItemIdSetUnused().

That said, I agree with your concern that the patch adds another pass
over the dead offsets. My intent in placing the check before the critical
section was to report a plain ERROR before modifying the page, but I
understand that doing so has a cost on pages with many dead line pointers.

If the extra pass is considered too expensive for this path, perhaps the
right answer is to keep this as an Assert-only invariant, or to look for a
cheaper place to validate it. I would be interested in whether others
think this invariant violation should be treated as corruption worth a
runtime check, or whether the current Assert is considered sufficient.