Thread: [PATCH] Fix references in comments, and sync up heap_page_is_all_visible() with heap_page_prune_and_freeze()

While working on [1] I found an outdated comment in heap_page_is_all_visible() and two other small fixes.

0001: Updates that comment so future authors know that this "stripped down function" should retain the logic in
heap_page_prune_and_freeze(),not lazy_scan_prune() as was the case before 6dbb490. 
0002: Mimics the same loop logic as in heap_page_is_all_visible() so as to a) stay in sync and b) benefit from the
mentionedCPU prefetching optimization. 
0003: Moves the ItemSetPointer() just a bit further down in the function again to a) stay in sync and b) to sometimes
avoidthat tiny overhead. 

best,

-greg

PS: per-community standards I've switched to my personal email address rather than gregburd@amazon.com

[1] https://www.postgresql.org/message-id/flat/78574B24-BE0A-42C5-8075-3FA9FA63B8FC@amazon.com

On Tue, May 06, 2025 at 03:39:07PM +0000, Gregory Burd wrote:
> 0001: Updates that comment so future authors know that this "stripped
> down function" should retain the logic in heap_page_prune_and_freeze(),
> not lazy_scan_prune() as was the case before 6dbb490.

Hm.  It certainly had some resemblance before commit 6dbb490, but looking
at the two code paths now, I'm not sure whether this comment is useful
anymore.  I do think it's more accurate to point to
heap_page_prune_and_freeze(), though.  And there's still an assertion in
lazy_scan_prune() to make sure things are in agreement.  Perhaps we should
rewrite the comment to something like

    * This is a specialized version of the logic from
    * heap_page_prune_and_freeze().  If you change anything here, make sure
    * that everything says in sync.  Note that an assertion in
    * lazy_scan_prune() calls us to verify that everybody still agrees.  Be
    * sure to avoid introducing new side effects here.

> 0002: Mimics the same loop logic as in heap_page_is_all_visible() so as
> to a) stay in sync and b) benefit from the mentioned CPU prefetching
> optimization.
> 0003: Moves the ItemSetPointer() just a bit further down in the function
> again to a) stay in sync and b) to sometimes avoid that tiny overhead.

These look generally reasonable to me, but they might be v19 material at
this point.  Have you been able to measure the impact of 0002?  I didn't
see much discussion about this in the thread that changed it for what is
now known as heap_page_prune_and_freeze() [0].

[0] https://postgr.es/m/17255-14c0ac58d0f9b583%40postgresql.org

-- 
nathan