Re: Pgoutput not capturing the generated columns

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>
To: Shubham Khanna <khannashubham1197@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Rajendra Kumar Dangwal <dangwalrajendra888@gmail.com>, pgsql-hackers@lists.postgresql.org, euler@eulerto.com
Date: 2024-10-04T04:05:52Z
Lists: pgsql-hackers

Attachments

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.

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →
  1. Doc: Generated column replication.

  2. Rename pubgencols_type to pubgencols in pg_publication.

  3. Doc: Fix column name in pg_publication catalog.

  4. Fix buildfarm failure introduced by commit e65dbc9927.

  5. Change publication's publish_generated_columns option type to enum.

  6. Fix \dRp+ output when describing publications with a lower server version.

  7. Replicate generated columns when 'publish_generated_columns' is set.

  8. Doc: Update the behavior of generated columns in Logical Replication.

  9. Replicate generated columns when specified in the column list.

  10. Doc: Generated columns are skipped for logical replication.