Re: Fix some inconsistencies with open-coded visibilitymap_set() callers - Mailing list pgsql-hackers

On Wed, Jul 2, 2025 at 7:33 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> > 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.

It doesn't make sense to me, either. I have a strong feeling that
visibilitymap_set() isn't well-scoped, but I don't know whether it
needs to do less or more or just different stuff. I don't think it can
be right for code that doesn't modify the page to set the page LSN;
and I suspect it would be best to try to get modifying the page,
marking the page dirty, and emitting WAL for the modification if
required into a tight segment of code, at least in the ideal world
where somebody else does the work and I just get to sit back and nod
wisely.

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

Either advancing the LSN of the heap page is important for some
purpose -- like making crash recovery work properly -- and therefore
the buffer needs to be marked dirty -- or it is not, and the LSN
shouldn't be set in the first place when the page is otherwise
unchanged. I agree with you that it's hard to see how anything goes
wrong as a result of bumping the LSN on a clean page without dirtying
it, but I think it's playing with fire. At the very least, making the
code consistent and intelligible could help future readers of the code
to avoid introducing bugs of their own.

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

I don't think this is the way. visibilitymap_set() shouldn't guess
what the caller has done or will do; the caller should make it clear
explicitly.

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

If it's useful to combine things as a precursor to future work, that
may be fair enough, but otherwise I don't see the point. It's not
"technically fine;" it's just straight-up fine. It's not desirable, in
the sense that we might end up having to do extra page reads later to
correct the situation, but crashes won't intervene between those two
critical sections often enough to matter. Unifying those critical
sections won't change anything about what states code elsewhere in the
tree must be prepared to see on disk.

> 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

Again, I disagree that this "doesn't really matter"; I think it
straight-up does not matter.

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

Without looking at the code, if you're saying we could go from 2 WAL
records down to 1, that would improve performance, which would be
worthwhile, but not a bug fix.

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

Yes, I think that would be worth trying to fix.

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