MIME-Version: 1.0
References: <utrlxij43fbguzw4kldte2spc4btoldizutcqyrfakqnbrp3ir@ph3sphpj4asz>
 <CAPpHfdt5BHkpvFnwg_RiMrCdD8W5FhOoahqY2Td-y3kb45UpZw@mail.gmail.com>
 <CAPpHfduJQPTrXpJr=-mHtsJH8+1v_X389K6trajPzhnA37C9nA@mail.gmail.com>
 <CALT9ZEGioDdvkx1rmjVzOW5nyxsxjX16FVifYY4b7MVFuc_80g@mail.gmail.com>
 <CABPTF7X2HXXiUk1+Vd_pzPhiYOHw0y=OnDXW87G7hfPL0577hw@mail.gmail.com>
In-Reply-To: 
 <CABPTF7X2HXXiUk1+Vd_pzPhiYOHw0y=OnDXW87G7hfPL0577hw@mail.gmail.com>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Thu, 12 Mar 2026 00:51:47 +0200
Message-ID: 
 <CAPpHfdszQKx+OmxPtU+th3SLiJbf7KbhBbHnTZ1yE6ohQ3-wDA@mail.gmail.com>
Subject: Re: Odd code around ginScanToDelete
To: Xuneng Zhou <xunengzhou@gmail.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>,
 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/CAPpHfdszQKx%2BOmxPtU%2Bth3SLiJbf7KbhBbHnTZ1yE6ohQ3-wDA%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 10, 2026 at 11:19=E2=80=AFAM Xuneng Zhou <xunengzhou@gmail.com>=
 wrote:
> On Fri, Mar 6, 2026 at 10:45=E2=80=AFPM Pavel Borisov <pashkin.elfe@gmail=
.com> wrote:
> >
> > Hi, Andres and Alexander!
> >
> > On Sat, 21 Feb 2026 at 13:55, Alexander Korotkov <aekorotkov@gmail.com>=
 wrote:
> > >
> > > On Wed, Feb 4, 2026 at 12:32=E2=80=AFAM Alexander Korotkov <aekorotko=
v@gmail.com> wrote:
> > > >
> > > > On Tue, Feb 3, 2026 at 6:26=E2=80=AFPM Andres Freund <andres@anaraz=
el.de> wrote:
> > > > >
> > > > > While looking at converting more places to UnlockReleaseBuffer(),=
 in the
> > > > > course of making UnlockReleaseBuffer() faster than the two separa=
te
> > > > > operations, I found this code:
> > > > >
> > > > > static bool
> > > > > ginScanToDelete(GinVacuumState *gvs, BlockNumber blkno, bool isRo=
ot,
> > > > >                                 DataPageDeleteStack *parent, Offs=
etNumber 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 alway=
s go down the
> > > > > !meDelete path. But it sure made me wonder if there's a hard to r=
each 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 som=
e fashion.
> > > >
> > > > Yes.  I think generally this area needs to be reworked to become mo=
re
> > > > clear, and have vast more comments.  It was wrong from my side tryi=
ng
> > > > 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 ro=
ot 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, b=
ut
> > > > 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 o=
n
> > > > 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.
>
> The refactor LGTM in general. The buffer-ownership rewrite looks
> cleaner and safer overall.
>
> > 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.
>
> I think freeing myStackItem->child inside recursive calls might not be
> worthwhile here. That node is intentionally reused for subsequent
> siblings at the same depth, and it carries state (leftBuffer) that can
> still be needed until the level is fully processed.
> Freeing/reallocating it per subtree would add churn and make the
> lifetime rules harder to reason about without meaningful memory
> savings (the number of nodes is bounded by tree depth, not number of
> pages). We currently free the chain once after ginScanToDelete()
> returns in ginVacuumPostingTree(), which matches the natural lifetime
> boundary
>
> > Could limiting the maximum recursion level be useful?
>
> Posting-tree depth is naturally small; a hard cap seems to add failure
> risk with little practical benefit.
>
> > 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 */
>
> + 1,  this is a good invariant check and improves readability
>
> One minor nit for the comment:
>
> The DataPageDeleteStack.buffer field comment says "valid only while
> recursing into children"
> this is true for internal pages, but for leaf pages the buffer is
> valid until the pageWasDeleted / leftBuffer decision. The validity
> window is actually "from when the caller sets it until the
> post-recursion cleanup."

Thank you for catching this.  I decided to remove this statement from
v2.  It's hard to explain the life cycle of the buffer clearly in one
sentence.  On the other hand, it's explained in the comments of
ginScanPostingTreeToDelete().

------
Regards,
Alexander Korotkov
Supabase