Re: Logical Replication of sequences

On Thu, 17 Apr 2025 at 13:52, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh,
>
> No comments for patch v20250416-0001
> No comments for patch v20250416-0002
> No comments for patch v20250416-0003
>
> Here are some comments for patch v20250416-0004
>
> ======
> src/backend/catalog/system_views.sql
>
> 1.
> +CREATE VIEW pg_publication_sequences AS
> +    SELECT
> +        P.pubname AS pubname,
> +        N.nspname AS schemaname,
> +        C.relname AS sequencename
> +    FROM pg_publication P,
> +         LATERAL pg_get_publication_sequences(P.pubname) GPS,
> +         pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)
> +    WHERE C.oid = GPS.relid;
> +
>
> Should we have some regression tests for this view?
>
> SUGGESTION
> test_pub=# CREATE SEQUENCE S1;
> test_pub=# CREATE SEQUENCE S2;
> test_pub=# CREATE PUBLICATION PUB1 FOR ALL SEQUENCES;
> test_pub=# SELECT * FROM pg_publication_sequences;
>  pubname | schemaname | sequencename
> ---------+------------+--------------
>  pub1    | public     | s1
>  pub1    | public     | s2
> (2 rows)

I felt it is not required, as this will be verified from create/alter
subscription.

> ======
> src/bin/pg_dump/pg_dump.c
>
> getSubscriptionRelations:
>
> Vignesh 16/4 answered my previous review comment #25:
> You are talking about the error message right, I have changed that.
>
> PS REPLY 17/4
> Yes, the error message, but also I thought 'tblinfo' var and
> FindTableByOid function name should refer to relations instead of
> tables?

I felt no need to change these things and bring a lot of differences
between the back branches.

The rest of the comments were fixed.

Regarding the below comments from [1].
4.
+   <para>
+    To verify, compare the sequences values between the publisher and
+    subscriber, and if necessary, execute
+    <link linkend="sql-altersubscription-params-refresh-publication-sequences">
+    <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION
SEQUENCES</command></link>.
+   </para>

/sequences values/sequence values/

Should we elaborate or give an example here, exactly how the user
should "compare the sequence values between the publisher and
subscriber".

I felt it was obvious, so no  need for an example in this case.

6.
+         <para>
+          See <xref linkend="sequence-definition-mismatches"/> for
recommendations on how
+          to handle any warnings about sequence definition differences between
+          the publisher and the subscriber, which might occur when
+          <literal>copy_data = true</literal>.
+         </para>

I have a question about functionality: I understand we do not actually
"synchronize" sequence data at this time if the copy_data=false, but
OTOH, shouldn't we still be checking (and WARNING) if there are any
pub/sub sequences difference detected, regardless of the copy_data
bool value? Otherwise, I think all we are doing is deferring the
checking/warning until later (e.g. during REFRESH PUBLICATION
SEQUENCES). Isn't it is better to get the warning earlier so the user
can fix it earlier?

I noticed the similar case with tables.
example:
pub:
create table t1(c1 int, c2 int);
create publication pub1 for table t1;
sub:
create table t1(c1 int);
create subscription sub1 connection ... publication pub1 with (copy_data=off);

In this case, we will not detect the error during create subscription
but at a later insert.
As the suggested case is similar to above, I felt it is ok.

======
doc/src/sgml/ref/create_subscription.sgml

7.
+         <para>
+          See <xref linkend="sequence-definition-mismatches"/>
+          for recommendations on how to handle any warnings about sequence
+          definition differences between the publisher and the subscriber,
+          which might occur when <literal>copy_data = true</literal>.
+         </para>

ditto previous review comment #6.

This is similar to the above comment.

The rest of the comments were fixed. The attached v20250422 version
patch has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHut+Ps2LzJwPGB8i2_ViS9c9VxeAeqDqvH5R8E-M8HvWeNfAQ@mail.gmail.com

Regards,
Vignesh