On Tue, 29 Oct 2024 at 11:30, Peter Smith <smithpb2...@gmail.com> wrote: > > Here are my review comments for patch v44-0002. > > ====== > Commit message. > > 1. > The commit message is missing.
This patch is now merged, so no change required. > ====== > 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'? I have changed it to gencol_published based on Amit's suggestion at [1]. > ~~~ > > 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. I felt this is not required, so not making any change for this. > ~~~ > > 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) I felt the current one is better, also Amit feels the same way as in [1]. Not making any change for this. > ~~~ > > 5. > + if (server_version >= 180000) > + *remotegencolpresent |= DatumGetBool(slot_getattr(slot, 5, &isnull)); > + > > Should this also say Assert(!isnull)? Added an assert > ====== > 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/ Modified The v45 version patch attached at [2] has the changes for the same. [1] - https://www.postgresql.org/message-id/CAA4eK1Lpzy3eqd2AOM%2BTXp80SFL1cCfX3cf9thjL-hJxn%2BAYGA%40mail.gmail.com [2] - https://www.postgresql.org/message-id/CALDaNm1oc-%2Buav380Z1k6gCZY5GJn5ZYKRexwM%2BqqGiRinUS-Q%40mail.gmail.com Regards, Vignesh
