Thankyou for the feedback.
On Thu, Jan 7, 2021 at 12:45 PM Hou, Zhijie <houzj.fnst@cn.fujitsu.com> wrote:
>
> > PSA the v11 patch for the Tablesync Solution1.
> >
> > Difference from v10:
> > - Addresses several recent review comments.
> > - pg_indent has been run
> >
> Hi
>
> I took a look into the patch and have some comments.
>
> 1.
> * So the state progression is always: INIT -> DATASYNC -> SYNCWAIT ->
> - * CATCHUP -> SYNCDONE -> READY.
> + * CATCHUP -> (sync worker TCOPYDONE) -> SYNCDONE -> READY.
>
> I noticed the new state TCOPYDONE is commented between CATCHUP and SYNCDONE,
> But It seems the SUBREL_STATE_TCOPYDONE is actually set before SUBREL_STATE_SYNCWAIT[1].
> Did i miss something here ?
>
> [1]-----------------
> + UpdateSubscriptionRelState(MyLogicalRepWorker->subid,
> + MyLogicalRepWorker->relid,
> + SUBREL_STATE_TCOPYDONE,
> + MyLogicalRepWorker->relstate_lsn);
> ...
> /*
> * We are done with the initial data synchronization, update the state.
> */
> SpinLockAcquire(&MyLogicalRepWorker->relmutex);
> MyLogicalRepWorker->relstate = SUBREL_STATE_SYNCWAIT;
> ------------------
>
Thanks for reporting this mistake. I will correct the comment for the
next patch (v12)
>
> 2.
> <literal>i</literal> = initialize,
> <literal>d</literal> = data is being copied,
> + <literal>C</literal> = table data has been copied,
> <literal>s</literal> = synchronized,
> <literal>r</literal> = ready (normal replication)
>
> +#define SUBREL_STATE_TCOPYDONE 't' /* tablesync copy phase is completed
> + * (sublsn NULL) */
> The character representing 'data has been copied' in the catalog seems different from the macro define.
>
Yes, same was already previously reported [1]
[1] https://www.postgresql.org/message-id/CAA4eK1Kyi037XZzyrLE71MS2KoMmNSqa6RrQLdSCeeL27gnL%2BA%40mail.gmail.com
It will be fixed in the next patch (v12)
----
Kind Regards,
Peter Smith.
Fujitsu Australia.
