Feedback-ID: id4a34324:Fastmail
Date: Fri, 24 Mar 2023 19:57:40 -0700
From: Andres Freund <andres@anarazel.de>
To: pgsql-hackers@postgresql.org, Tomas Vondra <tv@fuzzy.cz>,
	Anastasia Lubennikova <lubennikovaav@gmail.com>,
	Pavan Deolasee <pavan.deolasee@gmail.com>,
	Jeff Janes <jeff.janes@gmail.com>
Subject: hio.c does visibilitymap_pin()/IO while holding buffer lock
Message-ID: <20230325025740.wzvchp2kromw4zqz@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/20230325025740.wzvchp2kromw4zqz%40awork3.anarazel.de>
Precedence: bulk

Hi,

Starting with

commit 7db0cd2145f2bce84cac92402e205e4d2b045bf2
Author: Tomas Vondra <tomas.vondra@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