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