On Tue, 29 Oct 2024 at 11:30, Peter Smith <smithpb2250@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
