Re: Logical Replication of sequences

On Wed, 14 May 2025 at 09:55, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Sat, May 3, 2025 at 7:28 PM vignesh C <vignesh21(at)gmail(dot)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, ", ");

Modified

> 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.

Modified

> 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.

Modified

> 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."

Modified

The comments are fixed in the v20250514 version patch attached at [1].

[1] - https://www.postgresql.org/message-id/CALDaNm3GXa-kKTe3oqmKA8oniHvZfgYUXG8mVczv4GJzFwG7bg%40mail.gmail.com

Regards,
Vignesh