bug in stored generated column over domain with constraints.

Started by jian he11 months ago6 messages
#1jian he
jian.universality@gmail.com
1 attachment(s)

hi.

create domain d1 as int not null;
create domain d2 as int check (value > 1);
create domain d3 as int check (value is not null);

create table t0(b int, a d3 GENERATED ALWAYS as (b + 11) stored);
insert into t0 values (1, default);
ERROR: value for domain d3 violates check constraint "d3_check"

in the above example, a domain with check constraint not working as intended,
similarly, a domain with not-null constraint will also not work.
now table t0 can not insert any data, because of check constraint violation.
so i think this is a bug, (hope i didn't miss anything).

in ExecBuildProjectionInfo, we compile "values (1, default)" as
targetlist expression (CONST, COERCETODOMAIN)
Then in ExecResult, ExecProject, ExecInterpExpr we evaluate the
compiled expression;
we failed at ExecEvalConstraintCheck. we are acting like: ``null::d3``.

explain(costs off, verbose) insert into t0 values (1, default);
QUERY PLAN
----------------------------------------
Insert on public.t0
-> Result
Output: 1, NULL::integer

the plan is ``NULL::integer``, which will not fail, but ``NULL::d3`` will fail.
that means, the output plan is right. it's the execution wrong?

the main fix should be in rewriteTargetListIU.
UPDATE don't have this issue, since there is no Result node,
ExecComputeStoredGenerated will do the domain constraint check.

related:
https://git.postgresql.org/cgit/postgresql.git/commit/?id=0da39aa7667b06e16189d318f7850d559d446d52

Attachments:

v1-0001-fix-default-insertion-for-stored-generated-column.patchtext/x-patch; charset=US-ASCII; name=v1-0001-fix-default-insertion-for-stored-generated-column.patchDownload
From 1820d039b472ea71d3b80b98096a81bb5fc3857b Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Wed, 26 Feb 2025 01:05:46 +0800
Subject: [PATCH v1 1/1] fix default insertion for stored generated column with
 domain over constraints.

create domain d3 as int check (value is not null);
create table t0(b int, a d3 GENERATED ALWAYS as (b + 11) stored);

insert into t0 values (1, default);
ERROR:  value for domain d3 violates check constraint "d3_check"

explain(costs off, verbose) insert into t0 values (1, default);
              QUERY PLAN
---------------------------------------
 Insert on public.t0
   ->  Result
         Output: 1, NULL::integer

We first evaluate the Result node. Domain coercion in the Result node may lead
to domain constraint violations. These domain constraints should not be checked in
ExecResult, as ExecComputeStoredGenerated will handle them.

context: https://git.postgresql.org/cgit/postgresql.git/commit/?id=0da39aa7667b06e16189d318f7850d559d446d52
discussion: https://postgr.es/m/
---
 src/backend/executor/nodeModifyTable.c        | 31 +++++++++++++++---
 src/backend/optimizer/prep/preptlist.c        |  3 +-
 src/backend/parser/parse_coerce.c             |  5 +--
 src/backend/rewrite/rewriteHandler.c          | 23 +++++++++++--
 src/backend/rewrite/rewriteManip.c            |  3 +-
 src/include/parser/parse_coerce.h             |  2 +-
 .../regress/expected/generated_stored.out     | 32 +++++++++++++++++++
 src/test/regress/sql/generated_stored.sql     | 15 +++++++++
 8 files changed, 102 insertions(+), 12 deletions(-)

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index b0fe50075ad..1db7c1e45b6 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -67,6 +67,7 @@
 #include "storage/lmgr.h"
 #include "utils/builtins.h"
 #include "utils/datum.h"
+#include "utils/lsyscache.h"
 #include "utils/rel.h"
 #include "utils/snapmgr.h"
 
@@ -216,13 +217,33 @@ ExecCheckPlanOutput(Relation resultRel, List *targetList)
 		{
 			/* Normal case: demand type match */
 			if (exprType((Node *) tle->expr) != attr->atttypid)
-				ereport(ERROR,
-						(errcode(ERRCODE_DATATYPE_MISMATCH),
-						 errmsg("table row type and query-specified row type do not match"),
-						 errdetail("Table has type %s at ordinal position %d, but query expects %s.",
+			{
+				if (attr->attgenerated == '\0')
+				{
+					ereport(ERROR,
+							errcode(ERRCODE_DATATYPE_MISMATCH),
+							errmsg("table row type and query-specified row type do not match"),
+							errdetail("Table has type %s at ordinal position %d, but query expects %s.",
+									format_type_be(attr->atttypid),
+									attno,
+									format_type_be(exprType((Node *) tle->expr))));
+				}
+				else
+				{
+					/*XXX explain why this is needed */
+					Oid			baseTypeId;
+					int32		baseTypeMod = attr->atttypmod;
+					baseTypeId = getBaseTypeAndTypmod(attr->atttypid, &baseTypeMod);
+
+					if (exprType((Node *) tle->expr) != baseTypeId)
+						errcode(ERRCODE_DATATYPE_MISMATCH),
+						errmsg("table row type and query-specified row type do not match"),
+						errdetail("Table has domain type %s at ordinal position %d, but query expects %s.",
 								   format_type_be(attr->atttypid),
 								   attno,
-								   format_type_be(exprType((Node *) tle->expr)))));
+								   format_type_be(exprType((Node *) tle->expr)));
+				}
+			}
 		}
 		else
 		{
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index c2a77503d4b..769ff600eaf 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -440,7 +440,8 @@ expand_insert_targetlist(PlannerInfo *root, List *tlist, Relation rel)
 												 att_tup->atttypmod,
 												 att_tup->attcollation,
 												 att_tup->attlen,
-												 att_tup->attbyval);
+												 att_tup->attbyval,
+												 true);
 				/* Must run expression preprocessing on any non-const nodes */
 				if (!IsA(new_expr, Const))
 					new_expr = eval_const_expressions(root, new_expr);
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 0b5b81c7f27..7c10dbdb424 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -1266,10 +1266,11 @@ coerce_to_specific_type(ParseState *pstate, Node *node,
  *		Build a NULL constant, then wrap it in CoerceToDomain
  *		if the desired type is a domain type.  This allows any
  *		NOT NULL domain constraint to be enforced at runtime.
+ *		XXX explain need_coerce, maybe change to a better name.
  */
 Node *
 coerce_null_to_domain(Oid typid, int32 typmod, Oid collation,
-					  int typlen, bool typbyval)
+					  int typlen, bool typbyval, bool need_coerce)
 {
 	Node	   *result;
 	Oid			baseTypeId;
@@ -1287,7 +1288,7 @@ coerce_null_to_domain(Oid typid, int32 typmod, Oid collation,
 								(Datum) 0,
 								true,	/* isnull */
 								typbyval);
-	if (typid != baseTypeId)
+	if (typid != baseTypeId && need_coerce)
 		result = coerce_to_domain(result,
 								  baseTypeId, baseTypeMod,
 								  typid,
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index f0bce5f9ed9..a0481ad6000 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -990,8 +990,25 @@ rewriteTargetListIU(List *targetList,
 			/*
 			 * virtual generated column stores a null value; stored generated
 			 * column will be fixed in executor
+			 * XXX comments explain why we need special deal with INSERT
 			 */
 			new_tle = NULL;
+			if (apply_default && commandType == CMD_INSERT)
+			{
+				Node	   *new_expr;
+
+				new_expr = coerce_null_to_domain(att_tup->atttypid,
+												 att_tup->atttypmod,
+												 att_tup->attcollation,
+												 att_tup->attlen,
+												 att_tup->attbyval,
+												 att_tup->attgenerated == '\0');
+				if (new_expr)
+					new_tle = makeTargetEntry((Expr *) new_expr,
+											  attrno,
+											  pstrdup(NameStr(att_tup->attname)),
+											  false);
+			}
 		}
 		else if (apply_default)
 		{
@@ -1015,7 +1032,8 @@ rewriteTargetListIU(List *targetList,
 													 att_tup->atttypmod,
 													 att_tup->attcollation,
 													 att_tup->attlen,
-													 att_tup->attbyval);
+													 att_tup->attbyval,
+													 true);
 			}
 
 			if (new_expr)
@@ -1567,7 +1585,8 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
 													 att_tup->atttypmod,
 													 att_tup->attcollation,
 													 att_tup->attlen,
-													 att_tup->attbyval);
+													 att_tup->attbyval,
+													 true);
 				}
 				newList = lappend(newList, new_expr);
 			}
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 98a265cd3d5..09d54d39632 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1942,7 +1942,8 @@ ReplaceVarFromTargetList(Var *var,
 												 var->vartypmod,
 												 var->varcollid,
 												 vartyplen,
-												 vartypbyval);
+												 vartypbyval,
+												 true);
 				}
 		}
 		elog(ERROR, "could not find replacement targetlist entry for attno %d",
diff --git a/src/include/parser/parse_coerce.h b/src/include/parser/parse_coerce.h
index 8d775c72c59..1bb19c82e1f 100644
--- a/src/include/parser/parse_coerce.h
+++ b/src/include/parser/parse_coerce.h
@@ -64,7 +64,7 @@ extern Node *coerce_to_specific_type_typmod(ParseState *pstate, Node *node,
 											const char *constructName);
 
 extern Node *coerce_null_to_domain(Oid typid, int32 typmod, Oid collation,
-								   int typlen, bool typbyval);
+								   int typlen, bool typbyval, bool need_coerce);
 
 extern int	parser_coercion_errposition(ParseState *pstate,
 										int coerce_location,
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index 3ce0dd1831c..e1b436a2c02 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -843,6 +843,38 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 INSERT INTO gtest24r (a) VALUES (4);  -- ok
 INSERT INTO gtest24r (a) VALUES (6);  -- error
 ERROR:  value for domain gtestdomain1 violates check constraint "gtestdomain1_check"
+CREATE DOMAIN dnn AS int NOT NULL;
+CREATE DOMAIN dnn_check AS int CHECK (value IS NOT NULL);
+CREATE TABLE gtest24nn (
+    a int,
+    b dnn GENERATED ALWAYS AS (a + 1) STORED,
+    c dnn GENERATED ALWAYS AS (11) STORED,
+    d dnn_check GENERATED ALWAYS AS (a + 11) STORED
+);
+EXPLAIN (COSTS OFF, VERBOSE) INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT); --ok
+                           QUERY PLAN                           
+----------------------------------------------------------------
+ Insert on generated_stored_tests.gtest24nn
+   ->  Result
+         Output: 1, NULL::integer, NULL::integer, NULL::integer
+(3 rows)
+
+INSERT INTO gtest24nn VALUES (NULL, DEFAULT, DEFAULT, DEFAULT); --error
+ERROR:  domain dnn does not allow null values
+INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT) RETURNING *; --ok
+ a | b | c  | d  
+---+---+----+----
+ 1 | 2 | 11 | 12
+(1 row)
+
+UPDATE gtest24nn SET b = DEFAULT, c = default, d = default, a = 2 WHERE a = 1 RETURNING *; --ok
+ a | b | c  | d  
+---+---+----+----
+ 2 | 3 | 11 | 13
+(1 row)
+
+UPDATE gtest24nn SET a = NULL WHERE a = 2 RETURNING *; --error
+ERROR:  domain dnn does not allow null values
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) STORED);
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index b7749ce355f..f7988a74acd 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -416,6 +416,21 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 INSERT INTO gtest24r (a) VALUES (4);  -- ok
 INSERT INTO gtest24r (a) VALUES (6);  -- error
 
+CREATE DOMAIN dnn AS int NOT NULL;
+CREATE DOMAIN dnn_check AS int CHECK (value IS NOT NULL);
+CREATE TABLE gtest24nn (
+    a int,
+    b dnn GENERATED ALWAYS AS (a + 1) STORED,
+    c dnn GENERATED ALWAYS AS (11) STORED,
+    d dnn_check GENERATED ALWAYS AS (a + 11) STORED
+);
+
+EXPLAIN (COSTS OFF, VERBOSE) INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT); --ok
+INSERT INTO gtest24nn VALUES (NULL, DEFAULT, DEFAULT, DEFAULT); --error
+INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT) RETURNING *; --ok
+UPDATE gtest24nn SET b = DEFAULT, c = default, d = default, a = 2 WHERE a = 1 RETURNING *; --ok
+UPDATE gtest24nn SET a = NULL WHERE a = 2 RETURNING *; --error
+
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) STORED);
-- 
2.34.1

#2jian he
jian.universality@gmail.com
In reply to: jian he (#1)
1 attachment(s)
Re: bug in stored generated column over domain with constraints.

hi.

comments refined and minor aesthetic adjustments made.

Attachments:

v2-0001-fix-default-insertion-for-stored-generated-column.patchtext/x-patch; charset=US-ASCII; name=v2-0001-fix-default-insertion-for-stored-generated-column.patchDownload
From 22ef7ce384de3098fc19ae0bb9bc9777b269b8ec Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Wed, 26 Feb 2025 11:29:04 +0800
Subject: [PATCH v2 1/1] fix default insertion for stored generated column with
 domain over constraints.

create domain d3 as int check (value is not null);
create table t0(b int, a d3 GENERATED ALWAYS as (b + 11) stored);

insert into t0 values (1, default);
ERROR:  value for domain d3 violates check constraint "d3_check"

explain(costs off, verbose) insert into t0 values (1, default);
              QUERY PLAN
---------------------------------------
 Insert on public.t0
   ->  Result
         Output: 1, NULL::integer

We first evaluate the Result node. Domain coercion in the Result node may lead
to domain constraint violations. These domain constraints should not be checked in
ExecResult, as ExecComputeStoredGenerated will handle them.

context: https://git.postgresql.org/cgit/postgresql.git/commit/?id=0da39aa7667b06e16189d318f7850d559d446d52
discussion: https://postgr.es/m/CACJufxG59tip2+9h=rEv-ykOFjt0cbsPVchhi0RTij8bABBA0Q@mail.gmail.com
---
 src/backend/executor/nodeModifyTable.c        | 36 +++++++++++++----
 src/backend/optimizer/prep/preptlist.c        |  3 +-
 src/backend/parser/parse_coerce.c             |  5 ++-
 src/backend/rewrite/rewriteHandler.c          | 39 ++++++++++++++++---
 src/backend/rewrite/rewriteManip.c            |  3 +-
 src/include/parser/parse_coerce.h             |  2 +-
 .../regress/expected/generated_stored.out     | 32 +++++++++++++++
 src/test/regress/sql/generated_stored.sql     | 15 +++++++
 8 files changed, 118 insertions(+), 17 deletions(-)

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index b0fe50075ad..c467c735334 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -67,6 +67,7 @@
 #include "storage/lmgr.h"
 #include "utils/builtins.h"
 #include "utils/datum.h"
+#include "utils/lsyscache.h"
 #include "utils/rel.h"
 #include "utils/snapmgr.h"
 
@@ -216,13 +217,34 @@ ExecCheckPlanOutput(Relation resultRel, List *targetList)
 		{
 			/* Normal case: demand type match */
 			if (exprType((Node *) tle->expr) != attr->atttypid)
-				ereport(ERROR,
-						(errcode(ERRCODE_DATATYPE_MISMATCH),
-						 errmsg("table row type and query-specified row type do not match"),
-						 errdetail("Table has type %s at ordinal position %d, but query expects %s.",
-								   format_type_be(attr->atttypid),
-								   attno,
-								   format_type_be(exprType((Node *) tle->expr)))));
+			{
+				if (attr->attgenerated == '\0')
+				{
+					ereport(ERROR,
+							errcode(ERRCODE_DATATYPE_MISMATCH),
+							errmsg("table row type and query-specified row type do not match"),
+							errdetail("Table has type %s at ordinal position %d, but query expects %s.",
+									   format_type_be(attr->atttypid),
+									   attno,
+									   format_type_be(exprType((Node *) tle->expr))));
+				}
+				else
+				{
+					/* see rewriteTargetListIU comments for domain over generated column */
+					Oid			baseTypeId;
+					int32		baseTypeMod = attr->atttypmod;
+					baseTypeId = getBaseTypeAndTypmod(attr->atttypid, &baseTypeMod);
+
+					if (exprType((Node *) tle->expr) != baseTypeId)
+						ereport(ERROR,
+								errcode(ERRCODE_DATATYPE_MISMATCH),
+								errmsg("table row type and query-specified row type do not match"),
+								errdetail("Table has type %s at ordinal position %d, but query expects %s.",
+										   format_type_be(attr->atttypid),
+										   attno,
+										   format_type_be(exprType((Node *) tle->expr))));
+				}
+			}
 		}
 		else
 		{
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index c2a77503d4b..769ff600eaf 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -440,7 +440,8 @@ expand_insert_targetlist(PlannerInfo *root, List *tlist, Relation rel)
 												 att_tup->atttypmod,
 												 att_tup->attcollation,
 												 att_tup->attlen,
-												 att_tup->attbyval);
+												 att_tup->attbyval,
+												 true);
 				/* Must run expression preprocessing on any non-const nodes */
 				if (!IsA(new_expr, Const))
 					new_expr = eval_const_expressions(root, new_expr);
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 0b5b81c7f27..e2db53690b6 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -1266,10 +1266,11 @@ coerce_to_specific_type(ParseState *pstate, Node *node,
  *		Build a NULL constant, then wrap it in CoerceToDomain
  *		if the desired type is a domain type.  This allows any
  *		NOT NULL domain constraint to be enforced at runtime.
+ *		need_coerce is false means no need warp it in CoerceToDomain.
  */
 Node *
 coerce_null_to_domain(Oid typid, int32 typmod, Oid collation,
-					  int typlen, bool typbyval)
+					  int typlen, bool typbyval, bool need_coerce)
 {
 	Node	   *result;
 	Oid			baseTypeId;
@@ -1287,7 +1288,7 @@ coerce_null_to_domain(Oid typid, int32 typmod, Oid collation,
 								(Datum) 0,
 								true,	/* isnull */
 								typbyval);
-	if (typid != baseTypeId)
+	if (typid != baseTypeId && need_coerce)
 		result = coerce_to_domain(result,
 								  baseTypeId, baseTypeMod,
 								  typid,
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index f0bce5f9ed9..5964b1ecbe8 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -988,10 +988,37 @@ rewriteTargetListIU(List *targetList,
 		if (att_tup->attgenerated)
 		{
 			/*
-			 * virtual generated column stores a null value; stored generated
-			 * column will be fixed in executor
-			 */
+			 * We first project and evaluate tlist for the INSERT operation via
+			 * Result (variable-free) Node to produce the tuple to be inserted
+			 * by ExecInsert. tlist entries may contain generated column based
+			 * on domain type. we typically produce a CoerceToDomain Node for
+			 * those specific attributes, this generally is fine.  However if
+			 * that domain has a NOT NULL constraint or a check constraint
+			 * logically equivalent to a NOT NULL constraint, it would fail
+			 * earilier for domain constaint viotlation. All tlist entries
+			 * default to NULL for insert, and we cannot evaulte the generated
+			 * expression during ExecResult.
+			 *
+			 * so not produce CoerceToDomain if the attribute is generated column.
+			 * we will do the same work in ExecComputeStoredGenerated.
+			*/
 			new_tle = NULL;
+			if (apply_default && commandType == CMD_INSERT)
+			{
+				Node	   *new_expr;
+
+				new_expr = coerce_null_to_domain(att_tup->atttypid,
+												 att_tup->atttypmod,
+												 att_tup->attcollation,
+												 att_tup->attlen,
+												 att_tup->attbyval,
+												 att_tup->attgenerated == '\0');
+				if (new_expr)
+					new_tle = makeTargetEntry((Expr *) new_expr,
+											  attrno,
+											  pstrdup(NameStr(att_tup->attname)),
+											  false);
+			}
 		}
 		else if (apply_default)
 		{
@@ -1015,7 +1042,8 @@ rewriteTargetListIU(List *targetList,
 													 att_tup->atttypmod,
 													 att_tup->attcollation,
 													 att_tup->attlen,
-													 att_tup->attbyval);
+													 att_tup->attbyval,
+													 true);
 			}
 
 			if (new_expr)
@@ -1567,7 +1595,8 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
 													 att_tup->atttypmod,
 													 att_tup->attcollation,
 													 att_tup->attlen,
-													 att_tup->attbyval);
+													 att_tup->attbyval,
+													 true);
 				}
 				newList = lappend(newList, new_expr);
 			}
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 98a265cd3d5..09d54d39632 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1942,7 +1942,8 @@ ReplaceVarFromTargetList(Var *var,
 												 var->vartypmod,
 												 var->varcollid,
 												 vartyplen,
-												 vartypbyval);
+												 vartypbyval,
+												 true);
 				}
 		}
 		elog(ERROR, "could not find replacement targetlist entry for attno %d",
diff --git a/src/include/parser/parse_coerce.h b/src/include/parser/parse_coerce.h
index 8d775c72c59..1bb19c82e1f 100644
--- a/src/include/parser/parse_coerce.h
+++ b/src/include/parser/parse_coerce.h
@@ -64,7 +64,7 @@ extern Node *coerce_to_specific_type_typmod(ParseState *pstate, Node *node,
 											const char *constructName);
 
 extern Node *coerce_null_to_domain(Oid typid, int32 typmod, Oid collation,
-								   int typlen, bool typbyval);
+								   int typlen, bool typbyval, bool need_coerce);
 
 extern int	parser_coercion_errposition(ParseState *pstate,
 										int coerce_location,
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index 3ce0dd1831c..e1b436a2c02 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -843,6 +843,38 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 INSERT INTO gtest24r (a) VALUES (4);  -- ok
 INSERT INTO gtest24r (a) VALUES (6);  -- error
 ERROR:  value for domain gtestdomain1 violates check constraint "gtestdomain1_check"
+CREATE DOMAIN dnn AS int NOT NULL;
+CREATE DOMAIN dnn_check AS int CHECK (value IS NOT NULL);
+CREATE TABLE gtest24nn (
+    a int,
+    b dnn GENERATED ALWAYS AS (a + 1) STORED,
+    c dnn GENERATED ALWAYS AS (11) STORED,
+    d dnn_check GENERATED ALWAYS AS (a + 11) STORED
+);
+EXPLAIN (COSTS OFF, VERBOSE) INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT); --ok
+                           QUERY PLAN                           
+----------------------------------------------------------------
+ Insert on generated_stored_tests.gtest24nn
+   ->  Result
+         Output: 1, NULL::integer, NULL::integer, NULL::integer
+(3 rows)
+
+INSERT INTO gtest24nn VALUES (NULL, DEFAULT, DEFAULT, DEFAULT); --error
+ERROR:  domain dnn does not allow null values
+INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT) RETURNING *; --ok
+ a | b | c  | d  
+---+---+----+----
+ 1 | 2 | 11 | 12
+(1 row)
+
+UPDATE gtest24nn SET b = DEFAULT, c = default, d = default, a = 2 WHERE a = 1 RETURNING *; --ok
+ a | b | c  | d  
+---+---+----+----
+ 2 | 3 | 11 | 13
+(1 row)
+
+UPDATE gtest24nn SET a = NULL WHERE a = 2 RETURNING *; --error
+ERROR:  domain dnn does not allow null values
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) STORED);
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index b7749ce355f..f7988a74acd 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -416,6 +416,21 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 INSERT INTO gtest24r (a) VALUES (4);  -- ok
 INSERT INTO gtest24r (a) VALUES (6);  -- error
 
+CREATE DOMAIN dnn AS int NOT NULL;
+CREATE DOMAIN dnn_check AS int CHECK (value IS NOT NULL);
+CREATE TABLE gtest24nn (
+    a int,
+    b dnn GENERATED ALWAYS AS (a + 1) STORED,
+    c dnn GENERATED ALWAYS AS (11) STORED,
+    d dnn_check GENERATED ALWAYS AS (a + 11) STORED
+);
+
+EXPLAIN (COSTS OFF, VERBOSE) INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT); --ok
+INSERT INTO gtest24nn VALUES (NULL, DEFAULT, DEFAULT, DEFAULT); --error
+INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT) RETURNING *; --ok
+UPDATE gtest24nn SET b = DEFAULT, c = default, d = default, a = 2 WHERE a = 1 RETURNING *; --ok
+UPDATE gtest24nn SET a = NULL WHERE a = 2 RETURNING *; --error
+
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) STORED);
-- 
2.34.1

#3jian he
jian.universality@gmail.com
In reply to: jian he (#2)
1 attachment(s)
Re: bug in stored generated column over domain with constraints.

hi.

new patch attached.

rewriteTargetListIU, expand_insert_targetlist these two places can
make a null Const TargetEntry for the generated column in an INSERT
operation.

but since this problem only occurs in INSERT, so i placed the logic
within expand_insert_targetlist would be appropriate?

The following are excerpts of the commit message.
--------------------------------
create domain d3 as int check (value is not null);
create table t0(b int, a d3 GENERATED ALWAYS as (b + 11) stored);

insert into t0 values (1, default);
ERROR: value for domain d3 violates check constraint "d3_check"

explain(costs off, verbose) insert into t0 values (1, default);
QUERY PLAN
---------------------------------------
Insert on public.t0
-> Result
Output: 1, NULL::integer

For INSERT operation, for Query->targetList, we should not make a
generated column
over domain with constraint to a CoerceToDomain node, instead, we make it as a
simple null Const over domain's base type.

When a column is a generated column in an INSERT, expand_insert_targetlist
should unconditionally generate a null Const to be inserted. If we are not
doing this way, we might end up wrapping the null Const in a CoerceToDomain
node, which may trigger runtime error earlier if the domain has a NOT NULL
constraint. That's not fine, as generated columns are already handled in
ExecComputeStoredGenerated.
--------------------------------

Attachments:

v3-0001-fix-INSERT-generated-column-over-domain-with-cons.patchtext/x-patch; charset=UTF-8; name=v3-0001-fix-INSERT-generated-column-over-domain-with-cons.patchDownload
From 610ffedb4ee55207489397ad139a712c54dcdb1a Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Thu, 10 Apr 2025 16:47:37 +0800
Subject: [PATCH v3 1/1] fix INSERT generated column over domain with
 constraints
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

create domain d3 as int check (value is not null);
create table t0(b int, a d3 GENERATED ALWAYS as (b + 11) stored);

insert into t0 values (1, default);
ERROR:  value for domain d3 violates check constraint "d3_check"

explain(costs off, verbose) insert into t0 values (1, default);
              QUERY PLAN
---------------------------------------
 Insert on public.t0
   ->  Result
         Output: 1, NULL::integer

For INSERT operation, for Query->targetList, we should not make a generated column
over domain with constraint to a CoerceToDomain node, instead, we make it as a
simple null Const over domain's base type.

When a column is a generated column in an INSERT, expand_insert_targetlist
should unconditionally generate a null Const to be inserted. If we are not
doing this way, we might end up wrapping the null Const in a CoerceToDomain
node, which may trigger runtime error earlier if the domain has a NOT NULL
constraint. That's not fine, as generated columns are already handled in
ExecComputeStoredGenerated.

context: https://git.postgresql.org/cgit/postgresql.git/commit/?id=0da39aa7667b06e16189d318f7850d559d446d52
discussion: https://postgr.es/m/CACJufxG59tip2+9h=rEv-ykOFjt0cbsPVchhi0RTij8bABBA0Q@mail.gmail.com
---
 src/backend/executor/nodeModifyTable.c        | 36 ++++++++++++++----
 src/backend/optimizer/prep/preptlist.c        | 37 ++++++++++++++++++-
 .../regress/expected/generated_stored.out     | 32 ++++++++++++++++
 src/test/regress/sql/generated_stored.sql     | 15 ++++++++
 4 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 309e27f8b5f..5bd80456c3e 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -67,6 +67,7 @@
 #include "storage/lmgr.h"
 #include "utils/builtins.h"
 #include "utils/datum.h"
+#include "utils/lsyscache.h"
 #include "utils/rel.h"
 #include "utils/snapmgr.h"
 
@@ -216,13 +217,34 @@ ExecCheckPlanOutput(Relation resultRel, List *targetList)
 		{
 			/* Normal case: demand type match */
 			if (exprType((Node *) tle->expr) != attr->atttypid)
-				ereport(ERROR,
-						(errcode(ERRCODE_DATATYPE_MISMATCH),
-						 errmsg("table row type and query-specified row type do not match"),
-						 errdetail("Table has type %s at ordinal position %d, but query expects %s.",
-								   format_type_be(attr->atttypid),
-								   attno,
-								   format_type_be(exprType((Node *) tle->expr)))));
+			{
+				if (attr->attgenerated == '\0')
+				{
+					ereport(ERROR,
+							errcode(ERRCODE_DATATYPE_MISMATCH),
+							errmsg("table row type and query-specified row type do not match"),
+							errdetail("Table has type %s at ordinal position %d, but query expects %s.",
+									   format_type_be(attr->atttypid),
+									   attno,
+									   format_type_be(exprType((Node *) tle->expr))));
+				}
+				else
+				{
+					/* see rewriteTargetListIU comments for domain over generated column */
+					Oid			baseTypeId;
+					int32		baseTypeMod = attr->atttypmod;
+					baseTypeId = getBaseTypeAndTypmod(attr->atttypid, &baseTypeMod);
+
+					if (exprType((Node *) tle->expr) != baseTypeId)
+						ereport(ERROR,
+								errcode(ERRCODE_DATATYPE_MISMATCH),
+								errmsg("table row type and query-specified row type do not match"),
+								errdetail("Table has type %s at ordinal position %d, but query expects %s.",
+										   format_type_be(attr->atttypid),
+										   attno,
+										   format_type_be(exprType((Node *) tle->expr))));
+				}
+			}
 		}
 		else
 		{
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index c2a77503d4b..d28ea7ed071 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -44,6 +44,7 @@
 #include "optimizer/tlist.h"
 #include "parser/parse_coerce.h"
 #include "parser/parsetree.h"
+#include "utils/lsyscache.h"
 #include "utils/rel.h"
 
 static List *expand_insert_targetlist(PlannerInfo *root, List *tlist,
@@ -434,7 +435,7 @@ expand_insert_targetlist(PlannerInfo *root, List *tlist, Relation rel)
 			 */
 			Node	   *new_expr;
 
-			if (!att_tup->attisdropped)
+			if (!att_tup->attisdropped && att_tup->attgenerated == '\0')
 			{
 				new_expr = coerce_null_to_domain(att_tup->atttypid,
 												 att_tup->atttypmod,
@@ -445,6 +446,40 @@ expand_insert_targetlist(PlannerInfo *root, List *tlist, Relation rel)
 				if (!IsA(new_expr, Const))
 					new_expr = eval_const_expressions(root, new_expr);
 			}
+			else if (!att_tup->attisdropped && att_tup->attgenerated != '\0')
+			{
+				/*
+				* During an INSERT, we first process the target list (tlist) via
+				* Result node for projection and input datatype checks.  Later we
+				* do evaluation of generation expression in
+				* ExecComputeStoredGenerated.  The target list (tlist) entries may
+				* have generated column over on domain types.  Normally, this works
+				* fine.
+				*
+				* However, if that domain type has a NOT NULL constraint or a
+				* CHECK constraint is logically equivalent to NOT NULL, the
+				* INSERT may throw run-time error earlier due to domain not-null
+				* violation. This happens because generated column entries in
+				* the target list may initialized as null Const warpped inside
+				* CoerceToDomain node, and we actually evaulate it before
+				* ExecComputeStoredGenerated.
+				*
+				* To avoid it, the generated column's TargetEntry is
+				* unconditionally set as a null Const, So no need to worry about
+				* run-time error while evaluating CoerceToDomain earlier.
+				*/
+				Oid			baseTypeId;
+				int32		baseTypeMod = att_tup->atttypmod;
+
+				baseTypeId = getBaseTypeAndTypmod(att_tup->atttypid, &baseTypeMod);
+				new_expr = (Node *) makeConst(baseTypeId,
+											  baseTypeMod,
+											  att_tup->attcollation,
+											  att_tup->attlen,
+											  (Datum) 0,
+											  true,	/* isnull */
+											  att_tup->attbyval);
+			}
 			else
 			{
 				/* Insert NULL for dropped column */
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index 8cccd1d7fe9..ffa844ca903 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -847,6 +847,38 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 INSERT INTO gtest24r (a) VALUES (4);  -- ok
 INSERT INTO gtest24r (a) VALUES (6);  -- error
 ERROR:  value for domain gtestdomain1 violates check constraint "gtestdomain1_check"
+CREATE DOMAIN dnn AS int NOT NULL;
+CREATE DOMAIN dnn_check AS int CHECK (value IS NOT NULL);
+CREATE TABLE gtest24nn (
+    a int,
+    b dnn GENERATED ALWAYS AS (a + 1) STORED,
+    c dnn GENERATED ALWAYS AS (11) STORED,
+    d dnn_check GENERATED ALWAYS AS (a + 11) STORED
+);
+EXPLAIN (COSTS OFF, VERBOSE) INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT); --ok
+                           QUERY PLAN                           
+----------------------------------------------------------------
+ Insert on generated_stored_tests.gtest24nn
+   ->  Result
+         Output: 1, NULL::integer, NULL::integer, NULL::integer
+(3 rows)
+
+INSERT INTO gtest24nn VALUES (NULL, DEFAULT, DEFAULT, DEFAULT); --error
+ERROR:  domain dnn does not allow null values
+INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT) RETURNING *; --ok
+ a | b | c  | d  
+---+---+----+----
+ 1 | 2 | 11 | 12
+(1 row)
+
+UPDATE gtest24nn SET b = DEFAULT, c = default, d = default, a = 2 WHERE a = 1 RETURNING *; --ok
+ a | b | c  | d  
+---+---+----+----
+ 2 | 3 | 11 | 13
+(1 row)
+
+UPDATE gtest24nn SET a = NULL WHERE a = 2 RETURNING *; --error
+ERROR:  domain dnn does not allow null values
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) STORED);
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index 50e94e5c673..ba8ae62dea0 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -419,6 +419,21 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 INSERT INTO gtest24r (a) VALUES (4);  -- ok
 INSERT INTO gtest24r (a) VALUES (6);  -- error
 
+CREATE DOMAIN dnn AS int NOT NULL;
+CREATE DOMAIN dnn_check AS int CHECK (value IS NOT NULL);
+CREATE TABLE gtest24nn (
+    a int,
+    b dnn GENERATED ALWAYS AS (a + 1) STORED,
+    c dnn GENERATED ALWAYS AS (11) STORED,
+    d dnn_check GENERATED ALWAYS AS (a + 11) STORED
+);
+
+EXPLAIN (COSTS OFF, VERBOSE) INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT); --ok
+INSERT INTO gtest24nn VALUES (NULL, DEFAULT, DEFAULT, DEFAULT); --error
+INSERT INTO gtest24nn VALUES (1, DEFAULT, DEFAULT, DEFAULT) RETURNING *; --ok
+UPDATE gtest24nn SET b = DEFAULT, c = default, d = default, a = 2 WHERE a = 1 RETURNING *; --ok
+UPDATE gtest24nn SET a = NULL WHERE a = 2 RETURNING *; --error
+
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) STORED);
-- 
2.34.1

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: jian he (#3)
1 attachment(s)
Re: bug in stored generated column over domain with constraints.

jian he <jian.universality@gmail.com> writes:

new patch attached.

I looked this over. It's kind of astonishing that nobody has reported
this before, because AFAICT it's been broken since we invented
generated columns.

rewriteTargetListIU, expand_insert_targetlist these two places can
make a null Const TargetEntry for the generated column in an INSERT
operation.

rewriteTargetListIU will *not* do that: it explicitly does nothing
for an attgenerated column, except apply a bunch of error checks.
See about line 988 in HEAD. So it seems sufficient to fix
expand_insert_targetlist as you've done here. And then we have to
make ExecCheckPlanOutput cope, too.

I think that this patch is technically correct, but I don't like
it much because it pays little attention to keeping
expand_insert_targetlist and ExecCheckPlanOutput readable, and no
attention at all to keeping their logic parallel. I think we can
make the code cleaner by moving the default case to the end, as
in the attached.

The test cases seemed a bit overdone, especially in comparison
to the adjacent existing tests. Fine for development maybe,
but I don't see the value in carrying them forever. On the other
hand, there's a comment at the top of the test script:
-- keep these tests aligned with generated_virtual.sql
The VIRTUAL case is currently rejecting domains altogether.
But perhaps that won't be true forever, so I think we ought
to follow that advice.

So I end with the attached v4.

regards, tom lane

Attachments:

v4-0001-fix-INSERT-generated-column-over-domain-with-cons.patchtext/x-diff; charset=us-ascii; name*0=v4-0001-fix-INSERT-generated-column-over-domain-with-cons.p; name*1=atchDownload
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 309e27f8b5f..333cbf78343 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -212,19 +212,10 @@ ExecCheckPlanOutput(Relation resultRel, List *targetList)
 		attr = TupleDescAttr(resultDesc, attno);
 		attno++;
 
-		if (!attr->attisdropped)
-		{
-			/* Normal case: demand type match */
-			if (exprType((Node *) tle->expr) != attr->atttypid)
-				ereport(ERROR,
-						(errcode(ERRCODE_DATATYPE_MISMATCH),
-						 errmsg("table row type and query-specified row type do not match"),
-						 errdetail("Table has type %s at ordinal position %d, but query expects %s.",
-								   format_type_be(attr->atttypid),
-								   attno,
-								   format_type_be(exprType((Node *) tle->expr)))));
-		}
-		else
+		/*
+		 * Special cases here should match planner's expand_insert_targetlist.
+		 */
+		if (attr->attisdropped)
 		{
 			/*
 			 * For a dropped column, we can't check atttypid (it's likely 0).
@@ -239,6 +230,35 @@ ExecCheckPlanOutput(Relation resultRel, List *targetList)
 						 errdetail("Query provides a value for a dropped column at ordinal position %d.",
 								   attno)));
 		}
+		else if (attr->attgenerated)
+		{
+			/*
+			 * For a generated column, the planner will have inserted a null
+			 * of the column's base type (to avoid possibly failing on domain
+			 * not-null constraints).  It doesn't seem worth insisting on that
+			 * exact type though, since a null value is type-independent.  As
+			 * above, just insist on *some* NULL constant.
+			 */
+			if (!IsA(tle->expr, Const) ||
+				!((Const *) tle->expr)->constisnull)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATATYPE_MISMATCH),
+						 errmsg("table row type and query-specified row type do not match"),
+						 errdetail("Query provides a value for a generated column at ordinal position %d.",
+								   attno)));
+		}
+		else
+		{
+			/* Normal case: demand type match */
+			if (exprType((Node *) tle->expr) != attr->atttypid)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATATYPE_MISMATCH),
+						 errmsg("table row type and query-specified row type do not match"),
+						 errdetail("Table has type %s at ordinal position %d, but query expects %s.",
+								   format_type_be(attr->atttypid),
+								   attno,
+								   format_type_be(exprType((Node *) tle->expr)))));
+		}
 	}
 	if (attno != resultDesc->natts)
 		ereport(ERROR,
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index c2a77503d4b..ffc9d6c3f30 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -44,6 +44,7 @@
 #include "optimizer/tlist.h"
 #include "parser/parse_coerce.h"
 #include "parser/parsetree.h"
+#include "utils/lsyscache.h"
 #include "utils/rel.h"
 
 static List *expand_insert_targetlist(PlannerInfo *root, List *tlist,
@@ -419,9 +420,8 @@ expand_insert_targetlist(PlannerInfo *root, List *tlist, Relation rel)
 			 *
 			 * INSERTs should insert NULL in this case.  (We assume the
 			 * rewriter would have inserted any available non-NULL default
-			 * value.)  Also, if the column isn't dropped, apply any domain
-			 * constraints that might exist --- this is to catch domain NOT
-			 * NULL.
+			 * value.)  Also, normally we must apply any domain constraints
+			 * that might exist --- this is to catch domain NOT NULL.
 			 *
 			 * When generating a NULL constant for a dropped column, we label
 			 * it INT4 (any other guaranteed-to-exist datatype would do as
@@ -431,21 +431,17 @@ expand_insert_targetlist(PlannerInfo *root, List *tlist, Relation rel)
 			 * representation is datatype-independent.  This could perhaps
 			 * confuse code comparing the finished plan to the target
 			 * relation, however.
+			 *
+			 * Another exception is that if the column is generated, the value
+			 * we produce here will be ignored, and we don't want to risk
+			 * throwing an error.  So in that case we *don't* want to apply
+			 * domain constraints, so we must produce a NULL of the base type.
+			 * Again, code comparing the finished plan to the target relation
+			 * must account for this.
 			 */
 			Node	   *new_expr;
 
-			if (!att_tup->attisdropped)
-			{
-				new_expr = coerce_null_to_domain(att_tup->atttypid,
-												 att_tup->atttypmod,
-												 att_tup->attcollation,
-												 att_tup->attlen,
-												 att_tup->attbyval);
-				/* Must run expression preprocessing on any non-const nodes */
-				if (!IsA(new_expr, Const))
-					new_expr = eval_const_expressions(root, new_expr);
-			}
-			else
+			if (att_tup->attisdropped)
 			{
 				/* Insert NULL for dropped column */
 				new_expr = (Node *) makeConst(INT4OID,
@@ -456,6 +452,33 @@ expand_insert_targetlist(PlannerInfo *root, List *tlist, Relation rel)
 											  true, /* isnull */
 											  true /* byval */ );
 			}
+			else if (att_tup->attgenerated)
+			{
+				/* Generated column, insert a NULL of the base type */
+				Oid			baseTypeId = att_tup->atttypid;
+				int32		baseTypeMod = att_tup->atttypmod;
+
+				baseTypeId = getBaseTypeAndTypmod(baseTypeId, &baseTypeMod);
+				new_expr = (Node *) makeConst(baseTypeId,
+											  baseTypeMod,
+											  att_tup->attcollation,
+											  att_tup->attlen,
+											  (Datum) 0,
+											  true, /* isnull */
+											  att_tup->attbyval);
+			}
+			else
+			{
+				/* Normal column, insert a NULL of the column datatype */
+				new_expr = coerce_null_to_domain(att_tup->atttypid,
+												 att_tup->atttypmod,
+												 att_tup->attcollation,
+												 att_tup->attlen,
+												 att_tup->attbyval);
+				/* Must run expression preprocessing on any non-const nodes */
+				if (!IsA(new_expr, Const))
+					new_expr = eval_const_expressions(root, new_expr);
+			}
 
 			new_tle = makeTargetEntry((Expr *) new_expr,
 									  attrno,
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index 8cccd1d7fe9..16de30ab191 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -847,6 +847,11 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 INSERT INTO gtest24r (a) VALUES (4);  -- ok
 INSERT INTO gtest24r (a) VALUES (6);  -- error
 ERROR:  value for domain gtestdomain1 violates check constraint "gtestdomain1_check"
+CREATE DOMAIN gtestdomainnn AS int CHECK (VALUE IS NOT NULL);
+CREATE TABLE gtest24nn (a int, b gtestdomainnn GENERATED ALWAYS AS (a * 2) STORED);
+INSERT INTO gtest24nn (a) VALUES (4);  -- ok
+INSERT INTO gtest24nn (a) VALUES (NULL);  -- error
+ERROR:  value for domain gtestdomainnn violates check constraint "gtestdomainnn_check"
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) STORED);
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index 26bbe1e9c31..6300e7c1d96 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -800,6 +800,11 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 ERROR:  virtual generated column "b" cannot have a domain type
 --INSERT INTO gtest24r (a) VALUES (4);  -- ok
 --INSERT INTO gtest24r (a) VALUES (6);  -- error
+CREATE DOMAIN gtestdomainnn AS int CHECK (VALUE IS NOT NULL);
+CREATE TABLE gtest24nn (a int, b gtestdomainnn GENERATED ALWAYS AS (a * 2) VIRTUAL);
+ERROR:  virtual generated column "b" cannot have a domain type
+--INSERT INTO gtest24nn (a) VALUES (4);  -- ok
+--INSERT INTO gtest24nn (a) VALUES (NULL);  -- error
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) VIRTUAL);
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index 50e94e5c673..4ec155f2da9 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -419,6 +419,11 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 INSERT INTO gtest24r (a) VALUES (4);  -- ok
 INSERT INTO gtest24r (a) VALUES (6);  -- error
 
+CREATE DOMAIN gtestdomainnn AS int CHECK (VALUE IS NOT NULL);
+CREATE TABLE gtest24nn (a int, b gtestdomainnn GENERATED ALWAYS AS (a * 2) STORED);
+INSERT INTO gtest24nn (a) VALUES (4);  -- ok
+INSERT INTO gtest24nn (a) VALUES (NULL);  -- error
+
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) STORED);
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index 13cfbd76859..b4eedeee2fb 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -453,6 +453,11 @@ CREATE TABLE gtest24r (a int PRIMARY KEY, b gtestdomain1range GENERATED ALWAYS A
 --INSERT INTO gtest24r (a) VALUES (4);  -- ok
 --INSERT INTO gtest24r (a) VALUES (6);  -- error
 
+CREATE DOMAIN gtestdomainnn AS int CHECK (VALUE IS NOT NULL);
+CREATE TABLE gtest24nn (a int, b gtestdomainnn GENERATED ALWAYS AS (a * 2) VIRTUAL);
+--INSERT INTO gtest24nn (a) VALUES (4);  -- ok
+--INSERT INTO gtest24nn (a) VALUES (NULL);  -- error
+
 -- typed tables (currently not supported)
 CREATE TYPE gtest_type AS (f1 integer, f2 text, f3 bigint);
 CREATE TABLE gtest28 OF gtest_type (f1 WITH OPTIONS GENERATED ALWAYS AS (f2 *2) VIRTUAL);
#5jian he
jian.universality@gmail.com
In reply to: Tom Lane (#4)
Re: bug in stored generated column over domain with constraints.

On Tue, Apr 15, 2025 at 4:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

jian he <jian.universality@gmail.com> writes:

new patch attached.

I looked this over. It's kind of astonishing that nobody has reported
this before, because AFAICT it's been broken since we invented
generated columns.

rewriteTargetListIU, expand_insert_targetlist these two places can
make a null Const TargetEntry for the generated column in an INSERT
operation.

rewriteTargetListIU will *not* do that: it explicitly does nothing
for an attgenerated column, except apply a bunch of error checks.
See about line 988 in HEAD. So it seems sufficient to fix
expand_insert_targetlist as you've done here. And then we have to
make ExecCheckPlanOutput cope, too.

I think that this patch is technically correct, but I don't like
it much because it pays little attention to keeping
expand_insert_targetlist and ExecCheckPlanOutput readable, and no
attention at all to keeping their logic parallel. I think we can
make the code cleaner by moving the default case to the end, as
in the attached.

your ExecCheckPlanOutput change makes sense to me.
call getBaseTypeAndTypmod in ExecCheckPlanOutput would be a waste cycle,
given that we will compute the generated column later.

The test cases seemed a bit overdone, especially in comparison
to the adjacent existing tests. Fine for development maybe,
but I don't see the value in carrying them forever. On the other
hand, there's a comment at the top of the test script:
-- keep these tests aligned with generated_virtual.sql
The VIRTUAL case is currently rejecting domains altogether.
But perhaps that won't be true forever, so I think we ought
to follow that advice.

I submitted a patch for the domain over the virtual generated column,
so didn't add such a test on it.

Thanks for simplifying the tests, overall all looks good.

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: jian he (#5)
Re: bug in stored generated column over domain with constraints.

jian he <jian.universality@gmail.com> writes:

Thanks for simplifying the tests, overall all looks good.

OK, pushed and back-patched, but only to v14. I tried to make
a variant that'd work in v13, but it caused ExecCheckPlanOutput
to throw "Query provides a value for a generated column" errors
for UPDATEs. That's surely related to the major refactoring of
UPDATE targetlists that we did in v14. Given that we've had
zero field complaints about this problem so far, I think the
risk of breaking something in v13 exceeds the value of fixing
the bug, so I left v13 alone.

regards, tom lane