On Wed, Oct 9, 2024 at 11:00 AM vignesh C <vignes...@gmail.com> wrote:
>
> 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.
>
> There is inconsistency in replication when a generated column is
> specified in the column list. The generated column data is not
> replicated during initial sync whereas it is getting replicated during
> incremental sync:
> -- publisher
> CREATE TABLE t1(c1 int, c2 int GENERATED ALWAYS AS (c1 * 2) STORED)
> INSERT INTO t1 VALUES (1);
> CREATE PUBLICATION pub1 for table t1(c1, c2);
>
> --subscriber
> CREATE TABLE t1(c1 int, c2 int)
> CREATE SUBSCRIPTION sub1 connection 'dbname=postgres host=localhost
> port=5432' PUBLICATION pub1;
>
> -- Generate column data is not synced during initial sync
> postgres=# select * from t1;
> c1 | c2
> ----+----
> 1 |
> (1 row)
>
> -- publisher
> INSERT INTO t1 VALUES (2);
>
> -- Whereas generated column data is synced during incremental sync
> postgres=# select * from t1;
> c1 | c2
> ----+----
> 1 |
> 2 | 4
> (2 rows)
>
There was an issue for this scenario:
CREATE TABLE t1(c1 int, c2 int GENERATED ALWAYS AS (c1 * 2) STORED)
create publication pub1 for table t1(c1, c2)
In this case included_cols was getting set to NULL.
Changed it to get included_cols as it is instead of replacing with
NULL and changed the condition to:
if (server_version >= 180000)
{
remotegenlist[natt] = DatumGetBool(slot_getattr(slot, 5, &isnull));
/*
* If the column is generated and neither the generated column
* option is specified nor it appears in the column list, we will
* skip it.
*/
if (remotegenlist[natt] && !has_pub_with_pubgencols && !included_cols)
{
ExecClearTuple(slot);
continue;
}
}
I will further think if there is a better solution for this.
Please refer to the updated v39 Patches here in [1]. See [1] for the
changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjLjb%2B98i5ZQUphivxdOZ3hSGLfq2SiWQetUvk8zGyAQwQ%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
