On Tue, 8 Oct 2024 at 11:37, Shubham Khanna <khannashubham1...@gmail.com> wrote:
>
> On Fri, Oct 4, 2024 at 9:36 AM Peter Smith <smithpb2...@gmail.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);
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>.
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):
[1] -
https://www.postgresql.org/docs/devel/logical-replication-subscription.html#LOGICAL-REPLICATION-SUBSCRIPTION-EXAMPLES
Regards,
Vignesh
