Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit

On 23/01/2024 05:39, feichanghong wrote:
>
>> On Jan 23, 2024, at 04:47, Heikki Linnakangas <hlinnaka(at)iki(dot)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)