Re: hio.c does visibilitymap_pin()/IO while holding buffer lock

From: Andres Freund <andres(at)anarazel(dot)de>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, pgsql-hackers(at)postgresql(dot)org, Tomas Vondra <tv(at)fuzzy(dot)cz>, Anastasia Lubennikova <lubennikovaav(at)gmail(dot)com>, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>
Subject: Re: hio.c does visibilitymap_pin()/IO while holding buffer lock
Date: 2023-03-29 01:21:02
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On 2023-03-25 11:17:07 -0700, Andres Freund wrote:
> I don't see how that's easily possible with the current lock ordering
> rules. At least without giving up using RBM_ZERO_AND_LOCK for extending or
> stuffing even more things to happen with the the extension lock held, which I
> don't think we want to. I don't think INSERT_FROZEN is worth that price.

I think I might have been thinking of this too narrowly. It's extremely
unlikely that another backend would discover the page. And we can use
visibilitymap_pin_ok() to amortize the cost to almost nothing - there's a lot
of bits in an 8k block...

Here's a draft patch.

The bulk relation patch I am polishing has a similar issue, except that there
the problem is inserting into the FSM, instead of pinning a VM pageabout the
FSM. Hence the patch above makes the infrastructure a bit more general than
required for the HEAP_INSERT_FROZEN case alone (where we currently shouldn't
ever have a valid otherBuffer).

The way the parameter ordering for GetVisibilityMapPins() works make it
somewhat unwieldy - see e.g the existing
if (otherBuffer == InvalidBuffer || targetBlock <= otherBlock)
GetVisibilityMapPins(relation, buffer, otherBuffer,
targetBlock, otherBlock, vmbuffer,
GetVisibilityMapPins(relation, otherBuffer, buffer,
otherBlock, targetBlock, vmbuffer_other,

Which I now duplicated in yet another place.

Perhaps we just ought to switch buffer1/block1 with buffer2/block2 inside
GetVisibilityMapPins(), to avoid duplicating that code elsewhere?

Because we now track whether the *targetBuffer* was ever unlocked, we can be a
bit more narrow about the possibility of there not being sufficient space.

The patch could be narrowed for backpatching. But as there's likely no
practical problem at this point, I wouldn't want to backpatch anyway?


Andres Freund

Attachment Content-Type Size
v1-0001-hio-Release-extension-lock-before-initializing-pa.patch text/x-diff 2.0 KB
v1-0002-WIP-relation-extension-Don-t-pin-the-VM-while-hol.patch text/x-diff 7.8 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2023-03-29 01:40:26 Re: allow_in_place_tablespaces vs. pg_basebackup
Previous Message Hayato Kuroda (Fujitsu) 2023-03-29 01:01:51 RE: PGdoc: add missing ID attribute to create_subscription.sgml