Re: bug in stored generated column over domain with constraints. - Mailing list pgsql-hackers

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

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);