Re: Logical Replication of sequences

On Sun, 22 Jun 2025 at 08:05, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, 19 Jun 2025 at 11:26, Nisha Moond <nisha(dot)moond412(at)gmail(dot)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.
>
Hi Vignesh,

I have reviewed the patches 0001 and 0002. I do not have any comments
for 0001 patch. Here are comments for 0002 patch.

1. Initially, I have created a publication on sequence s1.
postgres=# CREATE PUBLICATION pub1 FOR ALL SEQUENCES;
CREATE PUBLICATION
postgres=# ALTER PUBLICATION pub1 SET TABLE t1;
ALTER PUBLICATION
postgres=# \d s1
Sequence "public.s1"
Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
--------+-------+---------+---------------------+-----------+---------+-------
bigint | 1 | 1 | 9223372036854775807 | 1 | no | 1
Publications:
"pub1"
postgres=# select * from pg_publication_rel;
oid | prpubid | prrelid | prqual | prattrs
-------+---------+---------+--------+---------
16415 | 16414 | 16388 | |
(1 row)

Here, we can set the publication to TABLE or TABLES FOR SCHEMA. Should
this be allowed?
If a publication is created on FOR ALL TABLES, such an operation is not allowed.

If we decide to allow it, it is currently not handled correctly as we
can still see "pub1" in Publications for sequence s1.

2. Similar to the comment given by Shveta in [1] point 3.
Same behaviour is present for ALTER PUBLICATION

postgres=# ALTER PUBLICATION pub2 ADD SEQUENCES;
ERROR: invalid publication object list
LINE 1: ALTER PUBLICATION pub2 ADD SEQUENCES;
^
DETAIL: One of TABLE, TABLES IN SCHEMA or ALL SEQUENCES must be
specified before a standalone table or schema name.

postgres=# ALTER PUBLICATION pub2 ADD TABLES;
ERROR: invalid publication object list
LINE 1: ALTER PUBLICATION pub2 ADD TABLES;
^
DETAIL: One of TABLE, TABLES IN SCHEMA or ALL SEQUENCES must be
specified before a standalone table or schema name.

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

Thanks and Regards,
Shlok Kyal