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

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;

Regards,
Vignesh