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-18T00:30:01Z
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

Attachments

Thanks for the review!

In addition to addressing your feedback, attached v28 includes a
number of small fixes to comments, commit messages, and other things.
Notably, I've added one new refactoring patch 0009, which reduces the
diff of 0010 -- using the GlobalVisState instead of OldestXmin for
page visibility -- even further.

On Wed, Dec 17, 2025 at 1:27 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> > I've done this. I've actually added three such verifications -- one
> > after each step where the VM is expected to change. It shouldn't be
> > very expensive, so I think it is okay. The way the test would fail if
> > the buffer wasn't correctly dirtied is that it would assert out -- so
> > the visibility map test wouldn't even have a chance to fail. But, I
> > think it is also okay to confirm that the expected things are
> > happening with the VM -- it just gives us extra coverage.
>
> +1 on extra coverage. Should we also do sql-level check that the VM
> indeed does not need to set PD_ALL_VISIBLE (check header bytes using
> pageinspect?).

That's an interesting idea. I checked and, AFAICT, there are no tests
currently directly comparing the flags column returned by the
pageinspect page_header() function to one of the flag values. I've
added the following to attached v28.

SELECT (flags & x'0004'::int) <> 0
        FROM page_header(get_raw_page('test_vac_unmodified_heap', 0));

But I'm not sure if it is weird/confusing to be comparing the flag
directly to the number 4 like this. I don't really want to bother with
adding another function to pageinspect returning the status of
PD_ALL_VISIBLE (like page_visible() or something).

> v27-0003 & v27-0004: I did not get the exact reason we introduced
> `identify_and_fix_vm_corruption` in 0003 and moved code in 0004 to
> another place. I can see we have this starting v25 of patch set. Well,
> maybe this is not an issue at all...

It's mostly for ease of review. This is a pretty sensitive area of
code, so I thought it would be easier for the reviewer to confirm
correctness if I split it up. Andres had mentioned that the commit was
hard to review because so many different things were happening.

In v27, 0003 moves the VM clear code into a helper. 0004 and 0005
moves all the VM setting/clearing code to
heap_page_prune_and_freeze(). And 0006 actually sets the VM in the
same critical section as pruning/freezing and emits a single WAL
record.

I'm not really sure which commits should stay independent in the final
version I push to master.

> in v27-0005. This patch changes code which is not exercised in
> tests[0]. I spent some time understanding the conditions when we
> entered this. There is a comment about non-finished relation
> extension, but I got no success trying to reproduce this. I ended up
> modifying code to lose PageSetAllVisible in proper places and running
> vacuum. Looks like everything works as expected. I will spend some
> more time on this, maybe I will be successful in writing an
> injection-point-based TAP test which hits this...

Based on the coverage report link you provided, that code is changed
by v27 0007, not 0005. 0005 is about moving an assertion out of
lazy_scan_prune(). 0007 changes lazy_scan_new_or_empty() (the code in
question).

Regarding 0007, it looks like what is uncovered (the orange bits in
the coverage report are uncovered, I assume) is empty pages _without_
PD_ALL_VISIBLE set. I don't see anywhere where PageSetAllVisible() is
called except vacuum and COPY FREEZE.

If I was trying to guess how empty pages with PD_ALL_VISIBLE set are
getting vacuumed, I would think it is due to SKIP_PAGES_THRESHOLD
causing us to vacuum an all-frozen empty page.

Then the question is, why wouldn't we have coverage of the empty page
first being set all-visible/all-frozen? It can't be COPY FREEZE
because the page is empty. And it can't be vacuum, because then we
would have coverage. It's very mysterious.

It would be good to have coverage for this case. I don't think you'll
need an injection point for the main case of "empty page not yet set
all-visible is vacuumed for the first time" (unless I'm
misunderstanding something).

I'm not sure how you'll test the "vacuuming an empty, previously
uninitialized page" case described in this comment, though.

             * It's possible that another backend has extended the heap,
             * initialized the page, and then failed to WAL-log the page due
             * to an ERROR.  Since heap extension is not WAL-logged, recovery
             * might try to replay our record setting the page all-visible and
             * find that the page isn't initialized, which will cause a PANIC.
             * To prevent that, check whether the page has been previously
             * WAL-logged, and if not, do that now.

You'd want to force an error during relation extension and then vacuum
the page. I don't know if you need an injection point to force the
error -- depends on what kind of error, I think.

So that I know for attribution, did you review 0003-0005?

- Melanie