On Mon, 13 Jan 2025 at 23:52, vignesh C <vignes...@gmail.com> wrote:
>
> On Mon, 13 Jan 2025 at 14:57, Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Mon, Jan 13, 2025 at 5:25 AM Peter Smith <smithpb2...@gmail.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
