On Mon, Aug 19, 2024 at 12:40 PM Peter Smith <smithpb2...@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.
