On Mon, Aug 19, 2024 at 12:40 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shubham, here are my review comments for the TAP tests patch v27-0002
>
> ======
> Commit message
>
> Tap tests for 'include-generated-columns'
>
> ~
>
> But, it's more than that-- these are the TAP tests for all
> combinations of replication related to generated columns. i.e. both
> with and without 'include_generated_columns' option enabled.
>
> ======
> src/test/subscription/t/011_generated.pl
>
> I was mistaken, thinking that the v27-0002 had already been refactored
> according to Vignesh's last review but it is not done yet, so I am not
> going to post detailed review comments until the restructuring is
> completed.
>
> ~
>
> OTOH, there are some problems I felt have crept into v26-0001 (TAP
> test is same as v27-0002), so maybe try to also take care of them (see
> below) in v28-0002.
>
> In no particular order:
>
> * I felt it is almost useless now to have the "combo" (
> "regress_pub_combo") publication. It used to have many tables when
> you first created it but with every version posted it is publishing
> less and less so now there are only 2 tables in it. Better to have a
> specific publication for each table now and forget about "combos"
>
> * The "TEST tab_gen_to_gen initial sync" seems to be not even checking
> the table data. Why not? e.g. Even if you expect no data, you should
> test for it.
>
> * The "TEST tab_gen_to_gen replication" seems to be not even checking
> the table data. Why not?
>
> * Multiple XXX comments like "... it needs more study to determine if
> the above result was actually correct, or a PG17 bug..." should be
> removed. AFAIK we should well understand the expected results for all
> combinations by now.
>
> * The "TEST tab_order replication" is now getting an error saying
> <missing replicated column: "c">, Now, that may now be the correct
> error for this situation, but in that case, then I think the test is
> not longer testing what it was intended to test (i.e. that column
> order does not matter....) Probably the table definition needs
> adjusting to make sure we are testing whenwe want to test, and not
> just making some random scenario "PASS".
>
> * The test "# TEST tab_alter" expected empty result also seems
> unhelpful. It might be related to the previous bullet.
I have addressed all the comments in the v-28-0002 Patch. Please refer
to the updated v28-0002 Patch here in [1]. See [1] for the changes
added.
[1] https://www.postgresql.org/message-id/CAHv8RjL7rkxk6qSroRPg5ZARWMdK2Nd4-QyYNeoc2vhBm3cdDg%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
