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. -- Thomas Munro https://enterprisedb.com
