Re: Odd code around ginScanToDelete

On Wed, Feb 4, 2026 at 12:32 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
>
> On Tue, Feb 3, 2026 at 6:26 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > While looking at converting more places to UnlockReleaseBuffer(), in the
> > course of making UnlockReleaseBuffer() faster than the two separate
> > operations, I found this code:
> >
> > static bool
> > ginScanToDelete(GinVacuumState *gvs, BlockNumber blkno, bool isRoot,
> > DataPageDeleteStack *parent, OffsetNumber myoff)
> > ...
> >
> > if (!meDelete)
> > {
> > if (BufferIsValid(me->leftBuffer))
> > UnlockReleaseBuffer(me->leftBuffer);
> > me->leftBuffer = buffer;
> > }
> > else
> > {
> > if (!isRoot)
> > LockBuffer(buffer, GIN_UNLOCK);
> >
> > ReleaseBuffer(buffer);
> > }
> >
> > if (isRoot)
> > ReleaseBuffer(buffer);
> >
> >
> > Which sure looks like it'd release buffer twice if isRoot is set? I guess
> > that's not reachable, because presumably the root page will always go down the
> > !meDelete path. But it sure made me wonder if there's a hard to reach bug.
>
> Yes, it's not possible to have meDelete set for root, because
> me->leftBuffer is always InvalidBuffer for the root. So the branch
> handling meDelete case should better do Assert(!isRoot).
>
> > This code was introduced in
> > commit e14641197a5
> > Author: Alexander Korotkov <akorotkov(at)postgresql(dot)org>
> > Date: 2019-11-19 23:07:36 +0300
> >
> > Fix deadlock between ginDeletePage() and ginStepRight()
> >
> > I didn't trace it further to see if it existed before that in some fashion.
>
> Yes. I think generally this area needs to be reworked to become more
> clear, and have vast more comments. It was wrong from my side trying
> to fix bugs there without reworking it into something more
> appropriate. I'm planning to put work on this during this week.
>
> > There's another oddity here: ginScanToDelete() requires that the root page has
> > been locked by the caller already, but will afaict re-read the root page? But
> > then have code to avoid locking it again, because that would not have worked?
> > Seems odd.
>
>
> It seems a bit odd for me that caller already have locked buffer, but
> passes BlockNumber making us re-read the buffer. But I'm not sure
> that's the same as your point. Could you, please, elaborate more on
> this?

Here is the refactoring patch. Sorry for the delay.

------
Regards,
Alexander Korotkov
Supabase