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 (???).
======
src/backend/replication/logical/tablesync.c
fetch_remote_table_info:
nitpick - missing end space in this comment /* TODO: use
ATTRIBUTE_GENERATED_VIRTUAL*/
======
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.
======
Kind Regards,
Peter Smith.
Fujitsu Australia.
