Re: Pgoutput not capturing the generated columns

Review comments for v31-0001.

(I tried to give only new comments, but there might be some overlap
with comments I previously made for v30-0001)

======
src/backend/catalog/pg_publication.c

1.
+
+ if (publish_generated_columns_given)
+ {
+ values[Anum_pg_publication_pubgencolumns - 1] =
BoolGetDatum(publish_generated_columns);
+ replaces[Anum_pg_publication_pubgencolumns - 1] = true;
+ }

nit - unnecessary whitespace above here.

======
src/backend/replication/pgoutput/pgoutput.c

2. prepare_all_columns_bms

+ /* Iterate the cols until generated columns are found. */
+ cols = bms_add_member(cols, i + 1);

How does the comment relate to the statement that follows it?

~~~

3.
+ * Skip generated column if pubgencolumns option was not
+ * specified.

nit - /pubgencolumns option/publish_generated_columns parameter/

======
src/bin/pg_dump/pg_dump.c

4.
getPublications:

nit - /i_pub_gencolumns/i_pubgencols/ (it's the same information but simpler)

======
src/bin/pg_dump/pg_dump.h

5.
+ bool pubgencolumns;
 } PublicationInfo;

nit - /pubgencolumns/pubgencols/ (it's the same information but simpler)

======
vsrc/bin/psql/describe.c

6.
  bool has_pubviaroot;
+ bool has_pubgencol;

nit - /has_pubgencol/has_pubgencols/ (plural consistency)

======
src/include/catalog/pg_publication.h

7.
+ /* true if generated columns data should be published */
+ bool pubgencolumns;
 } FormData_pg_publication;

nit - /pubgencolumns/pubgencols/ (it's the same information but simpler)

~~~

8.
+ bool pubgencolumns;
  PublicationActions pubactions;
 } Publication;

nit - /pubgencolumns/pubgencols/ (it's the same information but simpler)

======
src/test/regress/sql/publication.sql

9.
+-- Test the publication with or without 'PUBLISH_GENERATED_COLUMNS' parameter
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION pub1 FOR ALL TABLES WITH (PUBLISH_GENERATED_COLUMNS=1);
+\dRp+ pub1
+
+CREATE PUBLICATION pub2 FOR ALL TABLES WITH (PUBLISH_GENERATED_COLUMNS=0);
+\dRp+ pub2

9a.
nit - Use lowercase for the parameters.

~

9b.
nit - Fix the comment to say what the test is actually doing:
"Test the publication 'publish_generated_columns' parameter enabled or disabled"

======
src/test/subscription/t/031_column_list.pl

10.
Later I think you should add another test here to cover the scenario
that I was discussing with Sawada-San -- e.g. when there are 2
publications for the same table subscribed by just 1 subscription but
having different values of the 'publish_generated_columns' for the
publications.

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