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

On Friday, November 8, 2024 7:06 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> 
> Hi Amit,
> 
> On Thu, 7 Nov 2024 at 11:37, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Nov 5, 2024 at 12:53 PM Shlok Kyal <shlok.kyal.oss@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.

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.

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 ?

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.

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.

Best Regards,
Hou zj