Fix some inconsistencies with open-coded visibilitymap_set() callers

From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
To: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>
Subject: Fix some inconsistencies with open-coded visibilitymap_set() callers
Date: 2025-06-24 20:01:15
Message-ID: CAAKRu_YyNmUW=fEMdcuW_3Cw54kpjCH4w_C7+7d6U6Hpj_r0=A@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

visibilitymap_set() arguably breaks a few of the coding rules for
modifying and WAL logging buffers set out in
src/backend/access/transam/README.

In 4 of visibilitymap_set()'s 5 non-recovery callers, we set
PD_ALL_VISIBLE and mark the heap buffer dirty outside of the critical
section in which we make the VM changes and emit WAL.

In at least two of its callers, before visibilitymap_set() is called,
MarkBufferDirty() is used when MarkBufferDirtyHint() would be
appropriate (when checksums/wal_log_hints are disabled) -- because we
are not making other changes to the heap page.

And in at least one of its callers (see lazy_scan_prune()), where
PD_ALL_VISIBLE is already set and we don't mark the buffer dirty and
checksums/wal_log_hints are enabled, visibilitymap_set() will still
set the heap page LSN -- even though we didn't set the buffer dirty.

It may be uncommon for the page to be set PD_ALL_VISIBLE and the VM
bit to be clear because of a crash or error after modifying the heap
page but before setting the VM. But it seems easy for us to be only
setting the all-frozen bit in the VM and thus not need to set
PD_ALL_VISIBLE or mark the heap buffer dirty. In that case, we'll
incorrectly set the page LSN without having marked the buffer dirty
(when wal_log_hints/checksums on).

Besides all of these issues, having these operations open-coded all
over the place is error-prone. It's easy to forget to always
PageSetAllVisible() before visibilitymap_set().

I've attached a patch to add a heap-specific wrapper for
visibilitymap_set() that attempts to handle all cases.

One thing I noticed is that log_heap_visible() will do
XLogRegisterBuffer() for the heap page -- even if we made no changes
to the heap page. It seems like we shouldn't do that. The most common
case would be when setting all-visible pages all-frozen when checksums
are not enabled. I don't actually know how we handle replay when we
are not sure which blocks might be registered, though.

Besides feeling like principled cleanup, this patch is a prerequisite
for a larger project I am working on [1] (targeting 19) to combine VM
updates in the same WAL record as the heap page changes and eliminate
xl_heap_visible.

- Melanie

[1] https://www.postgresql.org/message-id/flat/CAAKRu_ZMw6Npd_qm2KM%2BFwQ3cMOMx1Dh3VMhp8-V7SOLxdK9-g%40mail.gmail.com

Attachment Content-Type Size
v1-0001-Introduce-heap-specific-wrapper-for-visibilitymap.patch text/x-patch 16.0 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nathan Bossart 2025-06-24 20:04:03 Re: regdatabase
Previous Message Navrotskiy Artem 2025-06-24 19:32:07 Re: Add Option To Check All Addresses For Matching target_session_attr