At Wed, 19 Apr 2017 04:18:18 +0200, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote in <c2cfda3b-9335-2b57-e9ee-a55a8646a...@2ndquadrant.com> > On 18/04/17 19:27, Fujii Masao wrote: > > On Wed, Apr 19, 2017 at 1:35 AM, Petr Jelinek > > <petr.jeli...@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.kyot...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers