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
