On Thu, Aug 1, 2024 at 2:02 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi, Here are my review comments for patch v22-0001
>
> All comments now are only for the TAP test.
>
> ======
> src/test/subscription/t/011_generated.pl
>
> 1. I added all new code for the missing combination test case
> "gen-to-missing". See nitpicks diff.
> - create a separate publication for this "tab_gen_to_missing" table
> because the test gives subscription errors.
> - for the initial data
> - for the replicated data
>
> ~~~
>
> 2. I added sub1 and sub2 subscriptions for every combo test
> (previously some were absent). See nitpicks diff.
>
> ~~~
>
> 3. There was a missing test case for nogen-to-gen combination, and
> after experimenting with this I am getting a bit suspicious,
>
> Currently, it seems that if a COPY is attempted then the error would
> be like this:
> 2024-08-01 17:16:45.110 AEST [18942] ERROR: column "b" is a generated column
> 2024-08-01 17:16:45.110 AEST [18942] DETAIL: Generated columns cannot
> be used in COPY.
>
> OTOH, if a COPY is not attempted (e.g. copy_data = false) then patch
> 0001 allows replication to happen. And the generated value of the
> subscriber "b" takes precedence.
>
> I have included these tests in the nitpicks diff of patch 0001.
>
> Those results weren't exactly what I was expecting. That is why it is
> so important to include *every* test combination in these TAP tests --
> because unless we know how it works today, we won't know if we are
> accidentally breaking the current behaviour with the other (0002,
> 0003) patches.
>
> Please experiment in patches 0001 and 0002 using tab_nogen_to_gen more
> to make sure the (new?) patch errors make sense and don't overstep by
> giving ERRORs when they should not.
>
> ~~~~
>
> Also, many other smaller issues/changes were done:
>
> ~~~
>
> Creating tables:
>
> nitpick - rearranged to keep all combo test SQLs in a consistent order
> throughout this file
> 1/ gen-to-gen
> 2/ gen-to-nogen
> 3/ gen-to-missing
> 4/ missing-to-gen
> 5/ nogen-to-gen
>
> nitpick - fixed the wrong comment for CREATE TABLE tab_nogen_to_gen.
>
> nitpick - tweaked some CREATE TABLE comments for consistency.
>
> nitpick - in the v22 patch many of the generated col 'b' use different
> computations for every test. It makes it unnecessarily difficult to
> read/review the expected results. So, I've made them all the same. Now
> computation is "a * 2" on the publisher side, and "a * 22" on the
> subscriber side.
>
> ~~~
>
> Creating Publications and Subscriptions:
>
>
> nitpick - added comment for all the CREATE PUBLICATION
>
> nitpick - added comment for all the CREATE SUBSCRIPTION
>
> nitpick - I moved the note about copy_data = false to where all the
> node_subscriber2 subscriptions are created. Also, don't explicitly
> refer to "patch 000" in the comment, because that will not make any
> sense after getting pushed.
>
> nitpick - I changed many subscriber names to consistently use "sub1"
> or "sub2" within the name (this is the visual cue of which
> node_subscriber<n> they are on). e.g.
> /regress_sub_combo2/regress_sub2_combo/
>
> ~~~
>
> Initial Sync tests:
>
> nitpick - not sure if it is possible to do the initial data tests for
> "nogen_to_gen" in the normal place. For now, it is just replaced by a
> comment.
> NOTE - Maybe this should be refactored later to put all the initial
> data checks in one place. I'll think about this point more in the next
> review.
>
> ~~~
>
> nitpick - Changed cleanup I drop subscriptions before publications.
>
> nitpick - remove the unnecessary blank line at the end.
>
> ======
>
> Please see the attached diffs patch (apply it atop patch 0001) which
> includes all the nipick changes mentioned above.
>
> ~~
>
> BTW, For a quicker turnaround and less churning please consider just
> posting the v23-0001 by itself instead of waiting to rebase all the
> subsequent patches. When 0001 settles down some more then rebase the
> others.
>
> ~~
>
> Also, please run the indentation tool over this code ASAP.
>
I have fixed all the comments. The attached Patch(v23-0001) contains
all the changes.
Thanks and Regards,
Shubham Khanna.