> 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