From 4604928edd7236587fcd27ee2ecc0c33124cbbfe Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 15 Apr 2025 12:08:34 -0400 Subject: [PATCH] Fix failure for generated column with a not-null domain constraint. If a GENERATED column is declared to have a domain data type where the domain's constraints disallow null values, INSERT commands failed because we built a targetlist that included coercing a null constant to the domain's type. The failure occurred even when the generated value would have been perfectly OK. This is adjacent to the issues fixed in 0da39aa76, but we didn't notice for lack of testing a domain with such a constraint. We aren't going to use the result of the targetlist entry for the generated column --- ExecComputeStoredGenerated will overwrite it. So it's not really necessary that it have the exact datatype of the generated column. This patch fixes the problem by changing the targetlist entry to be a null Const of the domain's base type, which should be sufficiently legal. (We do have to tweak ExecCheckPlanOutput to accept the situation, though.) This has been broken since we implemented generated columns. However, this patch only applies easily as far back as v14, partly because I (tgl) only carried 0da39aa76 back that far, but mostly because v14 significantly refactored the handling of INSERT/UPDATE targetlists. Given the lack of field complaints and the short remaining support lifetime of v13, I judge the cost-benefit ratio not good for devising a version that would work in v13. Reported-by: jian he Author: jian he Reviewed-by: Tom Lane Discussion: https://postgr.es/m/CACJufxG59tip2+9h=rEv-ykOFjt0cbsPVchhi0RTij8bABBA0Q@mail.gmail.com Backpatch-through: 14 --- src/backend/executor/nodeModifyTable.c | 46 +++++++++++++++------ src/backend/optimizer/prep/preptlist.c | 53 ++++++++++++++++++------- src/test/regress/expected/generated.out | 5 +++ src/test/regress/sql/generated.sql | 5 +++ 4 files changed, 81 insertions(+), 28 deletions(-) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 214482d193d..3a4ce5c32f8 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -129,19 +129,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). @@ -156,6 +147,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 e9151c9589a..125fcf01390 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, @@ -334,9 +335,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 @@ -346,21 +346,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, @@ -371,6 +367,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.out b/src/test/regress/expected/generated.out index dccf0ddc522..74680d385be 100644 --- a/src/test/regress/expected/generated.out +++ b/src/test/regress/expected/generated.out @@ -685,6 +685,11 @@ CREATE TABLE gtest24 (a int PRIMARY KEY, b gtestdomain1 GENERATED ALWAYS AS (a * INSERT INTO gtest24 (a) VALUES (4); -- ok INSERT INTO gtest24 (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/sql/generated.sql b/src/test/regress/sql/generated.sql index 4d2fc9c5ae8..06faa1d72a6 100644 --- a/src/test/regress/sql/generated.sql +++ b/src/test/regress/sql/generated.sql @@ -361,6 +361,11 @@ CREATE TABLE gtest24 (a int PRIMARY KEY, b gtestdomain1 GENERATED ALWAYS AS (a * INSERT INTO gtest24 (a) VALUES (4); -- ok INSERT INTO gtest24 (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);