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