Re: Why clearing the VM doesn't require registering vm buffer in wal record

On Sat, Jun 27, 2026 at 6:41 AM Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
> v3 set applies cleanly on REL_18_STABLE.

Oh, I didn't realize this was based on the back-branch code. Thanks.

I have reviewed 0001 and 0002 and in general I think this looks good.
There is possibly room for some concern about performance regressions
with 0002 since, as you point out in some of the added comments, we
will now keep the VM buffer locked for longer. It might be worth
benchmarking that to see how bad it looks, but I don't know that we
really have any choice. Correctness bugs are bad.

One problem is that I think the newly-registered VM blocks will get
fed to heap_mask() if wal_consistency_checking is turned on, but it's
really only meant to work with heap pages.

Other comments follow.

0001. I personally don't find this to be an improvement. Even it is, I
feel like a back-patched commit is not a good place to invent a new
convention for identifying block reference indexes. However, it's
ultimately a matter of style, so if I lose this particular argument,
c'est la vie. I just feel like if we introduce this for every WAL
record, it's going to be a lot of new #defines that are going to be
long and clunky, and if we don't, then we have a weird mix of styles.

0002. In heap_insert, the local variable called 'all_visible_cleared'
seems like it needs a bit of work. You'd expect such a variable to be
set at the point where we clear the all-visible bit, but it's actually
set before that, when we lock the buffer. Later, you write in the
comments "We locked vmbuffer if all_visible_cleared was true
regardless of whether or not we ended up modifying it," which
similarly implies that the variable is now misnamed. However,
PageClearAllVisible() is called if and only if all_visible_cleared is
true, so that comment doesn't seem to be entirely accurate. My first
instinct is to rename the variable to clear_all_visible and delete the
comment, but there might be something better.

Looks like heap_delete has the same issue, and heap_update has a
similar issue multiplied by two (all_visible_cleared vs.
all_visible_cleared_new).

In heap_update, the comment /* clear PD_ALL_VISIBLE flags, reset all
visibilitymap bits */ has been lost, and the code has been reordered
slightly in that area. This is fine, but keeping the existing comment
and/or code ordering could be worth considering.

In heap_lock_tuple, I think it might be a good idea to set
unlock_vmbuffer = false when we release the lock, and then just after
the out_unlocked: label, Assert(!unlock_vmbuffer). That way, if
someone adds an improvident goto in a future patch, it has a chance of
tripping an Assert.

I think the comment for heap_xlog_vm_clear could be better: instead of
saying that it "handles" something, say what it actually does.
Likewise "use it for redo" inside the function is a little imprecise.
If I'm understanding the purpose of this machinery correctly, the
point here is that any FPIs included for the VM block references need
to be applied, and otherwise (if there's no FPI or the block reference
is not included at all) we need to just clear the relevant bit. That
seems correct, but a little more work on the comments would make it
clearer to future readers, I feel.

In heap_xlog_multi_insert, you delete the comment "The visibility map
may need to be fixed even if the heap page is already up-to-date," and
replace it with a completely new comment that seems to be addresing
someone different concerns, but the identical comments in other
functions in the file are retained. Consider adding to the comment
instead of replacing it. Also, I am not sure I understand the point of
the new comment. Perhaps try to make it clearer.

I find the phrasing of the comment addition at the top of
visibilitymap_clear to be a little harder to understand than
necessary. Maybe: "Most callers should use visibilitymap_clear_locked
rather than this function. It is usually necessary to register the VM
buffer in the WAL record, and this necessitates holding the lock for
longer than it is held here. However, we retain this function for
backward compatibility, and for a few callers that don't have this
requirement."

The redo routines ignore the return value of
visibilitymap_clear_locked, which means they set the page LSN even if
it isn't modified. Apparently there is other precedent for that
treatment, but maybe it's best avoided anyway.

I think that the commit messages could use some polishing. For 0001,
if we keep it, I'd suggest that the subject line should use language
like "introduce macros for" instead of "alias" because nobody's going
to know what "alias" means without reading the commit itself. For
0002, I'd suggest that the subject line should be something like "Fix
heap WAL records to register VM buffers when clearing VM bits". I
think your goals here should be (1) to maximize the chance that
somebody can understand what the commit does if they just read the
one-line summary, or at most the whole commit message, and (2) to
minimize the amount of work it will take to turn your commit message
into a release note entry. I'm not saying your existing commit
messages are bad, but they read a bit as if they were written to
summarize work that the reader has already seen.

--
Robert Haas
EDB: http://www.enterprisedb.com