Re: [HACKERS] some review comments on logical rep code

At Wed, 19 Apr 2017 04:18:18 +0200, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote in
<c2cfda3b-9335-2b57-e9ee-a55a8646afcd@2ndquadrant.com>
> On 18/04/17 19:27, Fujii Masao wrote:
> > On Wed, Apr 19, 2017 at 1:35 AM, Petr Jelinek
> > <petr.jelinek@2ndquadrant.com> wrote:
> >> Thank you for working on this!
> >>
> >> On 18/04/17 10:16, Masahiko Sawada wrote:
> >>> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
> >>> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >>>>>>
> >>>>>>>> 10.
> >>>>>>>>>
> >>>>>>>>>     SpinLockAcquire(&MyLogicalRepWorker->relmutex);
> >>>>>>>>>     MyLogicalRepWorker->relstate =
> >>>>>>>>>       GetSubscriptionRelState(MyLogicalRepWorker->subid,
> >>>>>>>>>             MyLogicalRepWorker->relid,
> >>>>>>>>>             &MyLogicalRepWorker->relstate_lsn,
> >>>>>>>>>             false);
> >>>>>>>>>     SpinLockRelease(&MyLogicalRepWorker->relmutex);
> >>>>>>>>>
> >>>>>>>>> Non-"short-term" function like GetSubscriptionRelState() should not
> >>>>>>>>> be called while spinlock is being held.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> One option is adding new LWLock for the relation state change but this
> >>>>>>>> lock is used at many locations where the operation actually doesn't
> >>>>>>>> take time. I think that the discussion would be needed to get
> >>>>>>>> consensus, so patch for it is not attached.
> >>>>>>>
> >>>>>>> From the point of view of time, I agree that it doesn't seem to
> >>>>>>> harm. Bt I thing it as more significant problem that
> >>>>>>> potentially-throwable function is called within the mutex
> >>>>>>> region. It potentially causes a kind of dead lock.  (That said,
> >>>>>>> theoretically dosn't occur and I'm not sure what happens by the
> >>>>>>> dead lock..)
> >>>>>>>
> >>
> >> Hmm, I think doing what I attached should be fine here.
> > 
> > Thanks for the patch!
> > 
> >> We don't need to
> >> hold spinlock for table read, just for changing the
> >> MyLogicalRepWorker->relstate.
> > 
> > Yes, but the update of MyLogicalRepWorker->relstate_lsn also should
> > be protected with the spinlock.
> > 
> 
> Yes, sorry tired/blind, fixed.

Commit has been moved from after to before of the lock section.
This causes potential race condition. (As the same as the
potential dead-lock, I'm not sure it can cause realistic problem,
though..) Isn't it better to be after the lock section?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center