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.
~~~
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.
~~~
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".
~~~~
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.
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/
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
--
Thanks,
Nisha
vnisha_005-top-up-patch-for-error-message-improvment.patch
Description: Binary data
