From: Heikki Linnakangas <hlinnaka@iki.fi>
To: feichanghong <feichanghong@qq.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Subject: Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
Date: Mon, 29 Jan 2024 13:48:44 +0200
Message-ID: <72ce1cf6-d718-446e-bdcf-021a24c57c84@iki.fi> (raw)
In-Reply-To: <tencent_8FBD28E3B713E1E3C6C4870305C14EA9E305@qq.com>
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>
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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: hlinnaka@iki.fi, feichanghong@qq.com, michael@paquier.xyz, pgsql-bugs@lists.postgresql.org
Subject: Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
In-Reply-To: <72ce1cf6-d718-446e-bdcf-021a24c57c84@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox