[PATCH] Fix references in comments, and sync up heap_page_is_all_visible() with heap_page_prune_and_freeze()
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t51542psql -h localhost -U postgresBuilt from patchset v1 (message #1), July 27, 2026 at 09:13 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t51542_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t51542_1 && git checkout t51542_1Patchset v1 (message #1) is on t51542_1
While working on [1]/messages/by-id/78574B24-BE0A-42C5-8075-3FA9FA63B8FC@amazon.com 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 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.
best,
-greg
PS: per-community standards I've switched to my personal email address rather than gregburd@amazon.com
[1]: /messages/by-id/78574B24-BE0A-42C5-8075-3FA9FA63B8FC@amazon.com
Attachments:
t51542_1v1-0002-Reverse-loop-to-match-counterpart-and-optimize-fo.patchapplication/octet-stream; filename=v1-0002-Reverse-loop-to-match-counterpart-and-optimize-fo.patch; name=v1-0002-Reverse-loop-to-match-counterpart-and-optimize-fo.patchDownload+8-4
v1-0001-Fix-references-in-comments-in-heap_page_is_all_vi.patchapplication/octet-stream; filename=v1-0001-Fix-references-in-comments-in-heap_page_is_all_vi.patch; name=v1-0001-Fix-references-in-comments-in-heap_page_is_all_vi.patchDownload+5-6
v1-0003-Move-ItemPointerSet-call-to-better-match-heap_pag.patchapplication/octet-stream; filename=v1-0003-Move-ItemPointerSet-call-to-better-match-heap_pag.patch; name=v1-0003-Move-ItemPointerSet-call-to-better-match-heap_pag.patchDownload+1-3
On Tue, May 6, 2025 at 11:08 PM Gregory Burd <greg@burd.me> wrote:
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 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.best,
-greg
PS: per-community standards I've switched to my personal email address
rather than gregburd@amazon.com[1]
/messages/by-id/78574B24-BE0A-42C5-8075-3FA9FA63B8FC@amazon.com
Hi, looks good for me.
Best regards,
Stepan Neretin.
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]/messages/by-id/17255-14c0ac58d0f9b583@postgresql.org.
[0]: /messages/by-id/17255-14c0ac58d0f9b583@postgresql.org
--
nathan