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

On Fri, 29 Nov 2024 at 13:38, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 28 Nov 2024 at 16:38, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Nov 21, 2024 at 5:30 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >
> >
> > Review comments:
> > ===============
> > 1.
> > +
> > + /*
> > + * true if all generated columns which are part of replica identity are
> > + * published or the publication actions do not include UPDATE or DELETE.
> > + */
> > + bool replident_valid_for_update;
> > + bool replident_valid_for_delete;
> >
> > These are too generic names for the purpose they are used. How about
> > instead name them as gencols_valid_for_update and
> > gencols_valid_for_delete?
> >
> > 2. The comments atop RelationBuildPublicationDesc() is only about row
> > filter. We should update it for column list and generated columns as
> > well.
> >
> > 3. It is better to merge the functionality of the invalid column list
> > and unpublished generated columns as proposed by Hou-San above.
> >
>
> Thanks for reviewing the patch. I have addressed the comments and
> updated the patch.

Shouldn't unpublished_gen_col be set only if the column list is absent?
-               /* Transform the column list datum to a bitmapset. */
-               columns = pub_collist_to_bitmapset(NULL, datum, NULL);
+               /* Check if generated column is part of REPLICA IDENTITY */
+               *unpublished_gen_col |= att->attgenerated;

-               /* Remember columns that are part of the REPLICA IDENTITY */
-               idattrs = RelationGetIndexAttrBitmap(relation,
-
                  INDEX_ATTR_BITMAP_IDENTITY_KEY);
+               if (columns == NULL)
+               {
+                       /* Break the loop if unpublished generated
columns exist. */
+                       if (*unpublished_gen_col)
+                               break;
+
+                       /* Skip validating the column list since it is
not defined */
+                       continue;
+               }


This scenario worked in v11 but fails in v12:
CREATE TABLE testpub_gencol (a INT, b INT GENERATED ALWAYS AS (a + 1)
STORED NOT NULL);
CREATE UNIQUE INDEX testpub_gencol_idx ON testpub_gencol (b);
ALTER TABLE testpub_gencol REPLICA IDENTITY USING index testpub_gencol_idx;
CREATE PUBLICATION pub_gencol FOR TABLE testpub_gencol(b);
UPDATE testpub_gencol SET a = 100 WHERE a = 1;

Regards,
Vignesh