Re: Pgoutput not capturing the generated columns

On Wed, 15 Jan 2025 at 14:41, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> 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'

Modified

> ======
> 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?

Modified

> 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;

Modified

> =======
> 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'

Modified

> =======
> 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?

Modified

The v52 version patch attached at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm3OcXdY0EzDEKAfaK9gq2B67Mfsgxu93%2B_249ohyts%3D0g%40mail.gmail.com

Regards,
Vignesh