On Fri, 19 Jul 2024 at 04:59, Peter Smith <smithpb2...@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
