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(dot)jelinek(at)2ndquadrant(dot)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(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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