Re: Logical Replication of sequences

On Sat, May 3, 2025 at 7:28 PM vignesh C <vignesh21@gmail.com> wrote:
>
>
> There was one pending open comment #6 from [1]. This has been
> addressed in the attached patch.

Thank you for the patches, here are my comments for patch-004: sequencesync.c

copy_sequences()
-------------------
1)
+ if (first)
+ first = false;
+ else
+ appendStringInfoString(seqstr, ", ");

We can avoid additional variable here, suggestion -
if (seqstr->len > 0)
  appendStringInfoString(seqstr, ", ");
~~~~

2)
+ else
+ {
+ *sequence_sync_error = true;
+ append_mismatched_sequences(mismatched_seqs, seqinfo);
+ }

I think *sequence_sync_error = true can be removed from here, as we
can avoid setting it for every mismatch, as it is already set at the
end of the function if any sequence mismatches are found.
~~~~

3)
+ if (message_level_is_interesting(DEBUG1))
+ {
+ /* LOG all the sequences synchronized during current batch. */
+ for (int i = 0; i < curr_batch_seq_count; i++)
+ {
+ LogicalRepSequenceInfo *done_seq;
...
+
+ ereport(DEBUG1,
+ errmsg_internal("logical replication synchronization for
subscription \"%s\", sequence \"%s\" has finished",
+ get_subscription_name(subid, false),
+ done_seq->seqname));
+ }
+ }

3a) I think the DEBUG1 log can be moved inside the while loop just
above, to avoid traversing the list again unnecessarily.
~~~~

LogicalRepSyncSequences():
-----------------------------
4)
+ /*
+ * Sequence synchronization failed due to a parameter mismatch. Set the
+ * failure time to prevent immediate initiation of the sequencesync
+ * worker.
+ */
+ if (sequence_sync_error)
+ {
+ logicalrep_seqsyncworker_set_failuretime();
+ ereport(LOG,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("sequence synchronization failed because the parameters
between the publisher and subscriber do not match for all
sequences"));
+ }

I think saying "sequence synchronization failed" could be misleading,
as the matched sequences will still be synced successfully. It might
be clearer to reword it to something like:
"sequence synchronization failed for some sequences because the
parameters between the publisher and subscriber do not match."
~~~~

--
Thanks,
Nisha