Re: Single transaction in the tablesync worker?

On Fri, Jan 8, 2021 at 1:02 PM Hou, Zhijie <houzj(dot)fnst(at)cn(dot)fujitsu(dot)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.