Message-ID: <72ce1cf6-d718-446e-bdcf-021a24c57c84@iki.fi>
Date: Mon, 29 Jan 2024 13:48:44 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when
 ginFinishSplit
To: feichanghong <feichanghong@qq.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 pgsql-bugs <pgsql-bugs@lists.postgresql.org>
References: <tencent_A3CE810F59132D8E230475A5F0F7A08C8307@qq.com>
 <053bc845-3cb5-445e-8ea4-00f6c678ee25@iki.fi>
 <tencent_0F9D2F03B4F3E2C3DE4483E31B4147C9F605@qq.com>
 <d8f0b068-0e6e-4b2c-8932-62507eb7e1c6@iki.fi>
 <tencent_BEFB72867BA7CF879B492487284296D2F606@qq.com>
 <f8fa079c-c88c-4a4f-9c36-3854d12ddf70@iki.fi>
 <tencent_8FBD28E3B713E1E3C6C4870305C14EA9E305@qq.com>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <tencent_8FBD28E3B713E1E3C6C4870305C14EA9E305@qq.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/72ce1cf6-d718-446e-bdcf-021a24c57c84%40iki.fi>
Precedence: bulk

On 23/01/2024 05:39, feichanghong wrote:
> 
>> On Jan 23, 2024, at 04:47, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>
>> On 22/01/2024 18:21, feichanghong wrote:
>>>> From a performance point of view, this doesn't matter. Incomplete 
>>>> split are extremely rare. For convenience, though, I added a new 
>>>> function specifically for handling these "leftover" incomplete 
>>>> splits as opposed to finishing a split that you just made, which 
>>>> performs the lock-upgrade. See attached. I think that helps with 
>>>> readability, and makes it less likely that we'll forget the 
>>>> lock-upgrade in the future if the insertion code is refactored.
>>> I think that the lock-upgrade in the ginFinishOldSplit function is unsafe
>>> because it violates the requirement of the ginStepRight function that
>>> "The next page is locked first, before releasing the current page.”
>>
>> Good point.
>>
>> I started to work on a more invasive patch that would move the 
>> ginFinishOldSplit() calls to ginTraverseLock() and ginStepRight(), 
>> doing the interlocking correctly. That makes life easier for the 
>> callers; they don't need to deal with incomplete-splits anymore.
>>
>> But then I read the Page deletion section in the README and understood 
>> that my earlier patch is safe, after all. The lock-coupling in 
>> ginStepRight() is only needed for searches, not for inserts. There is 
>> another mechanism that prevents concurrent page deletions during an 
>> insert: VACUUM holds a cleanup-lock on the root page.
>>
>> Does that make sense, or am I missing something? Here's the same patch 
>> as before, with some extra comments to explain why it's safe.
> 
>  From my understanding, you are right. The README includes the following
> explanation:
> ginScanToDelete() traverses the whole tree in depth-first manner. It starts
> from the full cleanup lock on the tree root.  This lock prevents all the
> concurrent insertions into this tree while we're deleting pages. However,
> there are still might be some in-progress readers, who traversed root before
> we locked it.
> 
> For insert, ginFindLeafPage maintains a pin on the relevant pages along the
> path, naturally retaining the pin on the root page as well. Before calling
> ginScanToDelete to delete a page, it secures an exclusive lock on the 
> root page
> and no other backend holds a pin on the root page through 
> LockBufferForCleanup.

I have pushed this fix. Thanks for the report and the review!

-- 
Heikki Linnakangas
Neon (https://neon.tech)