Re: Pgoutput not capturing the generated columns

Hi - Here is patch set v38 to address documentation review comments.

Changes from v37:
v38-0001.
- The code is unchanged.
- The SGML changes previously in 0001 are now in 0002.
- In passing, the commit message was improved to address review
comments from Peter [1].
v38-0002.
- Now includes all SGML changes previously in patch 0001.
- Addresses all docs review comments from Vignesh [2].
v38-0003.
- Unchanged.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPv8Do3b7QhzHg7dRWhO317ZFZKY_mYQaFBOWVQ-P1805A%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALDaNm3VcrDdF2A2x5QTPhajUxy150z3wsV4W4OGOwTFE4--Wg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

//////////

Details are below:

On Wed, Oct 9, 2024 at 8:24 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 8 Oct 2024 at 11:37, Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Fri, Oct 4, 2024 at 9:36 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > Hi Shubham, here are my review comments for v36-0001.
> > >
> > > ======
> > > 1. General - merge patches
> > >
> > > It is long past due when patches 0001 and 0002 should've been merged.
> > > AFAIK the split was only because historically these parts had
> > > different authors. But, keeping them separated is not helpful anymore.
> > >
> > > ======
> > > src/backend/catalog/pg_publication.c
> > >
> > > 2.
> > > Bitmapset *
> > > -pub_collist_validate(Relation targetrel, List *columns)
> > > +pub_collist_validate(Relation targetrel, List *columns, bool pubgencols)
> > >
> > > Since you removed the WARNING, this parameter 'pubgencols' is unused
> > > so it should also be removed.
> > >
> > > ======
> > > src/backend/replication/pgoutput/pgoutput.c
> > >
> > > 3.
> > > /*
> > > - * If the publication is FOR ALL TABLES then it is treated the same as
> > > - * if there are no column lists (even if other publications have a
> > > - * list).
> > > + * To handle cases where the publish_generated_columns option isn't
> > > + * specified for all tables in a publication, we must create a column
> > > + * list that excludes generated columns. So, the publisher will not
> > > + * replicate the generated columns.
> > > */
> > > - if (!pub->alltables)
> > > + if (!(pub->alltables && pub->pubgencols))
> > >
> > > I still found that comment hard to understand. Does this mean to say
> > > something like:
> > >
> > > ------
> > > Process potential column lists for the following cases:
> > >
> > > a. Any publication that is not FOR ALL TABLES.
> > >
> > > b. When the publication is FOR ALL TABLES and
> > > 'publish_generated_columns' is false.
> > > A FOR ALL TABLES publication doesn't have user-defined column lists,
> > > so all columns will be replicated by default. However, if
> > > 'publish_generated_columns' is set to false, column lists must still
> > > be created to exclude any generated columns from being published
> > > ------
> > >
> > > ======
> > > src/test/regress/sql/publication.sql
> > >
> > > 4.
> > > +SET client_min_messages = 'WARNING';
> > > +CREATE TABLE gencols (a int, gen1 int GENERATED ALWAYS AS (a * 2) STORED);
> > >
> > > AFAIK you don't need to keep changing 'client_min_messages',
> > > particularly now that you've removed the WARNING message that was
> > > previously emitted.
> > >
> > > ~
> > >
> > > 5.
> > > nit - minor comment changes.
> > >
> > > ======
> > > Please refer to the attachment which implements any nits from above.
> > >
> >
> > I have fixed all the given comments. Also, I have created a new 0003
> > patch for the TAP-Tests related to the '011_generated.pl' file. I am
> > planning to merge 0001 and 0003 patches once they will get fixed.
> > The attached patches contain the required changes.
>
> Few comments for v37-0002 patch:
> 1.a) We could include the output of each command execution like
> "CREATE TABLE", "INSERT 0 3" and "CREATE PUBLICATION" as we have done
> in other places like in [1]:
> +test_pub=# CREATE TABLE tab_gen_to_gen (a int, b int GENERATED ALWAYS
> AS (a + 1) STORED);
> +test_pub=# INSERT INTO tab_gen_to_gen VALUES (1),(2),(3);
> +test_pub=# CREATE PUBLICATION pub1 FOR TABLE tab_gen_to_gen;
>
> 1.b) Similarly here too:
> +test_sub=# CREATE TABLE tab_gen_to_gen (a int, b int GENERATED ALWAYS
> AS (a * 100) STORED);
> +test_sub=# CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=test_pub'
> PUBLICATION pub1;
> +test_sub=# SELECT * from tab_gen_to_gen;
>
> 1.c) Similarly here too:
> +<programlisting>
> +test_pub=# CREATE TABLE t1 (a int PRIMARY KEY, b int,
> +test_pub-# c int GENERATED ALWAYS AS (a + 1) STORED,
> +test_pub-# d int GENERATED ALWAYS AS (b + 1) STORED);
> +
> +test_pub=# CREATE TABLE t2 (a int PRIMARY KEY, b int,
> +test_pub-# c int GENERATED ALWAYS AS (a + 1) STORED,
> +test_pub-# d int GENERATED ALWAYS AS (b + 1) STORED);
> +</programlisting>
> +<programlisting>
> +test_sub=# CREATE TABLE t1 (a int PRIMARY KEY, b int,
> +test_sub-# c int,
> +test_sub-# d int GENERATED ALWAYS AS (b * 100) STORED);
> +
> +test_sub=# CREATE TABLE t2 (a int PRIMARY KEY, b int,
> +test_sub-# c int,
> +test_sub-# d int);
>
> 1.d) Similarly here too:
> +<programlisting>
> +test_pub=# CREATE PUBLICATION pub1 FOR TABLE t1, t2(a,c)
> +test_pub-# WITH (publish_generated_columns=false);
> +</programlisting>
> +<programlisting>
> +test_sub=# CREATE SUBSCRIPTION sub1
> +test_sub-# CONNECTION 'dbname=test_pub'
> +test_sub-# PUBLICATION pub1;
> +</programlisting>
>
> 1.e) Similarly here too:
> + Insert some data to the publisher tables:
> +<programlisting>
> +test_pub=# INSERT INTO t1 VALUES (1,2);
> +test_pub=# INSERT INTO t2 VALUES (1,2);
>

OK. All the above are fixed in v38-0002. I had thought this change was
mostly a waste of space, but I hadn't noticed there was a precedent.
IMO consistency is better, so I have made all the changes as you
suggested.

> 2) All of the document changes of ddl.sgml, protocol.sgml,
> create_publication.sgml can also be moved from 0001 patch to 0002
> patch:
> diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
> index 8ab0ddb112..7b9c349343 100644
> --- a/doc/src/sgml/ddl.sgml
> +++ b/doc/src/sgml/ddl.sgml
> @@ -514,8 +514,10 @@ CREATE TABLE people (
> </listitem>
> <listitem>
> <para>
> - Generated columns are skipped for logical replication and cannot be
> - specified in a <command>CREATE PUBLICATION</command> column list.
> + Generated columns may be skipped during logical replication
> according to the
> + <command>CREATE PUBLICATION</command> parameter
> + <link linkend="sql-createpublication-params-with-publish-generated-columns">
> + <literal>publish_generated_columns</literal></link>.
>

OK. SGML updates are moved to patch 0002 in v38.

> 3) I felt "(except generated columns)" should be removed from here too:
> <variablelist>
> <varlistentry id="protocol-logicalrep-message-formats-TupleData">
> <term>TupleData</term>
> <listitem>
> <variablelist>
> <varlistentry>
> <term>Int16</term>
> <listitem>
> <para>
> Number of columns.
> </para>
> </listitem>
> </varlistentry>
> </variablelist>
>
> <para>
> Next, one of the following submessages appears for each column
> (except generated columns):
>

OK. This is fixed in v38. This change is in "53.9. Logical Replication
Message Formats".