public inbox for [email protected]
help / color / mirror / Atom feedOdd code around ginScanToDelete
2+ messages / 2 participants
[nested] [flat]
* Odd code around ginScanToDelete
@ 2026-02-03 16:26 Andres Freund <[email protected]>
2026-02-03 22:32 ` Re: Odd code around ginScanToDelete Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 2+ messages in thread
From: Andres Freund @ 2026-02-03 16:26 UTC (permalink / raw)
To: pgsql-hackers; Alexander Korotkov <[email protected]>
Hi,
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.
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.
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.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: Odd code around ginScanToDelete
2026-02-03 16:26 Odd code around ginScanToDelete Andres Freund <[email protected]>
@ 2026-02-03 22:32 ` Alexander Korotkov <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Alexander Korotkov @ 2026-02-03 22:32 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers
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?
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-02-03 22:32 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-03 16:26 Odd code around ginScanToDelete Andres Freund <[email protected]>
2026-02-03 22:32 ` Alexander Korotkov <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox