Re: some review comments on logical rep code

On 19/04/17 10:25, Kyotaro HORIGUCHI wrote:
> At Wed, 19 Apr 2017 04:18:18 +0200, Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote in <c2cfda3b-9335-2b57-e9ee-a55a8646afcd(at)2ndquadrant(dot)com>
>> 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.
>
> 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