Re: Interval for launching the table sync worker

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.

The reason why SUBREL_STATE_SYNCWAIT is not stored in catalog is that
we'd have to commit from sync worker which could leave table in
partially synchronized state on crash without the ability to resume
afterwards (since the slot on other side will be gone).

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