Odd code around ginScanToDelete

Hi,

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.

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.

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.

Greetings,

Andres Freund