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

On Fri, 29 Nov 2024 at 15:49, vignesh C <vignesh21@gmail.com> wrote:
>
> 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;
>

Thanks for reviewing the patch. I have fixed the issue and updated the patch.

Thanks and Regards,
Shlok Kyal