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.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/replication/logical/tablesync.c
b/src/backend/replication/logical/tablesync.c
index 108326b..7262709 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -703,19 +703,22 @@ copy_table(Relation rel)
char *
LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
{
- char *slotname;
- char *err;
+ char *slotname;
+ char *err;
+ char relstate;
+ XLogRecPtr relstate_lsn;
/* Check the state of the table synchronization. */
StartTransactionCommand();
+ relstate = GetSubscriptionRelState(MyLogicalRepWorker->subid,
+
MyLogicalRepWorker->relid,
+
&relstate_lsn, false);
+ CommitTransactionCommand();
+
SpinLockAcquire(&MyLogicalRepWorker->relmutex);
- MyLogicalRepWorker->relstate =
- GetSubscriptionRelState(MyLogicalRepWorker->subid,
-
MyLogicalRepWorker->relid,
-
&MyLogicalRepWorker->relstate_lsn,
- false);
+ MyLogicalRepWorker->relstate = relstate;
+ MyLogicalRepWorker->relstate_lsn = relstate_lsn;
SpinLockRelease(&MyLogicalRepWorker->relmutex);
- CommitTransactionCommand();
/*
* To build a slot name for the sync work, we are limited to
NAMEDATALEN -
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
