On Tue, Oct 29, 2024 at 11:30 AM Peter Smith <smithpb2...@gmail.com> wrote: > ====== > 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 feel no need to add a 'remote' to this variable name as the function name itself clarifies the same. Both in the function definition and at the caller site, we can name it '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. > It is better to add a comment because it makes this part of the code difficult to enhance in the same version (18) if required. > ~~~ > > 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) > The current way of the patch seems easier to follow. -- With Regards, Amit Kapila.
