Re: Pgoutput not capturing the generated columns

On Thu, 23 Jan 2025 at 05:52, Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.
> + */

Thanks for the comments, the attached v56 version patch has the fixes
for the above comments. v56-0002 is the same as the patch posted at
[1].
[1] - https://www.postgresql.org/message-id/CAHut%2BPuD7_yh54z-sGyK9xBq9cwUeJUG0BRQaHOhCLdw8msnjQ%40mail.gmail.com

Regards,
Vignesh