Re: Pgoutput not capturing the generated columns

Hi Vignesh.

Here are my review comments for patch v53-0003.

On Sun, Jan 19, 2025 at 11:17 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Fri, 17 Jan 2025 at 11:23, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Vignesh.
> >
> > Some review comments for patch v52-0003
> >
> > ======
> >
> > 1. GENERAL - change to use enum.
> >
> > On Thu, Jan 16, 2025 at 7:47 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Wed, 15 Jan 2025 at 11:17, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > > 2.
> > > > As suggested in more detail below, I think it would be better if you
> > > > can define a C code enum type for these potential values instead of
> > > > just using #define macros and char. I guess that will impact a lot of
> > > > the APIs.
> > >
> > > If we change it to enum, we will not be able to access
> > > PUBLISH_GENCOLS_NONE and PUBLISH_GENCOLS_STORED from describe.c files.
> > > Maybe that is the reason the macros were used in the case of
> > > pg_subscription.h also.
> >
> > Hm. I am not sure. Can't you just define the enum inside the #ifdef
> > EXPOSE_TO_CLIENT_CODE? I saw some examples of this already (see
> > src/include/catalog/pg_cast.h)
> >
> > e.g. I tried following, which compiles for me:
> >
> > #ifdef EXPOSE_TO_CLIENT_CODE
> >
> > typedef enum PublishGencolsType
> > {
> > /* Generated columns present should not be replicated. */
> > PUBLISH_GENCOLS_NONE = 'n',
> >
> > /* Generated columns present should be replicated. */
> > PUBLISH_GENCOLS_STORED = 's',
> >
> > } PublishGencolsType;
> >
> > #endif /* EXPOSE_TO_CLIENT_CODE */
> >
> > typedef struct Publication
> > {
> > Oid oid;
> > char *name;
> > bool alltables;
> > bool pubviaroot;
> > PublishGencolsType pubgencols_type;
> > PublicationActions pubactions;
> > } Publication;
>
> Yes, the compilation seems fine but listPublications which uses
> CppAsString2(PUBLISH_GENCOLS_NONE) does not get 'n' but gets it as
> publish_gencols_none like below:
> postgres=# \dRp
> ERROR: column "publish_gencols_none" does not exist
> LINE 9: WHEN PUBLISH_GENCOLS_NONE THEN 'non
>

1.
Yeah, but that is hardly any obstacle-- we can simply rewrite that
code fragment like shown below:

appendPQExpBuffer(&buf,
",\n (CASE pubgencols_type\n"
" WHEN '%c' THEN 'none'\n"
" WHEN '%c' THEN 'stored'\n"
" END) AS \"%s\"",
PUBLISH_GENCOLS_NONE,
PUBLISH_GENCOLS_STORED,
gettext_noop("Generated columns"));

...

I have made an experimental change (see PS_0003_DIFF.txt) which does
exactly this. All your tests pass just fine.

But, now I think you should be able to modify/improve lots of the APIs
to make good use of the enum PublishGencolsType instead of just
passing char.

======
doc/src/sgml/ref/create_publication.sgml

2.
<para>
Specifies whether the generated columns present in the tables
- associated with the publication should be replicated.
- The default is <literal>false</literal>.
+ associated with the publication should be replicated. Possible values
+ are <literal>none</literal> and <literal>stored</literal>.
+ The default is <literal>none</literal> meaning the generated
+ columns present in the tables associated with publication will not be
+ replicated.
+ </para>

nit - I think it looks better with a blank line above "The default
is..." giving each value its own paragraph, but it is OK as-is if you
prefer.

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

3.
* 2. Ensures that all the generated columns referenced in the REPLICA IDENTITY
- * are published either by listing them in the column list or by enabling
- * publish_generated_columns option. If any unpublished generated column is
- * found, *invalid_gen_col is set to true.
+ * are published either by listing them in the column list or if
+ * publish_generated_columns option is 's'(stored). If any unpublished
+ * generated column is found, *invalid_gen_col is set to true.

I know the "by listing them" part was existing wording but the result
now is tricky to understand.

CURRENT
Ensures that all the generated columns referenced in the REPLICA
IDENTITY are published either by listing them in the column list or if
publish_generated_columns option is 's'(stored).

SUGGESTION
Ensures that all the generated columns referenced in the REPLICA
IDENTITY are published, either by being explicitly named in the column
list or, if no column list is specified, by setting the option
publish_generated_columns = stored.

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