Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit

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.

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