Odd code around ginScanToDelete

[Andres Freund]

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 <akorot...@postgresql.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