Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ljkz9-0005pe-8Q for pgsql-bugs@arkaria.postgresql.org; Thu, 20 May 2021 15:53:19 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ljkz8-0001cF-71 for pgsql-bugs@arkaria.postgresql.org; Thu, 20 May 2021 15:53:18 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ljkz7-0001c3-SX for pgsql-bugs@lists.postgresql.org; Thu, 20 May 2021 15:53:18 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ljkz5-0003e4-HL for pgsql-bugs@lists.postgresql.org; Thu, 20 May 2021 15:53:16 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 14KFrCfo3689917; Thu, 20 May 2021 11:53:12 -0400 From: Tom Lane To: Vitaly Ustinov cc: Peter Eisentraut , pgsql-bugs@lists.postgresql.org, "David G. Johnston" Subject: Re: Generated column is not updated (Postgres 13) In-reply-to: References: <3513055.1621461801@sss.pgh.pa.us> <3569485.1621475748@sss.pgh.pa.us> Comments: In-reply-to Vitaly Ustinov message dated "Thu, 20 May 2021 09:51:22 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <3689820.1621525839.0@sss.pgh.pa.us> Date: Thu, 20 May 2021 11:53:12 -0400 Message-ID: <3689916.1621525992@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <3689820.1621525839.1@sss.pgh.pa.us> After looking at this further, I see that there already is a check for system columns in GENERATED expressions; it's just in the parser (scanNSItemForColumn), not where I was looking for it. And it explicitly exempts tableoid. I continue to think that that's a bad idea and we're gonna regret it, but at least the issue in ATRewriteTable turns out to be trivial to fix. So I did that and upgraded the test scenario to be a bit more realistic, in 0001 below. 0002 then just disallows whole-row variables. (I added an errdetail trying to explain that restriction, too.) It's worth pointing out here that what seems to me to be a "more realistic" test scenario involves a regclass constant for the table's own OID. If that doesn't scare you, it should, because it implies all kinds of constraints on the order in which these expressions are processed during CREATE or ALTER TABLE. The test passes check-world, which includes dump/reload, but I am sorely afraid that there are ways in which this sort of thing would fail. Do we *really* want to buy into supporting it? regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="0001-fix-tableoid-in-ATRewriteTable.patch"; charset="us-ascii" Content-ID: <3689820.1621525839.2@sss.pgh.pa.us> Content-Description: 0001-fix-tableoid-in-ATRewriteTable.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/table= cmds.c index ebc62034d2..85dcc33063 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5761,6 +5761,14 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHea= p, LOCKMODE lockmode) foreach(lc, dropped_attrs) newslot->tts_isnull[lfirst_int(lc)] =3D true; = + /* + * Constraints and GENERATED expressions might reference the + * tableoid column, so fill tts_tableOid with the desired + * value. (We must do this each time, because it gets + * overwritten with newrel's OID during storing.) + */ + newslot->tts_tableOid =3D RelationGetRelid(oldrel); + /* * Process supplied expressions to replace selected columns. * @@ -5804,11 +5812,6 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHea= p, LOCKMODE lockmode) &newslot->tts_isnull[ex->attnum - 1]); } = - /* - * Constraints might reference the tableoid column, so - * initialize t_tableOid before evaluating them. - */ - newslot->tts_tableOid =3D RelationGetRelid(oldrel); insertslot =3D newslot; } else diff --git a/src/test/regress/expected/generated.out b/src/test/regress/ex= pected/generated.out index 675773f0c1..71542e95d5 100644 --- a/src/test/regress/expected/generated.out +++ b/src/test/regress/expected/generated.out @@ -64,6 +64,7 @@ ERROR: both identity and generation expression specifie= d for column "b" of tabl LINE 1: ...t PRIMARY KEY, b int GENERATED ALWAYS AS identity GENERATED ..= . ^ -- reference to system column not allowed in generated column +-- (except tableoid, which we test below) CREATE TABLE gtest_err_6a (a int PRIMARY KEY, b bool GENERATED ALWAYS AS = (xmin <> 37) STORED); ERROR: cannot use system column "xmin" in column generation expression LINE 1: ...a (a int PRIMARY KEY, b bool GENERATED ALWAYS AS (xmin <> 37..= . @@ -455,14 +456,16 @@ DROP TYPE double_int; -- using tableoid is allowed CREATE TABLE gtest_tableoid ( a int PRIMARY KEY, - b bool GENERATED ALWAYS AS (tableoid <> 0) STORED + b bool GENERATED ALWAYS AS (tableoid =3D 'gtest_tableoid'::regclass) ST= ORED ); INSERT INTO gtest_tableoid VALUES (1), (2); +ALTER TABLE gtest_tableoid ADD COLUMN + c regclass GENERATED ALWAYS AS (tableoid) STORED; SELECT * FROM gtest_tableoid; - a | b = ----+--- - 1 | t - 2 | t + a | b | c = +---+---+---------------- + 1 | t | gtest_tableoid + 2 | t | gtest_tableoid (2 rows) = -- drop column behavior diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/gen= erated.sql index 63251c443a..914197608b 100644 --- a/src/test/regress/sql/generated.sql +++ b/src/test/regress/sql/generated.sql @@ -29,6 +29,7 @@ CREATE TABLE gtest_err_5a (a int PRIMARY KEY, b int DEFA= ULT 5 GENERATED ALWAYS A CREATE TABLE gtest_err_5b (a int PRIMARY KEY, b int GENERATED ALWAYS AS i= dentity GENERATED ALWAYS AS (a * 2) STORED); = -- reference to system column not allowed in generated column +-- (except tableoid, which we test below) CREATE TABLE gtest_err_6a (a int PRIMARY KEY, b bool GENERATED ALWAYS AS = (xmin <> 37) STORED); = -- various prohibited constructs @@ -218,9 +219,11 @@ DROP TYPE double_int; -- using tableoid is allowed CREATE TABLE gtest_tableoid ( a int PRIMARY KEY, - b bool GENERATED ALWAYS AS (tableoid <> 0) STORED + b bool GENERATED ALWAYS AS (tableoid =3D 'gtest_tableoid'::regclass) ST= ORED ); INSERT INTO gtest_tableoid VALUES (1), (2); +ALTER TABLE gtest_tableoid ADD COLUMN + c regclass GENERATED ALWAYS AS (tableoid) STORED; SELECT * FROM gtest_tableoid; = -- drop column behavior ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="0002-forbid-whole-row-in-generated.patch"; charset="us-ascii" Content-ID: <3689820.1621525839.3@sss.pgh.pa.us> Content-Description: 0002-forbid-whole-row-in-generated.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 431e62e389..ba3e1ecf45 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -3020,15 +3020,26 @@ check_nested_generated_walker(Node *node, void *co= ntext) AttrNumber attnum; = relid =3D rt_fetch(var->varno, pstate->p_rtable)->relid; + if (!OidIsValid(relid)) + return false; /* XXX shouldn't we raise an error? */ + attnum =3D var->varattno; = - if (OidIsValid(relid) && AttributeNumberIsValid(attnum) && get_attgener= ated(relid, attnum)) + if (attnum > 0 && get_attgenerated(relid, attnum)) ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("cannot use generated column \"%s\" in column generation exp= ression", get_attname(relid, attnum, false)), errdetail("A generated column cannot reference another generated co= lumn."), parser_errposition(pstate, var->location))); + /* A whole-row Var is necessarily self-referential, so forbid it */ + if (attnum =3D=3D 0) + ereport(ERROR, + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("cannot use whole-row variable in column generation expressi= on"), + errdetail("This would cause the generated column to depend on its o= wn value."), + parser_errposition(pstate, var->location))); + /* System columns were already checked in the parser */ = return false; } diff --git a/src/test/regress/expected/generated.out b/src/test/regress/ex= pected/generated.out index 71542e95d5..c2e5676196 100644 --- a/src/test/regress/expected/generated.out +++ b/src/test/regress/expected/generated.out @@ -46,6 +46,13 @@ ERROR: cannot use generated column "b" in column gener= ation expression LINE 1: ...AYS AS (a * 2) STORED, c int GENERATED ALWAYS AS (b * 3) STO..= . ^ DETAIL: A generated column cannot reference another generated column. +-- a whole-row var is a self-reference on steroids, so disallow that too +CREATE TABLE gtest_err_2c (a int PRIMARY KEY, + b int GENERATED ALWAYS AS (num_nulls(gtest_err_2c)) STORED); +ERROR: cannot use whole-row variable in column generation expression +LINE 2: b int GENERATED ALWAYS AS (num_nulls(gtest_err_2c)) STOR... + ^ +DETAIL: This would cause the generated column to depend on its own value= . -- invalid reference CREATE TABLE gtest_err_3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (c= * 2) STORED); ERROR: column "c" does not exist diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/gen= erated.sql index 914197608b..b7eb072671 100644 --- a/src/test/regress/sql/generated.sql +++ b/src/test/regress/sql/generated.sql @@ -17,6 +17,9 @@ CREATE TABLE gtest_err_1 (a int PRIMARY KEY, b int GENER= ATED ALWAYS AS (a * 2) S -- references to other generated columns, including self-references CREATE TABLE gtest_err_2a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (= b * 2) STORED); CREATE TABLE gtest_err_2b (a int PRIMARY KEY, b int GENERATED ALWAYS AS (= a * 2) STORED, c int GENERATED ALWAYS AS (b * 3) STORED); +-- a whole-row var is a self-reference on steroids, so disallow that too +CREATE TABLE gtest_err_2c (a int PRIMARY KEY, + b int GENERATED ALWAYS AS (num_nulls(gtest_err_2c)) STORED); = -- invalid reference CREATE TABLE gtest_err_3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (c= * 2) STORED); ------- =_aaaaaaaaaa0--