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

On Thu, Jul 10, 2025 at 9:57 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> A direct translation of this would be to add a boolean parameter to
> visibilitymap_set() like "heap_page_modified" or "set_heap_lsn". Is
> that along the lines you were thinking?

Yeah, something like that. I haven't thought through the details.

> > 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.
>
> I think one argument for having it in the same critical section is the
> defensive coding perspective. Our rule is that you make changes to the
> heap page, mark it dirty, emit WAL, and then stamp it with that LSN
> all in the same critical section.

I agree that we need to follow this rule.

> In the case of PD_ALL_VISIBLE, it is okay to break this rule because
> it is okay to lose the PD_ALL_VISIBLE hint as long as the VM bit
> doesn't get set. But, future programmers could see this and make other
> modifications to the heap page in the same area we set PD_ALL_VISIBLE
> (the one outside of a critical section).

But this is saying that PD_ALL_VISIBLE isn't really covered by the WAL
record in question -- instead, it's a related hint. Or really
semi-hint, since it's only conditionally OK to lose it. So the rule
above doesn't necessarily fully apply to this situation.

> Anyway, I'm now convinced that the way forward for this particular
> issue is to represent the VM changes in the same WAL record that has
> the heap page changes, so I won't further belabor the issue.

Makes sense.

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