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

Hi,

On 2026-07-16 18:35:30 -0400, Melanie Plageman wrote:
> On Wed, Jul 15, 2026 at 9:31 PM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > - if (!BufferIsValid(vmbuf) || BufferGetBlockNumber(vmbuf) != mapBlock)
> > - elog(ERROR, "wrong buffer passed to visibilitymap_clear");
> > + Assert(BufferIsValid(vmbuf) && BufferGetBlockNumber(vmbuf) == mapBlock);
> >
> > Isn't it be better to keep the elog(ERROR) in addition to the Assert()
> > so that wrong buffer is detected even in non-assert builds?
> >
> > Otherwise, a non-assert build could silently modify the wrong VM page
> > if a caller passes the wrong buffer. Since this commit increased the number
> > of callers and introduced visibilitymap_clear() into WAL redo paths,
> > it seems safer to retain the runtime check.
>
> I had a vague memory of discussing this with someone at some point
> (and that person agreeing we should turn the error into an assert) but
> now I can't find any record of it.

That might have been me.

I don't see any reason for this to be an elog(). So much would have to go
wrong for that to happen in a non-assert build. And we check many more crucial
things only in Asserts.

I'm also don't care too much about it being an elog, so I'm not going to push
for reverting the s/Assert/elog/. But I don't think it should be seen any sort
of policy about this kind of check needing to be an elog going forward.

Greetings,

Andres Freund