On Sun, Jul 19, 2026 at 10:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Melanie Plageman <melanieplageman@gmail.com> writes:
> > I've committed this.
>
> Coverity complained about this patch:
>
> /srv/coverity/git/pgsql-git/postgresql/src/backend/access/heap/pruneheap.c: 951 in
heap_page_fix_vm_corruption()
> 945 MarkBufferDirtyHint(prstate->buffer, true);
> 946 }
> 947
> 948 if (do_clear_vm)
> 949 {
> 950 LockBuffer(prstate->vmbuffer, BUFFER_LOCK_EXCLUSIVE);
> >>> CID 1697133: Error handling issues (CHECKED_RETURN)
> >>> Calling "visibilitymap_clear" without checking return value (as is done elsewhere 13 out of 14 times).
> 951 visibilitymap_clear(prstate->relation->rd_locator, prstate->block,
> 952 prstate->vmbuffer,
> 953 VISIBILITYMAP_VALID_BITS);
> 954 LockBuffer(prstate->vmbuffer, BUFFER_LOCK_UNLOCK);
> 955 prstate->old_vmbits = 0;
> 956 }
>
> I think it's right to complain --- if we check for failure everywhere
> else, why's it OK to not check here? If it is OK, a comment and an
> explicit cast to "(void)" would be appropriate.
Got it. I didn't know we always checked return values everywhere in
the code base.
In other cases where we call visibilitymap_clear(), we use the return
value to decide whether or not to emit WAL (because if the page wasn't
modified, we shouldn't). Not emitting WAL here is its own questionable
issue (historical, not something I introduced), but since we don't, I
didn't need the return value. I'll push a comment and cast next week
(I'm traveling this week and don't commit on the road if I can help
it).
- Melanie
