Re: Pgoutput not capturing the generated columns

Some review comments for patch v54-0004.

(since preparing this post, I saw you have already posted v55-0001 but
AFAIK, since 55-0001 is just a merge, the same review comments are
still applicable)

======
doc/src/sgml/catalogs.sgml

1.
       <para>
-       If true, this publication replicates the stored generated columns
-       present in the tables associated with the publication.
+       <literal>n</literal> indicates that the generated columns in the tables
+       associated with the publication should not be replicated.
+       <literal>s</literal> indicates that the stored generated columns in the
+       tables associated with the publication should be replicated.
       </para></entry>

It looks OK, but maybe we should use a wording style similar to that
used already for pg_subscription.substream?

Also, should this mention column lists?

SUGGESTION
Indicates how to handle generated column replication (when there is no
publication column list): <literal>n</literal> = generated columns in
the tables associated with the publication should not be replicated;
<literal>s</literal> = stored generated columns in the tables
associated with the publication should be replicated.

======
src/backend/commands/publicationcmds.c

parse_publication_options:

2.
    bool *publish_generated_columns_given,
-   bool *publish_generated_columns)
+   char *publish_generated_columns)

Why not use the PublishGencolsType enum here?

~~~

CreatePublication:

3.
- bool publish_generated_columns;
+ char publish_generated_columns;
  AclResult aclresult;

Why not use the PublishGencolsType enum here?

~~~

AlterPublicationOptions:

4.
  bool publish_generated_columns_given;
- bool publish_generated_columns;
+ char publish_generated_columns;

Why not use the PublishGencolsType enum here?

~~~

defGetGeneratedColsOption::

5.
+/*
+ * Extract the publish_generated_columns option value from a DefElem. "stored"
+ * and "none" values are accepted.
+ */
+static char
+defGetGeneratedColsOption(DefElem *def)
+{

The return type should be PublishGencolsType.

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

6.
- /* true if generated columns data should be published */
- bool pubgencols;
+ /*
+ * none if generated column data should not be published. stored if stored
+ * generated column data should be published.
+ */
+ char pubgencols_type;
 } FormData_pg_publication;


Maybe this was accidentally changed by some global replacement you
did. IMO the previous (v53) version comment was better here.

- bool pubgencols;
+ /*
+ * 'n'(none) if generated column data should not be published.
+ * 's'(stored) if stored generated column data should be published.
+ */

======

FYI, please see the attached diffs where I have already made some of
these changes while experimenting with the suggestions.

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