RE: Single transaction in the tablesync worker?

> 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?

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.

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?

Best regards,
houzj