On Mon, Jun 9, 2025 at 6:22 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> Reviewing your patch, I think there might be an issue still. You
> replaced has_lpdead_items with ndeleted. While ndeleted will count
> those items we set LP_UNUSED (which is what we want), it also counts
> LP_NORMAL items that vacuum sets LP_REDIRECT (see
> heap_prune_record_redirect()).
>
> Looking at PageGetHeapFreeSpace(), it seems like it only considers
> LP_UNUSED items as freespace. So, if an item is set LP_REDIRECT, there
> would be no need to update the FSM, I think.
I was wrong. Setting an item to LP_REDIRECT does free up space --
because there is no associated storage. PageGetFreeSpace() is
concerned with pd_upper and pd_lower. So after setting an item
LP_REDIRECT, we'll compactify_tuples() in PageRepairFragmentation(),
altering pd_upper/lower. Then future calls to PageGetHeapFreeSpace()
will report this updated number and we'll put that in the freespace
map. So, we can expect setting items LP_REDIRECT will result in new
freespace to report.
(I got confused by some code in PageGetHeapFreeSpace() that was
checking to make sure that, if there were >= MaxHeapTuplesPerPage line
pointers, that at least one of them is LP_UNUSED).
So, I think we should commit the fix you proposed.
The only question I have left is implementation: should we have
ndeleted as an output parameter of lazy_scan_prune() or have
lazy_scan_prune() return it (instead of void)?
In <= 16, heap_page_prune() returned the number of tuples deleted, so
I thought of maybe having lazy_scan_prune() do this. Though, maybe it
is confusing to have one result returned as the return value and the
others returned in output parameters unless there is something more
special about ndeleted. With heap_page_prune(), I think it was the
return value because that was kind of what heap_page_prune()
"accomplished".
- Melanie
