Re: Logical Replication of sequences

Hi Vignesh.

Some minor review comments for the patches in set v20250514.

======

Patch 0001.

1.1
For function 'pg_sequence_state', the DOCS call the 2nd parameter
'sequence_name', but the pg_proc.dat file calls it 'seq_name'. Should
these be made the same?

////////////////////

Patch 0004.

pg_sequence_state:

4.1

- errmsg("sequence \"%s.%s\" does not exist",
+ errmsg("logical replication sequence \"%s.%s\" does not exist",

Why isn't this change already be done in an early patch when this
function was first implemented?

~~~

copy_sequences:

4.2

+/*
+ * Copy existing data of sequnces from the publisher.
+ *

Typo: "sequnces"

~~~

4.3
+{
+ int total_seq = list_length(remotesequences);
+ int curr_seq = 0;
+
+/*
+ * We batch synchronize multiple sequences per transaction, because the
+ * alternative of synchronizing each sequence individually incurs overhead of
+ * starting and committing transactions repeatedly. On the other hand, we want
+ * to avoid keeping this batch transaction open for extended periods so it is
+ * currently limited to 100 sequences per batch.
+ */
+#define MAX_SEQUENCES_SYNC_PER_BATCH 100

Wrong indent for block comment.

~~~

4.4
+ if (res->status != WALRCV_OK_TUPLES)
+ ereport(ERROR,
+ errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not receive list of sequences information from the
publisher: %s",
+ res->err));

Should this say /sequences information/sequence information/

~~~

4.5
+ ereport(LOG,
+ errmsg("Logical replication sequence synchronization - total
unsynchronized: %d, attempted in this batch: %d; succeeded in this
batch: %d; mismatched in this batch: %d for subscription: \"%s\"",
+ total_seq, batch_seq_count, batch_success_count,
+ batch_mismatch_count, get_subscription_name(subid, false)));
+

This errmsg seems backwards. I think it should be expressed like the
other one immediately above. Also I think the information can be made
shorter -- e.g. no need to say "in this batch" multiple times.

SUGGESTION
"Logical replication sequence synchronization for subscription \"%s\"
- total unsynchronized: %d; batch #%d = %d attempted, %d succeeded, %d
mismatched"

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