From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
cc: andreas@visena.com, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: ERROR: attribute number 6 exceeds number of columns 5
In-reply-to: <18651.1578415999@sss.pgh.pa.us>
References: <VisenaEmail.200.231b0a41523275d0.16ea7f800c7@tc7-visena>
 <6352.1574783351@sss.pgh.pa.us>
 <20191127.120034.20463388950832898.horikyota.ntt@gmail.com>
 <18651.1578415999@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 07 Jan 2020 11:53:19 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <13762.1578419161.0@sss.pgh.pa.us>
Date: Tue, 07 Jan 2020 12:47:00 -0500
Message-ID: <13814.1578419220@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <13762.1578419161.1@sss.pgh.pa.us>

I wrote:
> So I believe that the real problem here is that the executor is
> evaluating GENERATED expressions at the wrong time.  It's evaluating
> them against the pre-conversion tuples when it should be evaluating
> them against the post-conversion tuples.  We need to go fix that,
> rather than inserting arbitrary restrictions in the DDL code.

I looked at that more closely, and realized that blaming the executor
is wrong: the real issue is that ALTER TABLE itself supposes that it
need only evaluate expressions against the old tuple.  That's easy
to fix with a bit more code though.  I propose the attached.

(Note that this should also allow relaxing the existing implementation
restriction against changing types of columns that GENERATED columns
depend on: all we have to do is re-parse the generation expression
and schedule it for evaluation.  I've not looked into that, and it
doesn't seem like a bug fix anyway.)

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-alter-type-for-generated-columns.patch";
	charset="us-ascii"
Content-ID: <13762.1578419161.2@sss.pgh.pa.us>
Content-Description: fix-alter-type-for-generated-columns.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/table=
cmds.c
index 1c4394a..421bc28 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -192,13 +192,15 @@ typedef struct NewConstraint
  * Phase 3 copy (this could be either a new column with a non-null defaul=
t, or
  * a column that we're changing the type of).  Columns without such an en=
try
  * are just copied from the old table during ATRewriteTable.  Note that t=
he
- * expr is an expression over *old* table values.
+ * expr is an expression over *old* table values, except when is_generate=
d
+ * is true; then it is an expression over columns of the *new* tuple.
  */
 typedef struct NewColumnValue
 {
 	AttrNumber	attnum;			/* which column */
 	Expr	   *expr;			/* expression to compute */
 	ExprState  *exprstate;		/* execution state */
+	bool		is_generated;	/* is it a GENERATED expression? */
 } NewColumnValue;
 =

 /*
@@ -4961,7 +4963,9 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap=
, LOCKMODE lockmode)
 =

 				/*
 				 * Process supplied expressions to replace selected columns.
-				 * Expression inputs come from the old tuple.
+				 *
+				 * First, evaluate expressions whose inputs come from the old
+				 * tuple.
 				 */
 				econtext->ecxt_scantuple =3D oldslot;
 =

@@ -4969,6 +4973,9 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap=
, LOCKMODE lockmode)
 				{
 					NewColumnValue *ex =3D lfirst(l);
 =

+					if (ex->is_generated)
+						continue;
+
 					newslot->tts_values[ex->attnum - 1]
 						=3D ExecEvalExpr(ex->exprstate,
 									   econtext,
@@ -4978,6 +4985,26 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHea=
p, LOCKMODE lockmode)
 				ExecStoreVirtualTuple(newslot);
 =

 				/*
+				 * Now, evaluate any expressions whose inputs come from the
+				 * new tuple.  We assume these columns won't reference each
+				 * other, so that there's no ordering dependency.
+				 */
+				econtext->ecxt_scantuple =3D newslot;
+
+				foreach(l, tab->newvals)
+				{
+					NewColumnValue *ex =3D lfirst(l);
+
+					if (!ex->is_generated)
+						continue;
+
+					newslot->tts_values[ex->attnum - 1]
+						=3D ExecEvalExpr(ex->exprstate,
+									   econtext,
+									   &newslot->tts_isnull[ex->attnum - 1]);
+				}
+
+				/*
 				 * Constraints might reference the tableoid column, so
 				 * initialize t_tableOid before evaluating them.
 				 */
@@ -5892,6 +5919,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab=
, Relation rel,
 			newval =3D (NewColumnValue *) palloc0(sizeof(NewColumnValue));
 			newval->attnum =3D attribute.attnum;
 			newval->expr =3D expression_planner(defval);
+			newval->is_generated =3D (colDef->generated !=3D '\0');
 =

 			tab->newvals =3D lappend(tab->newvals, newval);
 		}
@@ -10379,6 +10407,7 @@ ATPrepAlterColumnType(List **wqueue,
 		newval =3D (NewColumnValue *) palloc0(sizeof(NewColumnValue));
 		newval->attnum =3D attnum;
 		newval->expr =3D (Expr *) transform;
+		newval->is_generated =3D false;
 =

 		tab->newvals =3D lappend(tab->newvals, newval);
 		if (ATColumnChangeRequiresRewrite(transform, attnum))
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/ex=
pected/generated.out
index f62c93f..3ef10fa 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -593,6 +593,31 @@ ERROR:  cannot use generated column "b" in column gen=
eration expression
 DETAIL:  A generated column cannot reference another generated column.
 ALTER TABLE gtest25 ADD COLUMN x int GENERATED ALWAYS AS (z * 4) STORED; =
 -- error
 ERROR:  column "z" does not exist
+ALTER TABLE gtest25 ADD COLUMN c int DEFAULT 42,
+  ADD COLUMN x int GENERATED ALWAYS AS (c * 4) STORED;
+ALTER TABLE gtest25 ADD COLUMN d int DEFAULT 101;
+ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
+  ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) STORED;
+SELECT * FROM gtest25 ORDER BY a;
+ a | b  | c  |  x  |  d  |  y  =

+---+----+----+-----+-----+-----
+ 3 |  9 | 42 | 168 | 101 | 404
+ 4 | 12 | 42 | 168 | 101 | 404
+(2 rows)
+
+\d gtest25
+                                         Table "public.gtest25"
+ Column |       Type       | Collation | Nullable |                      =
 Default                        =

+--------+------------------+-----------+----------+----------------------=
--------------------------------
+ a      | integer          |           | not null | =

+ b      | integer          |           |          | generated always as (=
a * 3) stored
+ c      | integer          |           |          | 42
+ x      | integer          |           |          | generated always as (=
c * 4) stored
+ d      | double precision |           |          | 101
+ y      | double precision |           |          | generated always as (=
d * 4::double precision) stored
+Indexes:
+    "gtest25_pkey" PRIMARY KEY, btree (a)
+
 -- ALTER TABLE ... ALTER COLUMN
 CREATE TABLE gtest27 (
     a int,
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/gen=
erated.sql
index 6a56ae2..f75d760 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -316,6 +316,13 @@ ALTER TABLE gtest25 ADD COLUMN b int GENERATED ALWAYS=
 AS (a * 3) STORED;
 SELECT * FROM gtest25 ORDER BY a;
 ALTER TABLE gtest25 ADD COLUMN x int GENERATED ALWAYS AS (b * 4) STORED; =
 -- error
 ALTER TABLE gtest25 ADD COLUMN x int GENERATED ALWAYS AS (z * 4) STORED; =
 -- error
+ALTER TABLE gtest25 ADD COLUMN c int DEFAULT 42,
+  ADD COLUMN x int GENERATED ALWAYS AS (c * 4) STORED;
+ALTER TABLE gtest25 ADD COLUMN d int DEFAULT 101;
+ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
+  ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) STORED;
+SELECT * FROM gtest25 ORDER BY a;
+\d gtest25
 =

 -- ALTER TABLE ... ALTER COLUMN
 CREATE TABLE gtest27 (

------- =_aaaaaaaaaa0--