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. ------ Regards, Alexander Korotkov Supabase
v1-0001-Rework-ginScanToDelete-to-pass-Buffers-instead-of.patch
Description: Binary data
