Postgres Pro
Downloads
Home   >   mailing lists

Thread: Question about BarrierAttach spinlock

Question about BarrierAttach spinlock

From
Mark Dilger
Date:
Hackers,

In src/backend/storage/ipc/barrier.c, BarrierAttach
goes to the bother of storing the phase before
releasing the spinlock, and then returns the phase.

In nodeHash.c, ExecHashTableCreate ignores the
phase returned by BarrierAttach, and then immediately
calls BarrierPhase to get the phase that it just ignored.
I don't know that there is anything wrong with this, but
if the phase can be retrieved after the spinlock is
released, why hold the spinlock extra long in
BarrierAttach?

Just asking....

mark

Re: Question about BarrierAttach spinlock

From
Thomas Munro
Date:
On Fri, May 24, 2019 at 4:10 AM Mark Dilger <hornschnorter@gmail.com> wrote:
> In src/backend/storage/ipc/barrier.c, BarrierAttach
> goes to the bother of storing the phase before
> releasing the spinlock, and then returns the phase.
>
> In nodeHash.c, ExecHashTableCreate ignores the
> phase returned by BarrierAttach, and then immediately
> calls BarrierPhase to get the phase that it just ignored.
> I don't know that there is anything wrong with this, but
> if the phase can be retrieved after the spinlock is
> released, why hold the spinlock extra long in
> BarrierAttach?
>
> Just asking....

Well spotted.  I think you're right, and we could release the spinlock
a nanosecond earlier.  It must be safe to move that assignment, for
the reason explained in the comment of BarrierPhase(): after we
release the spinlock, we are attached, and the phase cannot advance
without us.  I will contemplate moving that for v13 on principle.

As for why ExecHashTableCreate() calls BarrierAttach(build_barrier)
and then immediately calls BarrierPhase(build_barrier), I suppose I
could remove the BarrierAttach() line and change the BarrierPhase()
call to BarrierAttach(), though I think that'd be slightly harder to
follow.  I suppose I could introduce a variable phase.

-- 
Thomas Munro
https://enterprisedb.com

Re: Question about BarrierAttach spinlock

From
Mark Dilger
Date:
On Thu, May 23, 2019 at 3:44 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Fri, May 24, 2019 at 4:10 AM Mark Dilger <hornschnorter@gmail.com> wrote:
> > In src/backend/storage/ipc/barrier.c, BarrierAttach
> > goes to the bother of storing the phase before
> > releasing the spinlock, and then returns the phase.
> >
> > In nodeHash.c, ExecHashTableCreate ignores the
> > phase returned by BarrierAttach, and then immediately
> > calls BarrierPhase to get the phase that it just ignored.
> > I don't know that there is anything wrong with this, but
> > if the phase can be retrieved after the spinlock is
> > released, why hold the spinlock extra long in
> > BarrierAttach?
> >
> > Just asking....
>
> Well spotted.  I think you're right, and we could release the spinlock
> a nanosecond earlier.  It must be safe to move that assignment, for
> the reason explained in the comment of BarrierPhase(): after we
> release the spinlock, we are attached, and the phase cannot advance
> without us.  I will contemplate moving that for v13 on principle.
>
> As for why ExecHashTableCreate() calls BarrierAttach(build_barrier)
> and then immediately calls BarrierPhase(build_barrier), I suppose I
> could remove the BarrierAttach() line and change the BarrierPhase()
> call to BarrierAttach(), though I think that'd be slightly harder to
> follow.  I suppose I could introduce a variable phase.

Thanks for the explanation!

mark