Re: Question about BarrierAttach spinlock

On Thu, May 23, 2019 at 3:44 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> On Fri, May 24, 2019 at 4:10 AM Mark Dilger <hornschnorter(at)gmail(dot)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