Re: Pgoutput not capturing the generated columns

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.

Thanks and Regards,
Shubham Khanna.