Re: Interval for launching the table sync worker

On Thu, Apr 20, 2017 at 8:43 PM, Petr Jelinek
<petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
> On 20/04/17 06:21, Masahiko Sawada wrote:
>> On Thu, Apr 20, 2017 at 12:30 AM, Petr Jelinek
>> <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>>> On 19/04/17 15:57, Masahiko Sawada wrote:
>>>> On Wed, Apr 19, 2017 at 10:07 PM, Petr Jelinek
>>>> <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>>>>> On 19/04/17 14:42, Masahiko Sawada wrote:
>>>>>> On Wed, Apr 19, 2017 at 5:12 PM, Kyotaro HORIGUCHI
>>>>>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>>>>> At Tue, 18 Apr 2017 18:40:56 +0200, Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote in <f64d87d1-bef3-5e3e-a999-ba302816a0ee(at)2ndquadrant(dot)com>
>>>>>>>> On 18/04/17 18:14, Peter Eisentraut wrote:
>>>>>>>>> On 4/18/17 11:59, Petr Jelinek wrote:
>>>>>>>>>> Hmm if we create hashtable for this, I'd say create hashtable for the
>>>>>>>>>> whole table_states then. The reason why it's list now was that it seemed
>>>>>>>>>> unnecessary to have hashtable when it will be empty almost always but
>>>>>>>>>> there is no need to have both hashtable + list IMHO.
>>>>>>>
>>>>>>> I understant that but I also don't like the frequent palloc/pfree
>>>>>>> in long-lasting context and double loop like Peter.
>>>>>>>
>>>>>>>>> The difference is that we blow away the list of states when the catalog
>>>>>>>>> changes, but we keep the hash table with the start times around. We
>>>>>>>>> need two things with different life times.
>>>>>>>
>>>>>>> On the other hand, hash seems overdone. Addition to that, the
>>>>>>> hash-version leaks stale entries while subscriptions are
>>>>>>> modified. But vacuuming them costs high.
>>>>>>>
>>>>>>>> Why can't we just update the hashtable based on the catalog? I mean once
>>>>>>>> the record is not needed in the list, the table has been synced so there
>>>>>>>> is no need for the timestamp either since we'll not try to start the
>>>>>>>> worker again.
>>>>>>
>>>>>> I guess the table sync worker for the same table could need to be
>>>>>> started again. For example, please image a case where the table
>>>>>> belonging to the publication is removed from it and the corresponding
>>>>>> subscription is refreshed, and then the table is added to it again. We
>>>>>> have the record of the table with timestamp in the hash table when the
>>>>>> table sync in the first time, but the table sync after refreshed could
>>>>>> have to wait for the interval.
>>>>>>
>>>>>
>>>>> But why do we want to wait in such case where user has explicitly
>>>>> requested refresh?
>>>>>
>>>>
>>>> Yeah, sorry, I meant that we don't want to wait but cannot launch the
>>>> tablesync worker in such case.
>>>>
>>>> But after more thought, the latest patch Peter proposed has the same
>>>> problem. Perhaps we need to scan always whole pg_subscription_rel and
>>>> remove the entry if the corresponding table get synced.
>>>>
>>>
>>> Yes that's what I mean by "Why can't we just update the hashtable based
>>> on the catalog". And if we do that then I don't understand why do we
>>> need both hastable and linked list if we need to update both based on
>>> catalog reads anyway.
>>
>> Thanks, I've now understood correctly. Yes, I think you're right. If
>> we update the hash table based on the catalog whenever table state is
>> invalidated, we don't need to have both hash table and list.
>>
>> BTW, in current HEAD the SUBREL_STATE_SYNCWAIT is not stored in the
>> pg_subscription_catalog. So the following condition seems not correct.
>> We should use "syncworker->relstate == SUBSCRIPTION_STATE_SYNCWAIT"
>> instead?
>>
>> /*
>> * There is a worker synchronizing the relation and waiting for
>> * apply to do something.
>> */
>> if (syncworker && rstate->state == SUBREL_STATE_SYNCWAIT)
>> {
>> /*
>> * There are three possible synchronization situations here.
>> *
>> * a) Apply is in front of the table sync: We tell the table
>> * sync to CATCHUP.
>> *
>> * b) Apply is behind the table sync: We tell the table sync
>> * to mark the table as SYNCDONE and finish.
>>
>> * c) Apply and table sync are at the same position: We tell
>> * table sync to mark the table as READY and finish.
>> *
>> * In any case we'll need to wait for table sync to change
>> * the state in catalog and only then continue ourselves.
>> */
>>
>
> Good catch. Although it's not comment that's wrong, it's the if. We
> should not compare rstate->state but syncworker->relstate.

I've attached a patch to fix this bug.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center