On Thu, 21 Nov 2024 at 15:26, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 19 Nov 2024 at 19:12, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Tue, 19 Nov 2024 at 14:39, Zhijie Hou (Fujitsu)
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > On Tuesday, November 19, 2024 3:15 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > >
> > > > I noticed that we can add 'publish_generated_columns = true' for the case of
> > > > generated column. So we won't need to remove the test. I have made the
> > > > changes in v9 patch [1].
> > >
> > > I think this would unexpectedly change the original purpose of that testcase,
> > > which is to test the bug mentioned in commit b797def.
> > >
> > > Basically, I expected the new testcase to fail if we remove the codes fix added in
> > > b797def, but the new testcase can pass even after that.
> > >
> > > If we confirmed that that bug will never be triggered after applying the fix in
> > > the thread, it would be better Tt remove that testcase and mention it in the
> > > commit message.
> > >
> > I agree that we can remove the test. I debugged and found the test
> > modified in above patch does not hit the condition added in commit
> > adedf54.
> > Also, according to me we cannot trigger the bug after the fix in this
> > thread. So, I think we can remove the testcase.
> >
> > I have attached the latest patch with an updated commit message and
> > also removed the testcase.
>
> Few comments:
> 1) This seems like a copy paste from
> pub_collist_contains_invalid_column, the comments should be updated
> according to this function:
> + /*
> + * For a partition, if pubviaroot is true, find the topmost
> ancestor that
> + * is published via this publication as we need to use its
> column list for
> + * the changes.
> + *
> + * Note that even though the column list used is for an ancestor, the
> + * REPLICA IDENTITY used will be for the actual child table.
> + */
> + if (pubviaroot && relation->rd_rel->relispartition)
>
> 2) Here drop index is not required as the drop table will take care of
> dropping the index too:
> +UPDATE testpub_gencol SET a = 100 WHERE a = 1;
> +DROP PUBLICATION pub_gencol;
> +DROP INDEX testpub_gencol_idx;
> +DROP TABLE testpub_gencol;
> +RESET client_min_messages;
>
Thanks for the comments. I have fixed the comments and attached the
updated patch.
Thanks and Regards,
Shlok Kyal