Horton

Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)

Melanie Plageman <melanieplageman@gmail.com>

View thread
From: Melanie Plageman <melanieplageman@gmail.com>
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>, Andrey Borodin <x4mmm@yandex-team.ru>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Heikki Linnakangas <hlinnaka@iki.fi>, Chao Li <li.evan.chao@gmail.com>
Date: 2025-12-18T19:57:57Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove table_scan_analyze_next_tuple unneeded parameter OldestXmin

  2. Simplify visibility check in heap_page_would_be_all_visible()

  3. Eliminate use of cached VM value in lazy_scan_prune()

  4. Combine visibilitymap_set() cases in lazy_scan_prune()

  5. Fix const qualification in prune_freeze_setup()

  6. Simplify vacuum visibility assertion

  7. Split heap_page_prune_and_freeze() into helpers

  8. Assert that cutoffs are provided if freezing will be attempted

  9. Split PruneFreezeParams initializers to one field per line

  10. Refactor heap_page_prune_and_freeze() parameters into a struct

  11. Make heap_page_is_all_visible independent of LVRelState

  12. Inline TransactionIdFollows/Precedes[OrEquals]()

  13. Add helper for freeze determination to heap_page_prune_and_freeze

  14. Bump XLOG_PAGE_MAGIC after xl_heap_prune change

  15. Correct prune WAL record opcode name in comment

  16. Add error codes when vacuum discovers VM corruption

  17. Remove unused xl_heap_prune member, reason

  18. Remove unneeded VM pin from VM replay

  19. Add assert and log message to visibilitymap_set

  20. Add error codes to some corruption log messages

On Thu, Dec 18, 2025 at 1:07 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> On Thu, 18 Dec 2025 at 20:18, Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>
> > But you are right, I don't see any non-error code path where a heap
> > page would become empty (all line pointers set unused) and then not be
> > set all-visible. Only vacuum sets line pointers unused and if all the
> > line pointers are unused it will always set the page all-visible.
> >
> > I think, though, that if we error out in lazy_scan_prune() after
> > returning from heap_page_prune_and_freeze() such that we don't set the
> > empty page all-visible, we can end up with an empty page without
> > PD_ALL_VISIBLE set. You can see how this might work by patching the VM
> > set code in lazy_scan_prune() to skip empty pages.
>
> Thank you for your explanation!  I completely forgot that PD_ALL_VIS
> is a non-persistent change (hint bit). so its update can be trivially
> lost.
> The simplest real-life example is being killed just after returning
> from heap_page_prune_and_freeze, yes.
> PFA tap test covering lazy_scan_new_or_empty code path for
> empty-but-not-all-visible page

Cool test! I'm going to have to think more about whether or not it is
worth adding a whole new TAP test for this codepath. Is there an
existing TAP test we could add it to so we don't need to make a new
cluster, etc? How long does the test take to run? Obviously it will be
quite short, but every bit we add to the test suite counts. I don't
actually know how much overhead there is with injection points.

I was chatting with Andres and he mentioned there is one other case
where you can end up in this code path (empty page without
PD_ALL_VISIBLE set) and this case does actually trigger this code:

            if (RelationNeedsWAL(vacrel->rel) &&
                !XLogRecPtrIsValid(PageGetLSN(page)))
                log_newpage_buffer(buf, true);

If you are inserting to a new page and you successfully call
PageInit() (making the page no longer considered new by PageIsNew()
because pd_upper will be set) but you error out before actually
inserting the tuple, then you will have an empty page without
PD_ALL_VISIBLE set. And assuming you error out before emitting WAL,
the page will not have a valid LSN set. So you will hit that code
which calls log_newpage_buffer().

I would say this case is so narrow (the log_newpage_buffer() codepath
in lazy_scan_new_or_empty()), it's not worth the added test overhead,
but I just wanted to share what I learned about when this code could
be hit.

Previously it was more common in the bulk extension case to have empty
pages not set PD_ALL_VISIBLE because bulk extension would call
PageInit() on all of the pages it extended so all the pages except the
target page were empty (today they are not initialized so they go into
the PageIsNew() branch).

So, in both cases, it seems like the empty page not set PD_ALL_VISIBLE
mostly only hit if we previously errored out.

- Melanie