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

On Wed, Jul 15, 2026 at 9:31 PM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> Thanks for working on them!

Thanks!

> In commit ed62d26caca
>
> - 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. My thought was that only developers
should be introducing new callers of visibilitymap_clear() and
developers should be using assert builds. My understanding is
typically we stick with asserts for situations we don't think users
can get themselves and would only be caused by wayward developers.
However, visibilitymap_set() still has the error, so it's probably
better to be consistent. And there's no reason to change this in
backbranches at least. I'll change it back. It may take me a few days,
but I won't forget!

- Melanie