Re: Some never executed code regarding the table syncworker - Mailing list pgsql-hackers

From Kyotaro HORIGUCHI
Subject Re: Some never executed code regarding the table syncworker
Date
Msg-id 20170404.182658.226990957.horiguchi.kyotaro@lab.ntt.co.jp
Whole thread Raw
Responses Re: Some never executed code regarding the table sync worker  (Masahiko Sawada <sawada.mshk@gmail.com>)
List pgsql-hackers
Hi,

At Sat, 1 Apr 2017 02:35:00 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoCSmEm-P=ND6xXW4fF46_f9QhF-Uwdyna__MEEq=jbHfw@mail.gmail.com>
> Hi all,
> 
> After launched the sync table worker it enters ApplyWorkerMain
> function. And then the table sync worker calls
> LogicalRepSyncTableStart to synchronize the target table.

Sure,

> In
> LogicalRepSyncTableStart, finish_sync_worker is always called and then
> the table sync worker process always exits after synched.

After entering the function with the relstates
SUBREL_STATE_INIT/DATASYNC if the relstate finally reaches
SUBREL_STATE_CATCHUP, finish_sync_worker is not called and
returns with the slotname. Then the slotname will be used by the
subsequently launched apply worker. Of course, it immediately
ends if failed to catch up, though.

Aren't you missing something? or I am?

> On the other
> hand, some source code seems to suppose that the table sync worker
> still continue to working even after LogicalRepSyncTableStart
> 
> For example in ApplyWorkerMain, LogicalRepSyncTableStart returns
> replication slot name that was used for synchronization but this code
> is never executed.
> 
>     if (am_tablesync_worker())
>     {
>        char *syncslotname;
> 
>        /* This is table synchroniation worker, call initial sync. */
>        syncslotname = LogicalRepSyncTableStart(&origin_startpos);
> 
>        /* The slot name needs to be allocated in permanent memory context. */
>        oldctx = MemoryContextSwitchTo(ApplyCacheContext);
>        myslotname = pstrdup(syncslotname);
>        MemoryContextSwitchTo(oldctx);
> 
>        pfree(syncslotname);
>     }
> 
> ------
> And, since the table sync worker doesn't call process_syncing_tables
> so far process_syncing_tables_for_sync is never executed.
> 
> /*
>  * Process state possible change(s) of tables that are being synchronized.
>  */
> void
> process_syncing_tables(XLogRecPtr current_lsn)
> {
>       if (am_tablesync_worker())
>               process_syncing_tables_for_sync(current_lsn);
>       else
>               process_syncing_tables_for_apply(current_lsn);
> }
> 
> These code will be used for future enhancement? I think that since
> leaving unused code is not good we can get rid of these unnecessary
> codes. Attached patch removes unnecessary codes related to the table
> sync worker.
> Please give me feedback.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




pgsql-hackers by date:

Previous
From: Etsuro Fujita
Date:
Subject: Re: postgres_fdw bug in 9.6
Next
From: Masahiko Sawada
Date:
Subject: Re: Some never executed code regarding the table sync worker