Here are my review comments for patch v44-0002. ====== Commit message.
1. The commit message is missing. ====== src/backend/replication/logical/tablesync.c fetch_remote_table_info: 2. +fetch_remote_table_info(char *nspname, char *relname, LogicalRepRelation *lrel, + List **qual, bool *remotegencolpresent) The name 'remotegencolpresent' sounds like it means a generated col is present in the remote table, but don't we only care when it is being published? So, would a better parameter name be more like 'remote_gencol_published'? ~~~ 3. Would it be better to introduce a new human-readable variable like: bool check_for_published_gencols = (server_version >= 180000); because then you could use that instead of having the 180000 check in multiple places. ~~~ 4. - lengthof(attrRow), attrRow); + server_version >= 180000 ? lengthof(attrRow) : lengthof(attrRow) - 1, attrRow); If you wish, that length calculation could be written more concisely like: lengthof(attrow) - (server_version >= 180000 ? 0 : 1) ~~~ 5. + if (server_version >= 180000) + *remotegencolpresent |= DatumGetBool(slot_getattr(slot, 5, &isnull)); + Should this also say Assert(!isnull)? ====== src/test/subscription/t/031_column_list.pl 6. + qq(0|1), + 'replication with generated columns in column list'); Perhaps this message should be worded slightly differently, to distinguish it from the "normal" replication message. /replication with generated columns in column list/initial replication with generated columns in column list/ ====== Kind Regards, Peter Smith. Fujitsu Australia
