Thanks for providing the comments. On Tue, 12 Nov 2024 at 12:52, Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com> wrote: > > On Friday, November 8, 2024 7:06 PM Shlok Kyal <shlok.kyal....@gmail.com> > wrote: > > > > Hi Amit, > > > > On Thu, 7 Nov 2024 at 11:37, Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Tue, Nov 5, 2024 at 12:53 PM Shlok Kyal <shlok.kyal....@gmail.com> > > wrote: > > > > > > > > To avoid the issue, we can disallow UPDATE/DELETE on table with > > > > unpublished generated column as REPLICA IDENTITY. I have attached a > > > > patch for the same. > > > > > > > > > > +CREATE PUBLICATION pub_gencol FOR TABLE testpub_gencol; UPDATE > > > +testpub_gencol SET a = 100 WHERE a = 1; > > > +ERROR: cannot update table "testpub_gencol" > > > +DETAIL: Column list used by the publication does not cover the > > > replica identity. > > > > > > This is not a correct ERROR message as the publication doesn't have > > > any column list associated with it. You have added the code to detect > > > this in the column list code path which I think is not required. BTW, > > > you also need to consider the latest commit 7054186c4e for this. I > > > guess you need to keep another flag in PublicationDesc to detect this > > > and then give an appropriate ERROR. > > > > I have addressed the comments and provided an updated patch. Also, I am > > currently working to fix this issue in back branches. > > Thanks for the patch. I am reviewing it and have some initial comments: > > > 1. > + char attgenerated = get_attgenerated(relid, attnum); > + > > I think it's unnecessary to initialize attgenerated here because the value > will > be overwritten if pubviaroot is true anyway. Also, the get_attgenerated() > is not cheap. > Fixed
> 2.
>
> I think the patch missed to check the case when table is marked REPLICA
> IDENTITY FULL, and generated column is not published:
>
> CREATE TABLE testpub_gencol (a INT, b INT GENERATED ALWAYS AS (a + 1) STORED
> NOT NULL);
> ALTER TABLE testpub_gencol REPLICA IDENTITY FULL;
> CREATE PUBLICATION pub_gencol FOR TABLE testpub_gencol;
> UPDATE testpub_gencol SET a = 2;
>
> I expected the UPDATE to fail in above case, but it can still pass after
> applying the patch.
>
Fixed
> 3.
>
> + * If the publication is FOR ALL TABLES we can skip the
> validation.
> + */
>
> This comment seems not clear to me, could you elaborate a bit more on this ?
>
I missed to handle the case FOR ALL TABLES. Have removed the comment.
> 4.
>
> Also, I think the patch does not handle the FOR ALL TABLE case correctly:
>
> 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 ALL TABLEs;
> UPDATE testpub_gencol SET a = 2;
>
> I expected the UPDATE to fail in above case as well.
>
Fixed
> 5.
>
> + else if (cmd == CMD_UPDATE && !pubdesc.replident_has_valid_gen_cols)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
> + errmsg("cannot update table \"%s\"",
> + RelationGetRelationName(rel)),
> + errdetail("REPLICA IDENTITY consists of an
> unpublished generated column.")));
>
> I think it would be better to use lower case "replica identity" to consistent
> with other existing messages.
>
Fixed
I have attached the updated patch here.
Thanks and Regards,
Shlok Kyal
v3-0001-Disallow-UPDATE-DELETE-on-table-with-generated-co.patch
Description: Binary data
