Re: Single transaction in the tablesync worker?

On Fri, Jan 8, 2021 at 1:02 PM Hou, Zhijie <houzj.fnst@cn.fujitsu.com> wrote:
>
> > PSA the v12 patch for the Tablesync Solution1.
> >
> > Differences from v11:
> >   + Added PG docs to mention the tablesync slot
> >   + Refactored tablesync slot drop (done by
> > DropSubscription/process_syncing_tables_for_sync)
> >   + Fixed PG docs mentioning wrong state code
> >   + Fixed wrong code comment describing TCOPYDONE state
> >
> Hi
>
> I look into the new patch and have some comments.
>
> 1.
> +       /* Setup replication origin tracking. */
> ①+      originid = replorigin_by_name(originname, true);
> +       if (!OidIsValid(originid))
> +       {
>
> ②+                      originid = replorigin_by_name(originname, true);
> +                       if (originid != InvalidRepOriginId)
> +                       {
>
> There are two different style code which check whether originid is valid.
> Both are fine, but do you think it’s better to have a same style here?

Yes. I think the 1st style is better, so I used the OidIsValid for all
the new code of the patch.
But the check in DropSubscription is an exception; there I used 2nd
style but ONLY to be consistent with another originid check which
already existed in that same function.

>
>
> 2.
>   *              state to SYNCDONE.  There might be zero changes applied between
>   *              CATCHUP and SYNCDONE, because the sync worker might be ahead of the
>   *              apply worker.
> + *        - The sync worker has a intermediary state TCOPYDONE which comes after
> + *             DATASYNC and before SYNCWAIT. This state indicates that the initial
>
> This comment about TCOPYDONE is better to be placed at [1]*, where is between DATASYNC and SYNCWAIT.
>
>  *         - Tablesync worker starts; changes table state from INIT to DATASYNC while
>  *               copying.
>  [1]*
>  *         - Tablesync worker finishes the copy and sets table state to SYNCWAIT;
>  *               waits for state change.
>

Agreed. I have moved the comment per your suggestion (and I also
re-worded it again).
Fixed in latest patch [v13]

> 3.
> +       /*
> +        * To build a slot name for the sync work, we are limited to NAMEDATALEN -
> +        * 1 characters.
> +        *
> +        * The name is calculated as pg_%u_sync_%u (3 + 10 + 6 + 10 + '\0'). (It's
> +        * actually the NAMEDATALEN on the remote that matters, but this scheme
> +        * will also work reasonably if that is different.)
> +        */
> +       StaticAssertStmt(NAMEDATALEN >= 32, "NAMEDATALEN too small");   /* for sanity */
> +
> +       syncslotname = psprintf("pg_%u_sync_%u", suboid, relid);
>
> The comments says syncslotname is limit to NAMEDATALEN - 1 characters.
> But the actual size of it is (3 + 10 + 6 + 10 + '\0') = 30,which seems not NAMEDATALEN - 1.
> Should we change the comment here?
>

The comment wording is a remnant from older code which had a
differently format slot name.
I think the comment is still valid, albeit maybe unnecessary since in
the current code the tablesync slot
name length is fixed. But I left the older comment here as a safety reminder
in case some future change would want to modify the slot name. What do
you think?

----
[v13] = https://www.postgresql.org/message-id/CAHut%2BPvby4zg6kM1RoGd_j-xs9OtPqZPPVhbiC53gCCRWdNSrw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia.