Re: Logical Replication of sequences

On Thu, 19 Jun 2025 at 11:26, Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Hi,
>
> Here are my review comments for v20250610 patches:
>
> Patch-0005:sequencesync.c
>
> 1) report_error_sequences()
>
> In case there are both missing and mismatched sequences, the ERROR
> message logged is -
>
> ```
> 2025-05-28 14:22:19.898 IST [392259] ERROR:  logical replication
> sequence synchronization failed for subscription "subs": sequences
> ("public"."n84") are missing on the publisher. Additionally,
> parameters differ for the remote and local sequences ("public.n1")
> ```
>
> I feel this error message is quite long. Would it be possible to split
> it into ERROR and DETAIL? Also, if feasible, we could consider
> including a HINT, as was done in previous versions.
>
> I explored a few possible ways to log this error with a hint. Attached
> top-up patch has the suggestion implemented. Please see if it seems
> okay to consider.

This looks good, merged it.
> ~~~
>
> 2) copy_sequences():
> + /* Retrieve the sequence object fetched from the publisher */
> + for (int i = 0; i < batch_size; i++)
> + {
> + LogicalRepSequenceInfo *sequence_info =
> lfirst(list_nth_cell(remotesequences, current_index + i));
> +
> + if (!strcmp(sequence_info->nspname, nspname) &&
> + !strcmp(sequence_info->seqname, seqname))
> + seqinfo = sequence_info;
> + }
>
> The current logic performs a search through the local sequence list
> for each sequence fetched from the publisher, repeating the traverse
> of 100(batch size) length of the list per sequence, which may impact
> performance.
>
> To improve efficiency, we can optimize it by sorting the local list
> and traverses it only once for matching. Kindly review the
> implementation in the attached top-up patch and consider merging it if
> it looks good to you.

Looks good, merged it.

> ~~~
>
> 3) copy_sequences():
> + if (message_level_is_interesting(DEBUG1))
> + ereport(DEBUG1,
> + errmsg_internal("logical replication synchronization for
> subscription \"%s\", sequence \"%s\" has finished",
> + MySubscription->name,
> + seqinfo->seqname));
> +
> + batch_succeeded_count++;
> + }
>
> The current debug log might be a bit confusing when sequences with the
> same name exist in different schemas. To improve clarity, we could
> include the schema name in the message, e.g.,
> " ... sequence "schema"."sequence" has finished".

Modified

> ~~~~
>
> Few minor comments in doc - Patch-0006 : logical-replication.sgml
>
> 4)
> +  <para>
> +   To replicate sequences from a publisher to a subscriber, first publish them
> +   using <link linkend="sql-createpublication-params-for-all-sequences">
> +   <command>CREATE PUBLICATION ... FOR ALL SEQUENCES</command></link>.
> +  </para>
>
> I think it would be better to use "To synchronize" instead of "To
> replicate" here, to maintain consistency and avoid confusion between
> replication and synchronization.

Modified

> 5)
> +   <para>
> +    Update the sequences at the publisher side few times.
> +<programlisting>
>
> /side few /side a few /
>
> 6) Can avoid using multiple "or" in the sentences below:
>
> 6a)
> -   a change set or replication set.  Each publication exists in only
> one database.
> +   generated from a table or a group of tables or the current state of all
> +   sequences, and might also be described as a change set or replication set
>
> / table or a group of tables/ table, a group of tables/

Modified

> 6b)
> +   Publications may currently only contain tables or sequences. Objects must be
> +   added explicitly, except when a publication is created using
> +   <literal>FOR TABLES IN SCHEMA</literal>, or <literal>FOR ALL
> TABLES</literal>,
> +   or <literal>FOR ALL SEQUENCES</literal>. Unlike tables, the current state of
>
> / IN SCHEMA</literal>, or <literal>FOR ALL TABLES/ IN
> SCHEMA</literal>, <literal>FOR ALL TABLES

Modified

Thanks for the comment, the attached v20250622 version patch has the
changes for the same.

Regards,
Vignesh