Re: Pgoutput not capturing the generated columns

On Mon, Oct 7, 2024 at 11:07 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Fri, Oct 4, 2024 at 9:36 AM Peter Smith <smithpb2250@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.
>

Regarding the 0001 patch, it seems to me that UPDATE and DELETE are
allowed on the table even if its replica identity is set to generated
columns that are not published. For example, consider the following
scenario:

create table t (a int not null, b int generated always as (a + 1)
stored not null);
create unique index t_idx on t (b);
alter table t replica identity using index t_idx;
create publication pub for table t with (publish_generated_columns = false);
insert into t values (1);
update t set a = 100 where a = 1;

The publication pub doesn't include the generated column 'b' which is
the replica identity of the table 't'. Therefore, the update message
generated by the last UPDATE would have NULL for the column 'b'. I
think we should not allow UPDATE and DELETE on such a table.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com