On Tue, Sep 24, 2024 at 7:08 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi. Here are my v32-0002 review comments:
>
> ======
> src/backend/replication/logical/tablesync.c
>
> 1. fetch_remote_table_info
>
> /*
> - * Get column lists for each relation.
> + * Get column lists for each relation, and check if any of the
> + * publications have the 'publish_generated_columns' parameter enabled.
>
> I am not 100% sure about this logic anymore. Maybe it is OK, but it
> requires careful testing because with Amit's "column lists take
> precedence" it is now possible for the publication to say
> 'publish_generated_columns=false', but the publication can still
> publish gencols *anyway* if they were specified in a column list.
>
> ~~~
>
This comment is still open. Will fix this in the next version of patches.
> 2.
> /*
> * Fetch info about column lists for the relation (from all the
> * publications).
> */
> + StringInfo pub_names = makeStringInfo();
> +
> + get_publications_str(MySubscription->publications, pub_names, true);
> resetStringInfo(&cmd);
> appendStringInfo(&cmd,
> ~
>
> nit - The comment here seems misplaced.
>
> ~~~
>
> 3.
> + if (server_version >= 120000)
> + {
> + has_pub_with_pubgencols = server_version >= 180000 && has_pub_with_pubgencols;
> +
> + if (!has_pub_with_pubgencols)
> + appendStringInfo(&cmd, " AND a.attgenerated = ''");
> + }
>
> My previous review comment about this [1 #10] was:
> Can the 'gencols_allowed' var be removed, and the condition just be
> replaced with if (!has_pub_with_pubgencols)? It seems equivalent
> unless I am mistaken.
>
> nit - So the current v32 code is not what I was expecting. What I
> meant was 'has_pub_with_pubgencols' can only be true if server_version
> >= 180000, so I thought there was no reason to check it again. For
> reference, I've changed it to like I meant in the nitpicks attachment.
> Please see if that works the same.
>
> ======
> [1] my review of v31-0002.
> https://www.postgresql.org/message-id/CAHut%2BPusbhvPrL1uN1TKY%3DFd4zu3h63eDebZvsF%3Duy%2BLBKTwgA%40mail.gmail.com
>
I have addressed all the comments in the v34-0002 Patch. Please refer
to the updated v34-0002 Patch here in [1]. See [1] for the changes
added.
[1] https://www.postgresql.org/message-id/CAHv8RjJkUdYCdK_bL3yvEV%3DzKrA2dsnZYa1VMT2H5v0%2BqbaGbA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
