RE: Logical Replication of sequences

Dear Vignesh,

Here are remained comments for v20250723 0003-0005. I've not checked the latest version.

01.
```
 *	  PostgreSQL logical replication: common synchronization code
```

How about: "Common code for synchronizations"? Since this file locates in
replication/logical, initial part is bit trivial.

02.

How do you feel to separate header file into syncutils.h file? We can put some
definitions needed for synchronizations.

03.
```
-extern void getSubscriptionTables(Archive *fout);
+extern void getSubscriptionRelations(Archive *fout);
```

Assuming that this function obtains both tables and sequences. I'm now wondering
we can say "relation" for both the tables and sequences in the context. E.g.,
getTableData()->makeTableDataInfo() seems to obtain both table and sequence data,
and dumpSequenceData() dumps sequences. How about keep getSubscriptionTables, or
getSubscriptionTablesAndSequences?

04.
```
/*
 * dumpSubscriptionTable
 *	  Dump the definition of the given subscription table mapping. This will be
 *    used only in binary-upgrade mode for PG17 or later versions.
 */
static void
dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo)
```

If you rename getSubscriptionTables, dumpSubscriptionTable should be also renamed.

05.
```
/*
 * Gets list of all relations published by FOR ALL SEQUENCES publication(s).
 */
List *
GetAllSequencesPublicationRelations(void)
```

It looks very similar with GetAllTablesPublicationRelations(). Can we combine them?
I feel we can pass the kind of target relation and pubviaroot.

06.
```
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -27,6 +27,7 @@
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
+#include "utils/memutils.h"
```

I can build without the header.

07.
```
+       /*
+        * XXX: If the subscription is for a sequence-only publication, creating a
+        * replication origin is unnecessary because incremental synchronization
+        * of sequences is not supported, and sequence data is fully synced during
+        * a REFRESH, which does not rely on the origin. If the publication is
+        * later modified to include tables, the origin can be created during the
+        * ALTER SUBSCRIPTION ... REFRESH command.
+        */
        ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname, sizeof(originname));
        replorigin_create(originname);
```

The comment is bit misleading because currently we create the replicaton origin
in the case. Can you clarify the point like:
```
XXX: Now the replication origin is created for all the cases, but it is unnecessary
when the subcription is for a sequence-only publicaiton....
```

08.
```
+                        * XXX: If the subscription is for a sequence-only publication,
+                        * creating this slot is unnecessary. It can be created later
+                        * during the ALTER SUBSCRIPTION ... REFRESH PUBLICATION or ALTER
+                        * SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES command, if the
+                        * publication is updated to include tables.
```

Same as above.

09.
```
+ *    Assert copy_data is true.
+ *    Assert refresh_tables is false.
+ *    Assert refresh_sequences is true
```

IIUC assertions are rarely described the comment stop function. If you want to
add, we should say like:
```
In Assert enabled builds, we verify that parameters are passed correctly...
```

10.
```
+#ifdef USE_ASSERT_CHECKING
+       /* Sanity checks for parameter values */
+       if (resync_all_sequences)
+               Assert(copy_data && !refresh_tables && refresh_sequences);
+#endif
```

How about below, which does not require ifdef.

```
	Assert(!resync_all_sequences ||
		   (copy_data && !refresh_tables && refresh_sequences));

```

11.
```
+               bool            issequence;
+               bool            istable;
```

Isn't it enough to use istable?

12.
```
+               /* Relation is either a sequence or a table */
+               issequence = get_rel_relkind(subrel->srrelid) == RELKIND_SEQUENCE;
```

How about adding an Assert() to ensure the relation is either of table or sequence?

13.
```
+ * not_ready:
+ * If getting tables and not_ready is false get all tables, otherwise,
+ * only get tables that have not reached READY state.
+ * If getting sequences and not_ready is false get all sequences,
+ * otherwise, only get sequences that have not reached READY state (i.e. are
+ * still in INIT state).
```
Two parts decribe mostly same point. How about:
If true, this function returns only the relations that are not in a ready state.
Otherwise returns all the relations of the subscription.

14.
```
+                       char            table_state;
```

It should be `relation_state`.

15.
```
+#define SEQ_LOG_CNT_INVALID            0
```

Can you add comment how we use it?

16.
```
+
+       TimestampTz last_seqsync_start_time;
```

I can't find the user of this attribute, is it needed?

17.
```
+                               FetchRelationStates(&has_pending_sequences);
+                               ProcessSyncingTablesForApply(current_lsn);
+                               if (has_pending_sequences)
+                                       ProcessSyncingSequencesForApply();
```

IIUC we do not always call ProcessSyncingSequencesForApply() because it would acquire
the LW lock. Can you clariy it as comments?

18.
```
+               case WORKERTYPE_SEQUENCESYNC:
+                       /* Should never happen. */
+                       Assert(0);
```

Should we call elog(ERROR) instead of Assert(0) like another case?

19.
```
	/* Find the leader apply worker and signal it. */
	logicalrep_worker_wakeup(MyLogicalRepWorker->subid, InvalidOid);
```

Do we have to signal to the leader even when the sequence worker exits?

Best regards,
Hayato Kuroda
FUJITSU LIMITED