Re: Skipping schema changes in publication

On Tue, 22 Jul 2025 at 07:28, Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.
>
Added the check in preprocess_pubobj_list(). I checked the syntaxes
and found that this function is not called for "FOR ALL TABLES" cases
and EXCEPT tables can only be used with "FOR ALL TABLES" publications.
So, I think handling for "EXCEPT tables" will not be required in the
function preprocess_pubobj_list()

>
> ======
> 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 (???)
>
> ~~~
Yes, it works. It works equivalent to publish_generated_columns = stored.
Eg:
postgres=# CREATE PUBLICATION pub1 FOR TABLE t1 with
(publish_generated_columns);
CREATE PUBLICATION
postgres=# select * from pg_publication;
  oid  | pubname | pubowner | puballtables | pubinsert | pubupdate |
pubdelete | pubtruncate | pubviaroot | pubgencols
-------+---------+----------+--------------+-----------+-----------+-----------+-------------+------------+------------
 16395 | pub1    |       10 | f            | t         | t         | t
        | t           | f          | s
(1 row)

For this patch, I have modified the test to use
'publish_generated_columns = stored'.

>
> 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'.
> ======

I have fixed all the comments and added the changes in the latest v18 patch.

Thanks,
Shlok Kyal