Question about BarrierAttach spinlock
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
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
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