Re: Pgoutput not capturing the generated columns

On Fri, 19 Jul 2024 at 04:59, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi, here are some review comments for patch v19-0003
>
> ======
> src/backend/catalog/pg_publication.c
>
> 1.
> /*
>  * Translate a list of column names to an array of attribute numbers
>  * and a Bitmapset with them; verify that each attribute is appropriate
>  * to have in a publication column list (no system or generated attributes,
>  * no duplicates).  Additional checks with replica identity are done later;
>  * see pub_collist_contains_invalid_column.
>  *
>  * Note that the attribute numbers are *not* offset by
>  * FirstLowInvalidHeapAttributeNumber; system columns are forbidden so this
>  * is okay.
>  */
> static void
> publication_translate_columns(Relation targetrel, List *columns,
>   int *natts, AttrNumber **attrs)
>
> ~
>
> I though the above comment ought to change: /or generated
> attributes/or virtual generated attributes/
>
> IIRC this was already addressed back in v16, but somehow that fix has
> been lost (???).
Modified the comment

> ======
> src/backend/replication/logical/tablesync.c
>
> fetch_remote_table_info:
> nitpick - missing end space in this comment /* TODO: use
> ATTRIBUTE_GENERATED_VIRTUAL*/
>
Fixed

> ======
>
> 2.
> (in patch v19-0001)
> +# tab3:
> +# publisher-side tab3 has generated col 'b'.
> +# subscriber-side tab3 has generated col 'b', using a different computation.
>
> (here, in patch v19-0003)
>  # tab3:
> -# publisher-side tab3 has generated col 'b'.
> -# subscriber-side tab3 has generated col 'b', using a different computation.
> +# publisher-side tab3 has stored generated col 'b' but
> +# subscriber-side tab3 has DIFFERENT COMPUTATION stored generated col 'b'.
>
> It has become difficult to review these TAP tests, particularly when
> different patches are modifying the same comment. e.g. I post
> suggestions to modify comments for patch 0001. Those get addressed OK,
> only to vanish in subsequent patches like has happened in the above
> example.
>
> Really this patch 0003 was only supposed to add the word "stored", not
> revert the entire comment to something from an earlier version. Please
> take care that all comment changes are carried forward correctly from
> one patch to the next.
Fixed

I have addressed the comment in v20-0003 patch. Please refer [1].

[1]: https://www.postgresql.org/message-id/CANhcyEUzUurrX38HGvG30gV92YDz6WmnnwNRYMVY4tiga-8KZg%40mail.gmail.com

Thanks and Regards,
Shlok Kyal