On Thu, May 23, 2019 at 3:44 PM Thomas Munro <thomas.mu...@gmail.com> wrote: > > On Fri, May 24, 2019 at 4:10 AM Mark Dilger <hornschnor...@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
