Re: Logical Replication of sequences

Hi Vignesh,

Here are some review comments for the patch v20241211-0003.

======
src/backend/replication/logical/syncutils.c

1.
+typedef enum
+{
+ SYNC_RELATIONS_STATE_NEEDS_REBUILD,
+ SYNC_RELATIONS_STATE_REBUILD_STARTED,
+ SYNC_RELATIONS_STATE_VALID,
+} SyncingRelationsState;
+

Even though the patch's intent was only to "move" this from
tablsync.c, this enum probably deserved a comment describing its
purpose.

~~~

2.
+List    *table_states_not_ready = NIL;

Maybe it is convenient to move this, but there is something about this
list being exposed out of a "utils" module back to the tablesync.c
module that seems a bit strange. Would it make more sense for this to
be the other way around e.g. declared in the tablesync.c but exposed
to the syncutils.c? (and then similarly in subsequent patch 0004 the
sequence_states_not_ready would belong in the sequencesync.c)

~~~

3.
+/*
+ * Process possible state change(s) of tables that are being synchronized.
+ */
+void
+SyncProcessRelations(XLogRecPtr current_lsn)
+{

IIUC there was a deliberate effort to rename some comments and
functions to say "relations" instead of "tables". AFAICT that was done
to encompass the SEQUENCES which can fit under the umbrella of
"relations". Anyway, it becomes confusing sometimes when there is a
mismatch. For example, here is a function (relations) with a function
comment (tables).

~~~

4.
+/*
+ * Common code to fetch the up-to-date sync state info into the static lists.
+ *
+ * Returns true if subscription has 1 or more tables, else false.
+ *
+ * Note: If this function started the transaction (indicated by the parameter)
+ * then it is the caller's responsibility to commit it.
+ */
+bool
+FetchRelationStates(bool *started_tx)

Here is another place where the function name is "relations", but the
function comment refers to "tables".

======
Kind Regards,
Peter Smith.
Fujitsu Australia