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