I just reviewed 0001 and got a few comments wrt code comments. I may find some time to review 0002 and 0003 next week.
> On Oct 16, 2025, at 19:23, Zhijie Hou (Fujitsu) <[email protected]> > wrote: > > <v20251016-0003-Documentation-for-sequence-synchronization.patch><v20251016-0002-New-worker-for-sequence-synchronization-du.patch><v20251016-0001-Introduce-REFRESH-SEQUENCES-for-subscripti.patch> 1 - 0001 - pg_subscription.c ``` + /* + * Skip sequence tuples. If even a single table tuple exists then the + * subscription has tables. + */ + if (get_rel_relkind(subrel->srrelid) == RELKIND_RELATION || + get_rel_relkind(subrel->srrelid) == RELKIND_PARTITIONED_TABLE) + { + has_subrels = true; + break; + } ``` The comment "If even a single table tuple exists then the subscription has tables” sounds redundant. I know it’s inherited from the old code, but now, with the “break” you newly added, the code logic is simple and clear, so I think the comment is no longer needed. 2 - 0001 - pg_subscription.c ``` @@ -542,12 +560,21 @@ HasSubscriptionTables(Oid subid) + * get_tables: get relations for tables of the subscription. + * + * get_sequences: get relations for sequences of the subscription. + * + * not_ready: + * If getting tables and not_ready is false, retrieve all tables; + * otherwise, retrieve only tables that have not reached the READY state. + * If getting sequences and not_ready is false, retrieve all sequences; + * otherwise, retrieve only sequences that have not reached the READY state. + * ``` This function comment sounds a bit verbose and repetitive. Suggested revision: ``` * get_tables: if true, include tables in the returned list. * get_sequences: if true, include sequences in the returned list. * not_ready: if true, include only objects that have not reached the READY state; * if false, include all objects of the requested type(s). ``` 3 - 0001 - subscriptioncmds.c ``` + * Currently, a replication slot is created for all subscriptions, + * including those for empty or sequence-only publications. While + * this is unnecessary, optimizing this behavior would require + * additional handling to ensure the apply worker operates + * smoothly without acquiring a slot on the publisher, thus adding + * complexity to the apply worker. Given that such subscriptions + * are infrequent, it doesn't seem to be worth doing anything + * about it. ``` Minor tweaks: * "optimizing this behavior” -> “optimizing it” * “doing anything about it” -> “addressing it" 4 - 0001 - subscriptioncmds.c ``` * 3) For ALTER SUBSCRIPTION ... REFRESH SEQUENCE statements with "copy_data = * true" and "origin = none": * - Warn the user that sequence data from another origin might have been * copied. ``` “Warn the user” -> “Warn users" Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
