Re: Logical Replication of sequences

Hi Vignesh, I have reviewed your latest patchset:

v20240814-0001. No comments
v20240814-0002. No comments
v20240814-0003. No comments
v20240814-0004. See below
v20240814-0005. No comments

//////

v20240814-0004.

======
src/backend/commands/subscriptioncmds.c

CreateSubscription:
nit - XXX comments

AlterSubscription_refresh:
nit - unnecessary parens in ereport

AlterSubscription:
nit - unnecessary parens in ereport

fetch_sequence_list:
nit - unnecessary parens in ereport

======
.../replication/logical/sequencesync.c

1. fetch_remote_sequence_data

+ * Returns:
+ * - TRUE if there are discrepancies between the sequence parameters in
+ * the publisher and subscriber.
+ * - FALSE if the parameters match.
+ */
+static bool
+fetch_remote_sequence_data(WalReceiverConn *conn, Oid relid, Oid remoteid,
+ char *nspname, char *relname, int64 *log_cnt,
+ bool *is_called, XLogRecPtr *page_lsn,
+ int64 *last_value)

IMO it is more natural to return TRUE for good results and FALSE for
bad ones. (FYI, I have implemented this reversal in the nitpicks
attachment).

~

nit - swapped columns seqmin and seqmax in the SQL to fetch them in
the natural order
nit - unnecessary parens in ereport

~~~

copy_sequence:
nit - update function comment to document the output parameter
nit - Assert that *sequence_mismatch is false on entry to this function
nit - tweak wrapping and add \n in the SQL
nit - unnecessary parens in ereport

report_sequence_mismatch:
nit - modify function comment
nit - function name changed
/report_sequence_mismatch/report_mismatched_sequences/ (now plural
(and more like the other one)

append_mismatched_sequences:
nit - param name /rel/seqrel/

~~~

2. LogicalRepSyncSequences:
+ Relation sequence_rel;
+ XLogRecPtr sequence_lsn;
+ bool sequence_mismatch;

The 'sequence_mismatch' variable must be initialized false, otherwise
we cannot trust it gets assigned.

~

LogicalRepSyncSequences:
nit - unnecessary parens in ereport
nit - move the for-loop variable declaration
nit - remove a blank line

process_syncing_sequences_for_apply:
nit - variable declaration indent

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