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
