Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access) - Mailing list pgsql-hackers

> On Sep 11, 2026, at 02:31, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Tue, Apr 21, 2026 at 5:37 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>>
>> On Mon, Apr 20, 2026 at 12:18 PM Melanie Plageman
>> <melanieplageman@gmail.com> wrote:
>>>
>>>
>>> Yes, I think changing it to a temp table is the easiest fix. We could
>>> also do autovacuum_enabled=false, I think, but making it a temp table
>>> seems cleanest.
>>>
>>> I wonder if we should move the EXPLAIN test above the results queries,
>>> then throw in a vacuum in between some of them so we exercise btree
>>> gist as a bitmap heap scan and as an index only scan. It could provide
>>> a little bit more coverage? Or maybe that isn't actually extra
>>> coverage. I'm not sure.
>>
>> I kept it simple and just committed making it a temp table in 62407d26b7c
>
> An adversarial LLM review of this patch series found that I call
> visibilitymap_pin() after taking a cleanup lock on the heap page in
> the on-access pruning path -- which is not good. Here is a small patch
> to fix that. Doing it before we're sure we can get the cleanup lock
> could occasionally lead to an unneeded pin, but such situations should
> be uncommon.
>
> - Melanie
> <v1-0001-Make-on-access-pruning-pin-visibility-map-before-.patch>

Looks reasonable to me to move visibilitymap_pin to before ConditionalLockBufferForCleanup. I saw the header comment of
ofvisibilitymap_pin explicitly says that "Because that can require I/O to read the map page, you shouldn't hold a lock
onthe heap page while doing that.”. 

I was thinking if we should unpin when ConditionalLockBufferForCleanup fails, but the new comment seems to resolve my
confusion,because next heap page may map the same VM page. 

So v1 LGTM.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/