Re: Fix some inconsistencies with open-coded visibilitymap_set() callers

On Mon, Jun 30, 2025 at 3:01 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> I'm pretty concerned about this change:
>
> /*
> * If the page is all visible, need to clear that, unless we're only
> * going to add further frozen rows to it.
> *
> * If we're only adding already frozen rows to a previously empty
> * page, mark it as all-visible.
> */
> if (PageIsAllVisible(page) && !(options & HEAP_INSERT_FROZEN))
> {
> all_visible_cleared = true;
> PageClearAllVisible(page);
> visibilitymap_clear(relation,
> BufferGetBlockNumber(buffer),
> vmbuffer, VISIBILITYMAP_VALID_BITS);
> }
> - else if (all_frozen_set)
> - PageSetAllVisible(page);
>
> One problem is that this falsifies the comment -- the second sentence
> in the comment refers to code that the patch deletes.

Yep, I should have updated that comment.

> But also, the
> all_frozen_set flag is used a bit further down when setting
> xlrec->flags, and now we'll set XLH_INSERT_ALL_FROZEN_SET when in fact
> we didn't set the all-frozen bit. It seems like the intention here was
> that the existing XLOG_HEAP2_MULTI_INSERT record should cover setting
> the all-frozen bit, if we do that, and it seems like you're changing
> that somehow, but it's not clear to me why we'd want to change that,
> and even if we do, and it doesn't appear to me that you've chased down
> all the loose ends i.e. if that's the plan, then I'd expect the redo
> routine to be modified and XLH_INSERT_ALL_FROZEN_SET to be deleted.

Right, yes, I accidentally forgot to remove setting PD_ALL_VISIBLE
from heap_xlog_multi_insert(). That was not great.

As for why I changed the responsibility, I was trying to make COPY
FREEZE more consistent with all of the other places we set the VM and
mark PD_ALL_VISIBLE. In other places in master, we make the changes to
the heap page that render the page all-visible and then separately set
PD_ALL_VISIBLE and emit an xl_heap_visible record.

Ironically, my larger patch set [1] seeks to eliminate xl_heap_visible
and add the changes to the VM to the WAL records that make the changes
rendering the page all-visible.

I think you are right that the change I made in the patch in this
thread is not an improvement to COPY FREEZE.

> I think this might actually be an underlying design problem with the
> patch -- heap_page_set_vm_and_log() seems to want to be in charge of
> both what we do with the heap page and what we do with the
> corresponding VM bit, but that runs contrary to the caller's need to
> bundle the VM bit changes with some WAL record that is doing other
> things to the heap page.

The way it is currently, visibilitymap_set() sets the heap page LSN
but it doesn't actually set PD_ALL_VISIBLE or mark the heap buffer
dirty. There is no way to tell visibilitymap_set() whether or not you
modified the heap page -- it just assumes you did. That separation of
duties didn't make sense to me.

In fact, even if you make visibilitymap_set() aware that you haven't
modified the heap page and then don't set the heap page LSN, we'll
still register the heap buffer when emitting the xl_heap_visible
record and then end up modifying it when replaying that record.

> heap_page_set_vm_and_log() says that it shouldn't be called during
> recovery but doesn't assert that it isn't.

Ah, good point.

So, I read and started addressing your other inline code review, but
after thinking more about your comment about
heap_page_set_vm_and_log() having a design problem, I went back to the
drawing board and tried to figure out 1) which of the issues I point
out are actually bugs and 2) how I can do my larger patch set without
these wrappers. I solved 2, but that's a story for another thread.

As for 1, so, I went through all of these cases and tried to figure
out what I thought was actually a bug and should be fixed. The only
one I think is a bug is the case in lazy_scan_prune() where the page
was already set all-visible but not already set all-frozen. The code
is as follows:

else if (all_visible_according_to_vm && presult.all_visible &&
presult.all_frozen && !VM_ALL_FROZEN(vacrel->rel, blkno,
&vmbuffer))
{
...
if (!PageIsAllVisible(page))
{
PageSetAllVisible(page);
MarkBufferDirty(buf);
}

...
old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
InvalidXLogRecPtr,
vmbuffer, InvalidTransactionId,
VISIBILITYMAP_ALL_VISIBLE |
VISIBILITYMAP_ALL_FROZEN);

In that case, when checksums or wal_log_hints are on, if
PD_ALL_VISIBLE is already set and we didn't otherwise dirty the page
during heap_page_prune_and_freeze(), visibilitymap_set() will stamp
the clean heap page with the LSN of the xl_heap_visible record.

I spent quite a bit of time trying to think of what could happen if we
advance the LSN of an otherwise clean page and then don't mark it
dirty to reflect having made that change. And, honestly, I couldn't
come up with something.

If someone evicts the page or we crash, we'll lose the LSN, but that
seems like it can't hurt anything if nothing else on the page has
changed.

But, I may be insufficiently creative. I know we are definitely not
supposed to advance the LSN of clean pages probably at all and even
more so without marking them dirty.

One thing we could do is check if the heap buffer is dirty before
setting the LSN in visibilitymap_set():

diff --git a/src/backend/access/heap/visibilitymap.c
b/src/backend/access/heap/visibilitymap.c
index 745a04ef26e..a4e2e454e1f 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -301,7 +301,7 @@ visibilitymap_set(Relation rel, BlockNumber
heapBlk, Buffer heapBuf,
* WAL record inserted above, so it would be incorrect to
* update the heap page's LSN.
*/
- if (XLogHintBitIsNeeded())
+ if (XLogHintBitIsNeeded() && BufferIsDirty(heapBuf))
{
Page heapPage = BufferGetPage(heapBuf);

Something about this doesn't feel altogether right, though, but I'm
not sure why.

As for the not bug cases:

For all the cases where we modify the heap and VM page not in the same
critical section, we could error out after making the heap page
changes before setting the VM. But because this would just lead to
PD_ALL_VISIBLE being set and the VM bit not being set, which is
technically fine.

It seems like it would be better to make all of the changes in the
same critical section, and, in some of the cases, it wouldn't be hard
to do so. But, since I cannot claim a correctness issue, we can leave
it the way it is.

In lazy_scan_new_or_empty(), it would be better to PD_ALL_VISIBLE
before marking the buffer dirty, but it doesn't really matter because
no one else could write the buffer out since we have an exclusive lock

And in heap_multi_insert() for the COPY FREEZE case, in recovery, we
set PD_ALL_VISIBLE and mark the buffer dirty when replaying both the
xl_heap_multi_insert record and when replaying the xl_heap_visible
record. This involves taking and releasing the content lock on the
heap buffer page twice to make redundant changes. Again, not a
correctness issue, but I think it makes much more sense to include the
VM changes in the xl_heap_multi_insert record.

So, in summary, do you think we should do something about the
lazy_scan_prune() -> visibilitymap_set() case where we advance the LSN
of a clean buffer and don't mark it dirty?

- Melanie

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