Re: Logical Replication of sequences

Hi Vignesh,

v20240809-0001. No comments.
v20240809-0002. See below.
v20240809-0003. See below.
v20240809-0004. No comments.

//////////

Here are my review comments for patch v20240809-0002.

nit - Tweak wording in new docs example, because a publication only
publishes the sequences; it doesn't "synchronize" anything.

//////////

Here are my review comments for patch v20240809-0003.

fetch_sequence_list:
nit - move comment
nit - minor rewording for parameter WARNING message

======
.../replication/logical/sequencesync.c
src/backend/replication/logical/tablesync.c

1.
Currently the declaration 'sequence_states_not_ready' list seems
backwards. IMO it makes more sense for the declaration to be in
sequencesync.c, and the extern in the tablesync.c. (please also see
review comment #3 below which might affect this too).

~~~

2.
static bool
-FetchTableStates(bool *started_tx)
+FetchTableStates(void)
{
- static bool has_subrels = false;
-
- *started_tx = false;
+ static bool has_subtables = false;
+ bool started_tx = false;

Maybe give the explanation why 'has_subtables' is declared static here.

~~~

3.
I am not sure that it was an improvement to move the
process_syncing_sequences_for_apply() function into the
sequencesync.c. Calling the sequence code from the tablesync code
still looks strange. OTOH, I see why you don't want to leave it in
tablesync.c.

Perhaps it would be better to refactor/move all following functions
back to the (apply) worker.c instead:
- process_syncing_relations
- process_syncing_sequences_for_apply(void)
- process_syncing_tables_for_apply(void)

Actually, now that there are 2 kinds of 'sync' workers, maybe you
should introduce a new module (e.g. 'commonsync.c' or
'syncworker.c...), where you can put functions such as
process_syncing_relations() plus any other code common to both
tablesync and sequencesync. That might make more sense then having one
call to the other.

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