On 19/04/17 10:25, Kyotaro HORIGUCHI wrote: > 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? >
We just read catalogs so there should not be locking issues. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers