On Thu, Jul 18, 2024 at 10:47 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shubham, here are my review comments for patch v19-0001.
>
> ======
> src/backend/replication/pgoutput/pgoutput.c
>
> 1.
> /*
> * Columns included in the publication, or NULL if all columns are
> * included implicitly. Note that the attnums in this bitmap are not
> + * publication and include_generated_columns option: other reasons should
> + * be checked at user side. Note that the attnums in this bitmap are not
> * shifted by FirstLowInvalidHeapAttributeNumber.
> */
> Bitmapset *columns;
> You replied [1] "The attached Patches contain all the suggested
> changes." but as I previously commented [2, #1], since there is no
> change to the interpretation of the 'columns' BMS caused by this
> patch, then I expected this comment would be unchanged (i.e. same as
> HEAD code). But this fix was missed in v19-0001.
>
> OTOH, if you do think there was a reason to change the comment then
> the above is still not good because "are not publication and
> include_generated_columns option" wording doesn't make sense.
>
> ======
> src/test/subscription/t/011_generated.pl
>
> Observation -- I added (in nitpicks diffs) some more comments for
> 'tab1' (to make all comments consistent with the new tests added). But
> when I was doing that I observed that tab1 and tab3 test scenarios are
> very similar. It seems only the subscription parameter is not
> specified (so 'include_generated_cols' default wll be tested). IIRC
> the default for that parameter is "false", so tab1 is not really
> testing that properly -- e.g. I thought maybe to test the default
> parameter it's better the subscriber-side 'b' should be not-generated?
> But doing that would make 'tab1' the same as 'tab2'. Anyway, something
> seems amiss -- it seems either something is not tested or is duplicate
> tested. Please revisit what the tab1 test intention was and make sure
> we are doing the right thing for it...
>
> ======
> 99.
> The attached nitpicks diff patch has some tweaked comments.
>
> ======
> [1] https://www.postgresql.org/message-id/CAHv8Rj%2BR0cj%3Dz1bTMAgQKQWx1EKvkMEnV9QsHGvOqTdnLUQi1A%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/CAHut%2BPtVfrbx0jb42LCmS%3D-LcMTtWxm%2BvhaoArkjg7Z0mvuXbg%40mail.gmail.com
The attached Patches contain all the suggested changes.
v21-0001 - Addressed the comments.
v21-0002 - Added the TAP Tests for 011_generated.pl file and modified
the patch accordingly.
v21-0003 - Added the TAP Tests for 011_generated.pl file and modified
the patch accordingly.
v21-0004- Rebased the Patch.
Thanks and Regards,
Shubham Khanna.