Re: Pgoutput not capturing the generated columns

From: Peter Smith <smithpb2250(at)gmail(dot)com>
To: Shubham Khanna <khannashubham1197(at)gmail(dot)com>
Cc: vignesh C <vignesh21(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Rajendra Kumar Dangwal <dangwalrajendra888(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, euler(at)eulerto(dot)com
Subject: Re: Pgoutput not capturing the generated columns
Date: 2024-10-04 04:05:52
Message-ID: CAHut+Ps9ksUQsRNWJtAj-SrMqfQq4pixfA2DSNOU3+nk_hPmTg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

======
Kind Regards,
Peter Smith.
Fujitsu Austrlia.

Attachment Content-Type Size
PS_NITPICKS_GENCOLS_v360001.txt text/plain 4.0 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Smith 2024-10-04 04:12:55 Re: Pgoutput not capturing the generated columns
Previous Message Michael Paquier 2024-10-04 03:55:45 Re: Rename PageData to XLogPageData