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

[Andres Freund]

Hi,

Starting with

commit 7db0cd2145f2bce84cac92402e205e4d2b045bf2
Author: Tomas Vondra <tomas.von...@postgresql.org>
Date:   2021-01-17 22:11:39 +0100

    Set PD_ALL_VISIBLE and visibility map bits in COPY FREEZE

RelationGetBufferForTuple does

        /*
         * The page is empty, pin vmbuffer to set all_frozen bit.
         */
        if (options & HEAP_INSERT_FROZEN)
        {
                Assert(PageGetMaxOffsetNumber(BufferGetPage(buffer)) == 0);
                visibilitymap_pin(relation, BufferGetBlockNumber(buffer), 
vmbuffer);
        }

while holding a buffer lock. visibilitymap_pin() reads pages, if vmbuffer
doesn't already point to the right block.


The lock ordering rules are to lock VM pages *before* locking heap pages.


I think the reason this hasn't yet bitten us badly, is that INSERT_FROZEN
effectively requires that the relation is access exclusive locked.  There
shouldn't be other backends locking multiple buffers in the relation (bgwriter
/ checkpointer can lock a single buffer at a time, but that's it).


I see roughly two ways forward:

1) We add a comment explaining why it's safe to violate lock ordering rules in
   this one situation

2) Change relevant code so that we only return a valid vmbuffer if we could do
   so without blocking / IO and, obviously, skip updating the VM if we
   couldn't get the buffer.

Greetings,

Andres Freund