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

On Mon, Jun 29, 2026 at 10:43 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> 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.

Thanks for the review!

As for performance regressions, I concocted what I felt was the worst
case (repro script pasted at the bottom of the email) which is a table
with all the pages covered by a single VM page and 32 clients each
trying to clear VM bits from different heap pages. It's not a perfect
benchmark because the only way to get the pages to keep being set
all-visible between updates was to run vacuum separately in a loop and
that wouldn't produce the same pattern of vacuums from run-to-run. I
didn't see any performance degradation for this with mostly default
settings. If I turn synchronous_commit off, I see a 10-15% TPS
performance degradation. This seems due to the extra time holding the
VM buffer lock. If you sample wait events, you see a big increase in
BufferContent lock wait events with the patch. As you mentioned, I
don't think there is anything we can do about it, though, and it
doesn't seem to be the bottleneck in the common configuration.

> 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.

visibilitymap_set() registers VM blocks already and those go through
wal consistency checking like all other HEAP2 records. It isn't great,
but it is basically harmless because heap_mask() loop conditions will
cause it to do nothing anyway. We could perhaps do something better
with non-heap pages in wal consistency checking, but we certainly
shouldn't do that on back branches.

> 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.

I agree with you that this is less than ideal on back branches and not
great that we don't have some way of enforcing people continuing to do
it even when done on master (leading to inconsistent usage).

However, I find registering VM buffers untenably confusing without
naming the various block IDs registered. Especially because we don't
replay them in the same order they were registered in. And for
heap_update(), we register up to 4 blocks (after the fix) and the
order is significant because we may be using one VM block for two heap
blocks. I had at some point suggested only macro-izing the update
block refs, but another reviewer preferred aliasing all of them, as it
isn't immediately clear what that block ID is with even two blocks
registered.

> 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.

I had originally wanted to keep the name all_visible_cleared since it
was an existing variable, but you're right that that is now confusing.
I've changed it as you suggested. I think the comment is clarifying
because we don't have a separate unlock_vmbuffer variable here.

> 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've done these two things.

> 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.

I think I've improved this.

> 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 didn't understand the existing comment or even what it was trying to
say, which is why I deleted it. I've restored it now and tried to
clarify my additional point.

> 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."

I adopted this phrasing.

> 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.

Done.

> 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.

I've tried to clarify my commit messages.

Your original review didn't mention 0003/0004 (the test commits). I
was hoping to get a review especially on 0003.

For 0004, I feel relatively confident in it, but I am wondering if the
extra validation I added to verify that the updates happen either on a
single page or across pages is overkill or not. It will make sure the
test continues to cover this case, but, it is quite a few extra lines
of test code.

For 0003, I am much less confident. I've never written test
infrastructure in Perl before. I used AI to help me write it, so I
could really use review from someone experienced with adding TAP test
utility functions. A few thoughts on it/questions:

_run_pg_combinebackup_with_debug_log's error handling is basically
copied from system_or_bail -- is that too duplicative? Also, I'm not
sure if the END code block is done correctly or not.

I think the overall behavior seems okay -- make a temp directory for
the logs from each pg_combinebackup run. Store the paths to those
directories in an array. If pg_combinebackup invocation itself fails,
print the verbose logs right away. Otherwise, at the end of the whole
test run, if any tests failed, print all the verbose logs. If no tests
failed, unlink the test files. It doesn't actually remove the temp
dirs though -- I think those are cleaned up in the general test
cleanup.

One annoying thing is we save the logs at the time of the
pg_combinebackup run, so we don't know if the assertions following it
pass or succeed. That means if any of the assertions fail we will
print all logs. This may be unavoidable, as we wouldn't even be sure
which logs to attribute to which assertions.

Also combinebackup_debug_logs_printed hash set is a little
over-complicated -- it was added because we could print the same log
output twice if pg_combinebackup itself failed. Maybe there is a less
complicated way to handle this? I don't think the END block is
guaranteed to run depending on how pg_combinebackup fails, so I don't
know that we can be sure the logs print without doing it right when
pg_combinebackup fails.

- Melanie

repro (set sync commit off to see the regression):

psql -c "ALTER SYSTEM SET shared_buffers = '2GB';" \
-c "ALTER SYSTEM SET max_wal_size = '150 GB';" \
-c "ALTER SYSTEM SET maintenance_work_mem = '1GB';" \
-c "alter system set synchronous_commit = on"
pg_ctl -l $PGLOG restart

psql -f- <<EOF
DROP TABLE IF EXISTS foo;
CREATE TABLE foo (
cid int not null, -- which client owns the row
k int not null, -- key within that client's partition
filler text not null
) WITH (autovacuum_enabled = off);
ALTER TABLE foo ALTER COLUMN filler SET STORAGE PLAIN;

INSERT INTO foo
SELECT (g-1) % 16, (g-1) / 16, repeat('x', 4200)
FROM generate_series(1, 32000) g;

CREATE UNIQUE INDEX foo_cid_k ON foo (cid, k); -- avoid seq scans
VACUUM (FREEZE, DISABLE_PAGE_SKIPPING) foo;
EOF

while :; do psql -c "VACUUM (FREEZE) foo;"; sleep 0.05; done &

pgbench -n -c 32 -j 32 -t 10000 -D rpc=2000 \
-f- <<EOF
\set k random(0, :rpc - 1)
UPDATE foo SET filler = filler WHERE cid = :client_id AND k = :k;
EOF

kill %1