jian he <jian.universal...@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
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);
