Re: Some never executed code regarding the table sync worker

On Tue, Apr 4, 2017 at 6:26 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hi,
>
> At Sat, 1 Apr 2017 02:35:00 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCSmEm-P=ND6xXW4fF46_f9QhF-Uwdyna__MEEq=jbHfw(at)mail(dot)gmail(dot)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