Re: Disallow UPDATE/DELETE on table with unpublished generated column as REPLICA IDENTITY

On Thu, 21 Nov 2024 at 15:26, vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 19 Nov 2024 at 19:12, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Tue, 19 Nov 2024 at 14:39, Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Tuesday, November 19, 2024 3:15 PM Shlok Kyal <shlok.kyal.oss@gmail.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