Обсуждение: 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