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>
In-Reply-To: 
 <CALT9ZEGioDdvkx1rmjVzOW5nyxsxjX16FVifYY4b7MVFuc_80g@mail.gmail.com>
From: Xuneng Zhou <xunengzhou@gmail.com>
Date: Tue, 10 Mar 2026 17:19:39 +0800
Message-ID: 
 <CABPTF7X2HXXiUk1+Vd_pzPhiYOHw0y=OnDXW87G7hfPL0577hw@mail.gmail.com>
Subject: Re: Odd code around ginScanToDelete
To: Pavel Borisov <pashkin.elfe@gmail.com>,
 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/CABPTF7X2HXXiUk1%2BVd_pzPhiYOHw0y%3DOnDXW87G7hfPL0577hw%40mail.gmail.com>
Precedence: bulk

Hi Pavel, Alexander,

On Fri, Mar 6, 2026 at 10:45=E2=80=AFPM Pavel Borisov <pashkin.elfe@gmail.c=
om> wrote:
>
> Hi, Andres and Alexander!
>
> On Sat, 21 Feb 2026 at 13:55, Alexander Korotkov <aekorotkov@gmail.com> w=
rote:
> >
> > On Wed, Feb 4, 2026 at 12:32=E2=80=AFAM Alexander Korotkov <aekorotkov@=
gmail.com> wrote:
> > >
> > > On Tue, Feb 3, 2026 at 6:26=E2=80=AFPM Andres Freund <andres@anarazel=
.de> wrote:
> > > >
> > > > While looking at converting more places to UnlockReleaseBuffer(), i=
n 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, Offset=
Number 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 rea=
ch 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 =
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 ro=
ot 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 ha=
ve 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.

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."


--
Best,
Xuneng