Re: Question about BarrierAttach spinlock

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.

--
Thomas Munro
https://enterprisedb.com