> 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
