Re: some review comments on logical rep code

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.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services