Обсуждение: Re: Some never executed code regarding the table syncworker

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

On Tue, Apr 4, 2017 at 6:26 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> 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.
>

Oops, you're right. I had misunderstood that path. Thank you.
Removed this from open item.

Regards,

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