Re: Logical Replication of sequences - Mailing list pgsql-hackers

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) <houzj.fnst@fujitsu.com> 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
inheritedfrom the old code, but now, with the “break” you newly added, the code logic is simple and clear, so I think
thecomment 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/