On Mon, Jun 9, 2025 at 6:22 PM Melanie Plageman <melanieplage...@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
