Re: Pgoutput not capturing the generated columns - Mailing list pgsql-hackers

On Tue, 8 Oct 2024 at 11:37, 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.

Few comments:
1) Since we are no longer throwing an error for generated columns, the
function header comments also need to be updated accordingly " Checks
for and raises an ERROR for any; unknown columns, system columns,
duplicate columns or generated columns."
-               if (TupleDescAttr(tupdesc, attnum - 1)->attgenerated)
-                       ereport(ERROR,
-
errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
-                                       errmsg("cannot use generated
column \"%s\" in publication column list",
-                                                  colname));
-

2) Tab completion missing for "PUBLISH_GENERATED_COLUMNS" option in
ALTER PUBLICATION ... SET (
postgres=# alter publication pub2 set (PUBLISH
PUBLISH                     PUBLISH_VIA_PARTITION_ROOT

3) I was able to compile without this include, may be this is not required:
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -118,6 +118,7 @@
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
+#include "utils/rel.h"

4) You can include "\dRp+ pubname" after each of the create/alter
publication to verify the columns that will be published:
+-- Test the 'publish_generated_columns' parameter enabled or disabled for
+-- different scenarios with/without generated columns in column lists.
+CREATE TABLE gencols (a int, gen1 int GENERATED ALWAYS AS (a * 2) STORED);
+
+-- Generated columns in column list, when 'publish_generated_columns'=false
+CREATE PUBLICATION pub1 FOR table gencols(a, gen1) WITH
(publish_generated_columns=false);

+-- Generated columns in column list, when 'publish_generated_columns'=true
+CREATE PUBLICATION pub2 FOR table gencols(a, gen1) WITH
(publish_generated_columns=true);
+
+-- Generated columns in column list, then set
'publication_generate_columns'=false
+ALTER PUBLICATION pub2 SET (publish_generated_columns = false);
+
+-- Remove generate columns from column list, when
'publish_generated_columns'=false
+ALTER PUBLICATION pub2 SET TABLE gencols(a);
+
+-- Add generated columns in column list, when 'publish_generated_columns'=false
+ALTER PUBLICATION pub2 SET TABLE gencols(a, gen1);

Regards,
Vignesh