> On Sep 11, 2026, at 02:31, Melanie Plageman <[email protected]> wrote:
>
> On Tue, Apr 21, 2026 at 5:37 PM Melanie Plageman
> <[email protected]> wrote:
>>
>> On Mon, Apr 20, 2026 at 12:18 PM Melanie Plageman
>> <[email protected]> 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 of
visibilitymap_pin explicitly says that "Because that can require I/O to read
the map page, you shouldn't hold a lock on the 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/
