Re: Interval for launching the table sync worker

Hello,

At Sun, 23 Apr 2017 00:51:52 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoApBU+nz7FYaWX6gjyB9r6WWrTujbL4rrZiocHLc=pEbA(at)mail(dot)gmail(dot)com>
> On Fri, Apr 21, 2017 at 11:19 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > On Fri, Apr 21, 2017 at 5:33 PM, Kyotaro HORIGUCHI
> > <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >> Hello,
> >>
> >> At Thu, 20 Apr 2017 13:21:14 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDrw0OaHE=oVRRhQX248kjJ7W+1ViM3K76aP46HnHJsnQ(at)mail(dot)gmail(dot)com>
> >>> 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:
> >>> >> 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.
> >>
> >> Ah, ok. The patch from Peter still generating and replacing the
> >> content of the list. The attached patch stores everything into
> >> SubscriptionRelState. Oppositte to my anticiation, the hash can
> >> be efectively kept small and removed.
> >>
> >
> > Thank you for the patch!
> > Actually, I also bumped into the same the situation where we got the
> > following error during hash_seq_search. I guess we cannot commit a
> > transaction during hash_seq_scan but the sequential scan loop in
> > process_syncing_tables_for_apply could attempt to do that.

Ah. Thanks. I forgot that I saw the same error once. The hash has
nothing to do with any transactions. The scan now runs after
freezing of the hash.

Petr:
> I think we should document why this is done this way - I mean it's not
> very obvious that it's okay to update just when not found and why and
> even less obvious that it would be in fact wrong to update already
> existing entry.

It is not prudently designed. I changed there.. seemingly more
reasonable, maybe.

Petr:
> I also think that in it's current form the
> GetSubscriptionNotReadyRelations should be moved to tablesync.c

Removed then moved to tablesync.c.

Petr:
> I also think we can't add the new fields to
> SubscriptionRelState directly as that's used by catalog access
> functions as well.

Yeah, it was my lazyness. A new struct SubscriptionWorkerState is
added in tablesync.c and the interval stuff runs on it.

At Sun, 23 Apr 2017 00:51:52 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoApBU+nz7FYaWX6gjyB9r6WWrTujbL4rrZiocHLc=pEbA(at)mail(dot)gmail(dot)com>
> So I guess we should commit the changing status to SUBREL_STATE_READY
> after finished hash_seq_scan.

We could so either, but I thought that a hash can explicitly
"unfreeze" without a side effect. The attached first one adds the
function in dynahash.c. I don't think that just allowing
unregsiterd scan on unfrozen hash is worse that this.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center