public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Shlok Kyal <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Zhijie Hou (Fujitsu) <[email protected]>
Cc: vignesh C <[email protected]>
Cc: YeXiu <[email protected]>
Cc: Ian Lawrence Barwick <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Skipping schema changes in publication
Date: Tue, 22 Jul 2025 11:58:29 +1000
Message-ID: <CAHut+PuviFA6C7qps=+kDYfe3P99as8NCjbR=SYxoi0o96ipoA@mail.gmail.com> (raw)
In-Reply-To: <CANhcyEUtYV-9ujtxLasnxN_peT+3LuZjcRx1xUECh1CCmANB8w@mail.gmail.com>
References: <CALDaNm3=JrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh=tamA@mail.gmail.com>
<CALj2ACVOzhs+BD+abFV2x4oKJdsDNd6SgsE7r8UjnZDCKGEckA@mail.gmail.com>
<CAA4eK1K6Kr88d2S0zFdHRMyuoaZeNh+ktU+oigmCuD09_x_-+g@mail.gmail.com>
<CAHut+PsvC-NezO3MJkdyEz=G1QRje2LntjwhQiEeVbmhOQuBMA@mail.gmail.com>
<CALDaNm18VH2j8cTqfELHQ=0ZNognbGBhbHPteJenWQC6C2dueQ@mail.gmail.com>
<CALDaNm0k_0Ccj47wzJzzPFwgQB7w=R5+Q2_nSqYrmMmjhmcRUw@mail.gmail.com>
<CAHut+Pv_0DwyWoGQNMF+G2AGqMuJTzWQKRtmxaC+=zLTPL-Zkw@mail.gmail.com>
<CALDaNm2-GJt2HsYTkLqQ=ecm=R-vOBw1=aM_d2EiYbz39x_cTQ@mail.gmail.com>
<TYCPR01MB8373C3120C2B3112001ED6F1EDCF9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm0iZZDB300Dez_97S8G6_RW5QpQ8ef6X3wq8tyK-8wnXQ@mail.gmail.com>
<CAHut+PtiomM+iyAZHvb2dzfsPvRru266KuBe49hKy2n2h+m_zA@mail.gmail.com>
<CALDaNm30KDnwX4Czi29fqLb8JBkuwqjbpj9ixwNXXox574NZqQ@mail.gmail.com>
<CALDaNm1PfKRJsEzbKpyt=v4p3bw+_SzE+LFPsMhR5X+qs+0pPw@mail.gmail.com>
<TYCPR01MB83730A2F1D6A5303E9C1416AEDD99@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm0sAU4s1KTLOEWv=rYo5dQK6uFTJn_0FKj3XG1Nv4D-qw@mail.gmail.com>
<CALDaNm3CLRa95tpas6tEj8x58MUNDShxBNoYS+P8Uq5cryoAOw@mail.gmail.com>
<CALDaNm0EKC3o=v+F7GneGibuCULGKkBWXmNaVB4GR9HoqD066A@mail.gmail.com>
<CALDaNm1Z1Rmqj9s6P9ZzmrVA9F_vZ_DwwhYAJmsjqmY6dS3-hA@mail.gmail.com>
<CAB8KJ=jJGuW=ozKmXZzKDUHZ_-J2ZYGOtJo=i2cnNbSu6=KuYg@mail.gmail.com>
<CALDaNm1mbFP8fxHU_H1Ex4cT2Aq3n8FE79tq0TO5ThvFnDUYMA@mail.gmail.com>
<CAB8KJ=jq4RwTs8K7pokmXQwQppP2ChVJLMSAdXaxAX+c1r+mdg@mail.gmail.com>
<CALDaNm1mJvLni8GODebKBmyegXuZ18bLoG-Pz6H1MCX=vphCYA@mail.gmail.com>
<CALDaNm3dWZCYDih55qTNAYsjCvYXMFv=46UsDWmfCnXMt3kPCg@mail.gmail.com>
<CALDaNm1AQZYgT0tALRrkvpP1Q+8+e7vkGCUjQ-jim1C0q3e=zA@mail.gmail.com>
<CAA4eK1KRdAPC=5=7tQ1GW0cRwD=zaDMi+T4u_k4GxPhPY6e8BQ@mail.gmail.com>
<OS3PR01MB5718C8BE84B862E7E0CEC29B94BD2@OS3PR01MB5718.jpnprd01.prod.outlook.com>
<CAA4eK1KYQz7cf46_D=6VkZ4J6Y8vJ88MMi=6zm2TJXDP+V1mLg@mail.gmail.com>
<CANhcyEXZq4mP5dNgg7u=sMPwvxA4_ZN9U92uZEuzs=0xTu+8Yg@mail.gmail.com>
<CANhcyEXspT3v5-Tdop9uqQV2HWBvZoN5P0BxXQ6Md6Mr7GXK9A@mail.gmail.com>
<CAHut+PuiaLOCkiAx9nPnjk6wTbPFvnm9T5svTuKbgwJwTdea8w@mail.gmail.com>
<CANhcyEV_MePxgftHY65et1WdOAk70M0C7PZ1STPUO8PXHVB1YA@mail.gmail.com>
<CAHut+Ps0hSNqrjv_jT1AuXxO-CrZue3ixE0jKsxVhtArMrkujQ@mail.gmail.com>
<CANhcyEXX3viVpYcGHD_fzhf_f6CDQWr2+VBywrJf5zm_XiB4tg@mail.gmail.com>
<CAHut+PsXP_61ZXuVOx5u9FZGK3oH4taaA59oOzgqyygZx8ezWw@mail.gmail.com>
<CANhcyEU+aPu6iAH2cTA0cDtn3pd6c_njBONCt3FubYZoEEnm8Q@mail.gmail.com>
<CAHut+Pv2P6dJ7hZj_fmzN+=xzjvpOpgkAJvDZg3TD2xpvmY1NQ@mail.gmail.com>
<CANhcyEW2LK4diNeCG862DE40yQoV3VAgf59kXUq2TuR8fnw5vQ@mail.gmail.com>
<CAHut+PuSHScrODVGCM7P53Mv1HE2N6ThzkH4+gQ1eFXVeD-OCA@mail.gmail.com>
<CANhcyEUtYV-9ujtxLasnxN_peT+3LuZjcRx1xUECh1CCmANB8w@mail.gmail.com>
Hi Shlok,
Some review comments for patch v17-0003. I also checked the TAP test this time.
======
doc/src/sgml/logical-replication.sgml
1.
+ <literal>publish_generated_columns</literal></link>. Specifying generated
+ columns in a column list using the <literal>EXCEPT</literal> clause excludes
+ the specified generated columns from being published, regardless of the
+ <link linkend="sql-createpublication-params-with-publish-generated-columns">
+ <literal>publish_generated_columns</literal></link> setting. However, for
I think that is not quite the same wording I had previously suggested.
It sounds a bit odd/redundant saying "Specifying" and "specified" in
the same sentence.
======
src/backend/parser/gram.y
2. check_except_collist
I'm wondering if this checking should be done within the existing
preprocess_pubobj_list() function, alongside all the other ERROR
checking. Care needs to be taken to make sure the pubtable->except is
referring to an EXCEPT (col-list), instead of the other kind of EXCEPT
tables, but in general I think it is better to keep all the
publication combinations checking errors like this in one place.
======
src/bin/psql/describe.c
3. addFooterToPublicationDesc
- appendPQExpBuffer(&buf, " (%s)",
- PQgetvalue(result, i, 2));
+ {
+ if (!PQgetisnull(result, i, 3) &&
+ strcmp(PQgetvalue(result, i, 3), "t") == 0)
+ appendPQExpBuffer(&buf, " EXCEPT (%s)",
+ PQgetvalue(result, i, 2));
+ else
+ appendPQExpBuffer(&buf, " (%s)",
+ PQgetvalue(result, i, 2));
+ }
Do you really need to check !PQgetisnull(result, i, 3) here? (e.g.
The comment does not say that this attribute can be NULL)
======
.../t/037_rep_changes_except_collist.pl
4.
+# Copyright (c) 2021-2025, PostgreSQL Global Development Group
+
+# Logical replication tests for except table publications
Comment is wrong. These tests are for EXCEPT (column-list)
~~~
5.
+# Test for except column publications
+# Initial setup
+$node_publisher->safe_psql('postgres', "CREATE SCHEMA sch1");
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab2 (a int, b int NOT NULL, c int)");
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE sch1.tab2 (a int, b int, c int)");
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab3 (a int, b int, c int)");
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab4 (a int, b int GENERATED ALWAYS AS (a * 2) STORED,
c int GENERATED ALWAYS AS (a * 3) STORED)"
+);
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab5 (a int, b int GENERATED ALWAYS AS (a * 2) STORED,
c int GENERATED ALWAYS AS (a * 3) STORED)"
+);
+$node_publisher->safe_psql('postgres', "INSERT INTO tab2 VALUES (1, 2, 3)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO sch1.tab2 VALUES (1, 2, 3)");
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tap_pub_col FOR TABLE tab2 EXCEPT (a), sch1.tab2
EXCEPT (b, c)"
+);
5a.
I think you don't need to say "Test for except column publications",
because that is the purpose of thie entire file.
~
5b.
You can combine multiple of these safe_psql calls together
~
5c.
It might help make tests easier to read if you named those generated
columns 'b', 'c' cols as 'bgen', 'cgen' instead.
~
5d.
The table names are strange, because why does it start at tab2 when
there is not a tab1?
~~~
6.
+$node_subscriber->safe_psql('postgres', "CREATE SCHEMA sch1");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab2 (a int, b int NOT NULL, c int)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE sch1.tab2 (a int, b int, c int)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab3 (a int, b int, c int)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab4 (a int, b int, c int)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab5 (a int, b int, c int)");
You can combine multiple of these safe_psql calls together
~~~
7.
+# Test initial sync
+my $result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2");
+is($result, qq(|2|3),
+ 'check that initial sync for except column publication');
The message seems strange. Do you mean "check initial sync for an
'EXCEPT (column-list)' publication"
NOTE: There are many other messages where you wrote "for except column
publication" but I think maybe all of those can be improved a bit like
above.
~~~
8.
+$node_publisher->safe_psql('postgres', "INSERT INTO tab2 VALUES (4, 5, 6)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO sch1.tab2 VALUES (4, 5, 6)");
+$node_publisher->wait_for_catchup('tap_sub_col');
8a.
You can combine multiple of these safe_psql calls together.
NOTE: I won't keep repeating this review comment but I think maybe
there are lots more places where the safe_psql can all be combined to
expected multiple statements.
~
8b.
I felt all those commands should be under the "Test incremental
changes" comment.
~~~
9.
+is($result, qq(1||3), 'check alter publication with EXCEPT');
Maybe that should've said with 'EXCEPT (column-list)'
~~~
10.
+# Test for publication created with publish_generated_columns as true on table
+# with generated columns and column list specified with EXCEPT
+$node_publisher->safe_psql('postgres', "INSERT INTO tab4 VALUES (1)");
+$node_publisher->safe_psql('postgres',
+ "ALTER PUBLICATION tap_pub_col SET (publish_generated_columns)");
+$node_publisher->safe_psql('postgres',
+ "ALTER PUBLICATION tap_pub_col SET TABLE tab4 EXCEPT(b)");
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tap_sub_col REFRESH PUBLICATION");
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub_col');
10a.
I felt the test comments for both those generated columns parameter
test should give more explanation to say what is the expected result
and why.
~
10b.
How does "ALTER PUBLICATION tap_pub_col SET
(publish_generated_columns)" even work? I thought the
"pubish_generated_columns" is an enum but you did not specify any enum
value here (???)
~~~
11.
+ 'check publication(publish_generated_columns as false) with
generated columns and EXCEPT'
Hmm. I thought there is no such thing as "publish_generated_columns as
false", and also the EXCEPT should say 'EXCEPT (column-list)'
~~~
12.
I wonder if there should be another boundary condition test case as follows:
- have some table with cols a,b,c.
- create a publication 'EXCEPT (a,b,c)', so you don't publish anything at all.
- then ALTER the TABLE to add a column 'd'.
- now the publication should publish only 'd'.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
view thread (377+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Skipping schema changes in publication
In-Reply-To: <CAHut+PuviFA6C7qps=+kDYfe3P99as8NCjbR=SYxoi0o96ipoA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox