MIME-Version: 1.0
References: <utrlxij43fbguzw4kldte2spc4btoldizutcqyrfakqnbrp3ir@ph3sphpj4asz>
 <CAPpHfdt5BHkpvFnwg_RiMrCdD8W5FhOoahqY2Td-y3kb45UpZw@mail.gmail.com>
 <CAPpHfduJQPTrXpJr=-mHtsJH8+1v_X389K6trajPzhnA37C9nA@mail.gmail.com>
In-Reply-To: 
 <CAPpHfduJQPTrXpJr=-mHtsJH8+1v_X389K6trajPzhnA37C9nA@mail.gmail.com>
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Fri, 6 Mar 2026 18:45:06 +0400
Message-ID: 
 <CALT9ZEGioDdvkx1rmjVzOW5nyxsxjX16FVifYY4b7MVFuc_80g@mail.gmail.com>
Subject: Re: Odd code around ginScanToDelete
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CALT9ZEGioDdvkx1rmjVzOW5nyxsxjX16FVifYY4b7MVFuc_80g%40mail.gmail.com>
Precedence: bulk

Hi, Andres and Alexander!

On Sat, 21 Feb 2026 at 13:55, Alexander Korotkov <aekorotkov@gmail.com> wro=
te:
>
> On Wed, Feb 4, 2026 at 12:32=E2=80=AFAM Alexander Korotkov <aekorotkov@gm=
ail.com> wrote:
> >
> > On Tue, Feb 3, 2026 at 6:26=E2=80=AFPM Andres Freund <andres@anarazel.d=
e> 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, OffsetNu=
mber myoff)
> > > ...
> > >
> > >         if (!meDelete)
> > >         {
> > >                 if (BufferIsValid(me->leftBuffer))
> > >                         UnlockReleaseBuffer(me->leftBuffer);
> > >                 me->leftBuffer =3D 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@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 fa=
shion.
> >
> > 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 p=
age? 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.

Hi, Andres and Alexander!

I've looked into the patch v1.
Overall, it looks good to me.

Some thoughts:

Is it worth/possible in recursive calls of ginScanToDelete() to free
allocated myStackItem->child after processing all children of the
current level, when they are not needed anymore?
Previously to this patch, palloc-ed "me" variable also was't freed at
recursion levels.

Could limiting the maximum recursion level be useful?

In the comment to myStackItem before ginScanToDelete(), it might be
worth adding that after processing all pages on the current level,
myStackItem is not needed anymore.

> > 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).
Looks like this additional Assert is not in patch v1.

In the root call of ginScanToDelete(gvs, &root); we can add Assert
checking that its return result is false:
-               ginScanToDelete(gvs, &root);
+              deleted =3D ginScanToDelete(gvs, &root);
+.             Assert(!deleted); /* Root page is never deleted */

Additionally, it could be good to rename all vacuum functions related
to posting tree pages only, to include "Posting" (e.g., ginDeletePage
-> ginDeletePostingPage). The same is for the functions only for the
entry tree. It is already named this way in many places (e.g.
ginVacuumPostingTreeLeaves). It could be good to extend this to all
relevant functions.

Several small proposals on wording:
"rightmost non-deleted page to its left" -> "closest non-deleted
sibling page to its left"
"each entry tracks the buffer of the page" -> "each entry tracks the
buffers of the page" (as two buffers are mentioned)
"must already be pinned" -> "must already have been pinned"

Best regards,
Pavel Borisov
Supabase