Re: Pgoutput not capturing the generated columns

On Mon, 13 Jan 2025 at 23:52, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 13 Jan 2025 at 14:57, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Jan 13, 2025 at 5:25 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > Future -- there probably need to be further clarifications/emphasis to
> > > describe how the generated column replication feature only works for
> > > STORED generated columns (not VIRTUAL ones), but IMO it is better to
> > > address that separately *after* dealing with these missing
> > > documentation patches.
> > >
> >
> > I thought it was better to deal with the ambiguity related to the
> > 'virtual' part first. I have summarized the options we have regarding
> > this in an email [1]. I prefer to extend the current option to take
> > values as 's', and 'n'. This will keep the room open to extending it
> > with a new value 'v'. The primary reason to go this way is to avoid
> > adding new options in the future. It is better to keep the number of
> > subscription options under control. Do you have any preference?
>
> Yes, this seems a better approach. Here is the attached patch which
> handles the same.
>
Hi Vignesh,

I have reviewed the patch and have following comments:

In file: create_publication.sgml

1.
+ <para>
+ If set to <literal>stored</literal>, the generated columns present in
+ the tables associated with publication will be replicated.
</para>

Instead of 'generated columns' we should use 'stored generated columns'

======
In file: pg_publication.c

2. I feel this condition can be more specific. Since a new macro will
be introduced for upcoming Virtual Generated columns.

- if (att->attisdropped || (att->attgenerated && !include_gencols))
+ if (att->attisdropped ||
+ (att->attgenerated && (gencols_type == PUBLISH_GENCOL_NONE)))
continue;

Something like:
if (att->attisdropped)
continue;
if (att->attgenerated == ATTRIBUTE_GENERATED_STORED &&
gencols_type != PUBLISH_GENCOL_STORED)
continue;

Thoughs?

3. Similarly this can be updated here as well:

- if (att->attisdropped || (att->attgenerated && !pub->pubgencols))
+ if (att->attisdropped ||
+ (att->attgenerated && pub->pubgencols_type == PUBLISH_GENCOL_NONE))
continue;

=======
In file proto.c

4. I feel this condition should also be more specific:

/* All non-generated columns are always published. */
- return att->attgenerated ? include_gencols : true;
+ return att->attgenerated ? (gencols_type == PUBLISH_GENCOL_STORED) : true;

We should return 'true' for 'gencols_type == PUBLISH_GENCOL_STORED'
only if 'att->attgenerated = ATTRIBUTE_GENERATED_STORED'

=======
In file publicationcmds.c

5.
/*
* As we don't allow a column list with REPLICA IDENTITY FULL, the
- * publish_generated_columns option must be set to true if the table
- * has any stored generated columns.
+ * publish_generated_columns option must be set to 's'(stored) if the
+ * table has any stored generated columns.
*/
- if (!pubgencols &&
+ if (gencols_type == PUBLISH_GENCOL_NONE &&
relation->rd_att->constr &&

To be consistent with the comment, I think we should check if
'gencols_type != PUBLISH_GENCOL_STORED' instead of 'gencols_type ==
PUBLISH_GENCOL_NONE'.
Thoughts?

Thanks and Regards,
Shlok Kyal