On Tue, Mar 10, 2026 at 11:19 AM Xuneng Zhou <[email protected]> wrote: > On Fri, Mar 6, 2026 at 10:45 PM Pavel Borisov <[email protected]> wrote: > > > > Hi, Andres and Alexander! > > > > On Sat, 21 Feb 2026 at 13:55, Alexander Korotkov <[email protected]> > > wrote: > > > > > > On Wed, Feb 4, 2026 at 12:32 AM Alexander Korotkov <[email protected]> > > > wrote: > > > > > > > > On Tue, Feb 3, 2026 at 6:26 PM Andres Freund <[email protected]> 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 protected]> > > > > > 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. > > > > 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 = 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
