On Wed, Jul 15, 2026 at 9:31 PM Fujii Masao <[email protected]> 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
