Subject: Re: Odd code around ginScanToDelete

From: Alexander Korotkov <[email-masked]>
Date: Sat, 21 Feb 2026 11:54:43 +0200

On Wed, Feb 4, 2026 at 12:32 AM Alexander Korotkov <[email-masked]> wrote:
>
> On Tue, Feb 3, 2026 at 6:26 PM Andres Freund <[email-masked]> 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, 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.
>
> 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 <[email-masked]>
> >   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 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.
>
>
> 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.

------
Regards,
Alexander Korotkov
Supabase

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 6 Mar 2026 18:45:06 +0400

Hi, Andres and Alexander!

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

> > 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 = 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

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 10 Mar 2026 17:19:39 +0800

Hi Pavel, Alexander,

> Overall, it looks good to me.

The refactor LGTM in general. The buffer-ownership rewrite looks
cleaner and safer overall.

> Previously to this patch, palloc-ed "me" variable also was't freed at

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.

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

---

From: Pavel Borisov <[email-masked]>
Date: Tue, 10 Mar 2026 13:28:53 +0400

Hi, Xuneng

> returns in ginVacuumPostingTree(), which matches the natural lifetime
I proposed not freeing child when child iteration is complete. They
indeed can be reused. I proposed cleaning children when "my" iteration
is complete. At that time all the children iterations are completed
and not needed when we return level up.

Regards,
Pavel Borisov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 12 Mar 2026 00:22:02 +0200

> and not needed when we return level up.

This is not clear for me.  We need stack items to keep track of left
pages until we scan the whole posting tree.  After scanning the whole
posting tree we can free stack items as we do now.

------
Regards,
Alexander Korotkov
Supabase

---

From: Pavel Borisov <[email-masked]>
Date: Thu, 12 Mar 2026 02:35:23 +0400

> posting tree we can free stack items as we do now.

Hi, Alexander!
You are right, that we can free all posting tree stack items after the
whole tree, as we do now. But I think we can also do it earlier. It
looks like all "children" items are needed and could be reused only
until iteration on "my" level ends. When function returns up the
recursion "my" level becomes "child" for a caller, and previous
"child" is not used anymore.

It's optional, maybe we can do freeing at the end of posting tree iteration.

Regards,
Pavel Borisov

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 12 Mar 2026 00:37:31 +0200

> "child" is not used anymore.

No matter how many levels we can go up, we can still descend and need
the leftBuffer stored at any stack level.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 12 Mar 2026 00:41:10 +0200

Hi, Pavel!

Thank you for your review!

> myStackItem is not needed anymore.

Already answered in this thread.

> +.             Assert(!deleted); /* Root page is never deleted */

Done.

> ginVacuumPostingTreeLeaves). It could be good to extend this to all

Renamed as you proposed.

> sibling page to its left"

I renamed that to just "left sibling".  Deleted pages are not in the
tree already.  And "the rightmost page to its left" is just left
sibling.

> buffers of the page" (as two buffers are mentioned)

I prefer to just use word "buffer" twice to make it more explicit.

> "must already be pinned" -> "must already have been pinned"

Done.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 12 Mar 2026 00:51:47 +0200

> window is actually "from when the caller sets it until the

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

---

From: Pavel Borisov <[email-masked]>
Date: Thu, 12 Mar 2026 12:41:44 +0400

> ginScanPostingTreeToDelete().

Hi, Alexander!
Patch v2 looks good to me. I also agree with Xuneng that this
refactoring improved the logic to make it look clearer.
Thank you for the explanation of buffers lifetime!

Regards,
Pavel Borisov

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 12 Mar 2026 11:05:51 +0200

Hi, Pavel!

> Thank you for the explanation of buffers lifetime!

Thank you for your feedback. I'll push the patch if no objections.

------
Regards,
Alexander Korotkov
Supabase

---

From: jinbinge <[email-masked]>
Date: Thu, 12 Mar 2026 18:12:06 +0800 (CST)

Hi,

Overall solid to me. I got a nitpick:
in the code comments, ginDeletePage should be ginDeletePostingPag(ginDeletePage -> ginDeletePostingPage).
There are 3 places that need to be revised.
------
Regards,
jinbinge

---

From: Xuneng Zhou <[email-masked]>
Date: Mon, 16 Mar 2026 16:31:45 +0800

> the leftBuffer stored at any stack level.

Yeah, the important point is that a stack item here represents
per-depth scan state, not just one recursive invocation. Returning
from one subtree does not necessarily mean that depth is finished
globally: the caller may move to a sibling and descend again, and that
later descent can still need the child level's saved leftBuffer from
the subtree we just finished. The stronger condition is that no more
pages remain to be scanned to the right at that depth; the code
already uses GinPageRightMost(page) for that when releasing the child
level's leftBuffer.

-- 
Best,
Xuneng