On Fri, 5 Jul 2024 at 13:47, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for v14-0002.
>
> ======
> src/backend/replication/logical/tablesync.c
>
> 2. copy_table
>
> + attnamelist = make_copy_attnamelist(relmapentry, remotegenlist);
> +
> /* Start copy on the publisher. */
> initStringInfo(&cmd);
>
> - /* Regular table with no row filter */
> - if (lrel.relkind == RELKIND_RELATION && qual == NIL)
> + /* check if remote column list has generated columns */
> + if(MySubscription->includegencols)
> + {
> + for (int i = 0; i < relmapentry->remoterel.natts; i++)
> + {
> + if(remotegenlist[i])
> + {
> + remote_has_gencol = true;
> + break;
> + }
> + }
> + }
> +
>
> There is some subtle logic going on here:
>
> For example, the comment here says "Check if the remote column list
> has generated columns", and it then proceeds to iterate the remote
> attributes checking the remotegenlist[i]. But the remotegenlist[] was
> returned from a prior call to make_copy_attnamelist() and according to
> the make_copy_attnamelist logic, it is NOT returning all remote
> generated-cols in that list. Specifically, it is stripping some of
> them -- "Do not include generated columns of the subscription table in
> the [remotegenlist] column list.".
>
> So, actually this loop seems to be only finding cases (setting
> remote_has_gen = true) where the remote column is generated but the
> match local column is *not* generated. Maybe this was the intended
> logic all along but then certainly the comment should be improved to
> describe it better.
'remotegenlist' is actually constructed in function 'fetch_remote_table_info'
and it has an entry for every column in the column list specifying
whether a column is
generated or not.
In the function 'make_copy_attnamelist' we are not modifying the list.
So, I think the current comment would be sufficient. Thoughts?
> ======
> src/test/subscription/t/004_sync.pl
>
> nitpick - changes to comment style to make the test case separations
> much more obvious
> nitpick - minor comment wording tweaks
>
> 5.
> Here, you are confirming we get an ERROR when replicating from a
> non-generated column to a generated column. But I think your patch
> also added exactly that same test scenario in the 011_generated (as
> the sub5 test). So, maybe this one here should be removed?
For 0004_sync.pl, it is tested when 'include_generated_columns' is not
specified. Whereas for the test in 011_generated
'include_generated_columns = true' is specified.
I thought we should have a test for both cases to test if the error
message format is the same for both cases. Thoughts?
I have attached the patches and I have addressed the rest of the
comment and added changes in v16-0002. I have not modified the
v16-0001 patch.
Thanks and Regards,
Shlok Kyal
