Handle default NULL insertion a little better.

If a column is omitted in an INSERT, and there's no column default,
the code in preptlist.c generates a NULL Const to be inserted.
Furthermore, if the column is of a domain type, we wrap the Const
in CoerceToDomain, so as to throw a run-time error if the domain
has a NOT NULL constraint.  That's fine as far as it goes, but
there are two problems:

1. We're being sloppy about the type/typmod that the Const is
labeled with.  It really should have the domain's base type/typmod,
since it's the input to CoerceToDomain not the output.  This can
result in coerce_to_domain inserting a useless length-coercion
function (useless because it's being applied to a null).  The
coercion would typically get const-folded away later, but it'd
be better not to create it in the first place.

2. We're not applying expression preprocessing (specifically,
eval_const_expressions) to the resulting expression tree.
The planner's primary expression-preprocessing pass already happened,
so that means the length coercion step and CoerceToDomain node miss
preprocessing altogether.

This is at the least inefficient, since it means the length coercion
and CoerceToDomain will actually be executed for each inserted row,
though they could be const-folded away in most cases.  Worse, it
seems possible that missing preprocessing for the length coercion
could result in an invalid plan (for example, due to failing to
perform default-function-argument insertion).  I'm not aware of
any live bug of that sort with core datatypes, and it might be
unreachable for extension types as well because of restrictions of
CREATE CAST, but I'm not entirely convinced that it's unreachable.
Hence, it seems worth back-patching the fix (although I only went
back to v14, as the patch doesn't apply cleanly at all in v13).

There are several places in the rewriter that are building null
domain constants the same way as preptlist.c.  While those are
before the planner and hence don't have any reachable bug, they're
still applying a length coercion that will be const-folded away
later, uselessly wasting cycles.  Hence, make a utility routine
that all of these places can call to do it right.

Making this code more careful about the typmod assigned to the
generated NULL constant has visible but cosmetic effects on some
of the plans shown in contrib/postgres_fdw's regression tests.

Discussion: https://postgr.es/m/1865579.1738113656@sss.pgh.pa.us
Backpatch-through: 14

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 03bdc3a3aaf8973da7425aa526651fa3b1632016..375769b828cfb6c889472f4f1b5cd287c0954b30 100644 (file)
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -4223,13 +4223,13 @@ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
 
 PREPARE st7 AS INSERT INTO ft1 (c1,c2,c3) VALUES (1001,101,'foo');
 EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
-                                                                                           QUERY PLAN                                                                                            
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+                                                                                             QUERY PLAN                                                                                              
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Insert on public.ft1
    Remote SQL: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
    Batch Size: 1
    ->  Result
-         Output: NULL::integer, 1001, 101, 'foo'::text, NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying, 'ft1       '::character(10), NULL::user_enum
+         Output: NULL::integer, 1001, 101, 'foo'::text, NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying(10), 'ft1       '::character(10), NULL::user_enum
 (5 rows)
 
 ALTER TABLE "S 1"."T 1" RENAME TO "T 0";
@@ -4257,13 +4257,13 @@ EXECUTE st6;
 (9 rows)
 
 EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
-                                                                                           QUERY PLAN                                                                                            
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+                                                                                             QUERY PLAN                                                                                              
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Insert on public.ft1
    Remote SQL: INSERT INTO "S 1"."T 0"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
    Batch Size: 1
    ->  Result
-         Output: NULL::integer, 1001, 101, 'foo'::text, NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying, 'ft1       '::character(10), NULL::user_enum
+         Output: NULL::integer, 1001, 101, 'foo'::text, NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying(10), 'ft1       '::character(10), NULL::user_enum
 (5 rows)
 
 ALTER TABLE "S 1"."T 0" RENAME TO "T 1";
@@ -4634,13 +4634,13 @@ explain (verbose, costs off) select * from ft3 f, loct3 l
 -- ===================================================================
 EXPLAIN (verbose, costs off)
 INSERT INTO ft2 (c1,c2,c3) SELECT c1+1000,c2+100, c3 || c3 FROM ft2 LIMIT 20;
-                                                                                                                    QUERY PLAN                                                                                                                    
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+                                                                                                                      QUERY PLAN                                                                                                                      
+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Insert on public.ft2
    Remote SQL: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
    Batch Size: 1
    ->  Subquery Scan on "*SELECT*"
-         Output: "*SELECT*"."?column?", "*SELECT*"."?column?_1", NULL::integer, "*SELECT*"."?column?_2", NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying, 'ft2       '::character(10), NULL::user_enum
+         Output: "*SELECT*"."?column?", "*SELECT*"."?column?_1", NULL::integer, "*SELECT*"."?column?_2", NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying(10), 'ft2       '::character(10), NULL::user_enum
          ->  Foreign Scan on public.ft2 ft2_1
                Output: (ft2_1.c1 + 1000), (ft2_1.c2 + 100), (ft2_1.c3 || ft2_1.c3)
                Remote SQL: SELECT "C 1", c2, c3 FROM "S 1"."T 1" LIMIT 20::bigint
@@ -5750,14 +5750,14 @@ SELECT c1,c2,c3,c4 FROM ft2 ORDER BY c1;
 
 EXPLAIN (verbose, costs off)
 INSERT INTO ft2 (c1,c2,c3) VALUES (1200,999,'foo') RETURNING tableoid::regclass;
-                                                                                           QUERY PLAN                                                                                            
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+                                                                                             QUERY PLAN                                                                                              
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Insert on public.ft2
    Output: (ft2.tableoid)::regclass
    Remote SQL: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
    Batch Size: 1
    ->  Result
-         Output: 1200, 999, NULL::integer, 'foo'::text, NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying, 'ft2       '::character(10), NULL::user_enum
+         Output: 1200, 999, NULL::integer, 'foo'::text, NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying(10), 'ft2       '::character(10), NULL::user_enum
 (6 rows)
 
 INSERT INTO ft2 (c1,c2,c3) VALUES (1200,999,'foo') RETURNING tableoid::regclass;

diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index 133966b6250616b3330371cddbfff87ea14fe9b9..a29fa9f9bc583c374834218d65dc0ca5f125aff3 100644 (file)
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -46,7 +46,8 @@
 #include "parser/parsetree.h"
 #include "utils/rel.h"
 
-static List *expand_insert_targetlist(List *tlist, Relation rel);
+static List *expand_insert_targetlist(PlannerInfo *root, List *tlist,
+                                                                         Relation rel);
 
 
 /*
@@ -102,7 +103,7 @@ preprocess_targetlist(PlannerInfo *root)
         */
        tlist = parse->targetList;
        if (command_type == CMD_INSERT)
-               tlist = expand_insert_targetlist(tlist, target_relation);
+               tlist = expand_insert_targetlist(root, tlist, target_relation);
        else if (command_type == CMD_UPDATE)
                root->update_colnos = extract_update_targetlist_colnos(tlist);
 
@@ -148,7 +149,8 @@ preprocess_targetlist(PlannerInfo *root)
                        ListCell   *l2;
 
                        if (action->commandType == CMD_INSERT)
-                               action->targetList = expand_insert_targetlist(action->targetList,
+                               action->targetList = expand_insert_targetlist(root,
+                                                                                                                         action->targetList,
                                                                                                                          target_relation);
                        else if (action->commandType == CMD_UPDATE)
                                action->updateColnos =
@@ -352,7 +354,7 @@ extract_update_targetlist_colnos(List *tlist)
  * but now this code is only applied to INSERT targetlists.
  */
 static List *
-expand_insert_targetlist(List *tlist, Relation rel)
+expand_insert_targetlist(PlannerInfo *root, List *tlist, Relation rel)
 {
        List       *new_tlist = NIL;
        ListCell   *tlist_item;
@@ -406,26 +408,18 @@ expand_insert_targetlist(List *tlist, Relation rel)
                         * confuse code comparing the finished plan to the target
                         * relation, however.
                         */
-                       Oid                     atttype = att_tup->atttypid;
-                       Oid                     attcollation = att_tup->attcollation;
                        Node       *new_expr;
 
                        if (!att_tup->attisdropped)
                        {
-                               new_expr = (Node *) makeConst(atttype,
-                                                                                         -1,
-                                                                                         attcollation,
-                                                                                         att_tup->attlen,
-                                                                                         (Datum) 0,
-                                                                                         true, /* isnull */
-                                                                                         att_tup->attbyval);
-                               new_expr = coerce_to_domain(new_expr,
-                                                                                       InvalidOid, -1,
-                                                                                       atttype,
-                                                                                       COERCION_IMPLICIT,
-                                                                                       COERCE_IMPLICIT_CAST,
-                                                                                       -1,
-                                                                                       false);
+                               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
                        {

diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index c4e958e4aa81af65c7cfbd291df3c4ff9610fcc1..da4e1bd77419990e971c5ae51a1641181bafcfd3 100644 (file)
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -1263,6 +1263,43 @@ coerce_to_specific_type(ParseState *pstate, Node *node,
                                                                                  constructName);
 }
 
+/*
+ * coerce_null_to_domain()
+ *             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.
+ */
+Node *
+coerce_null_to_domain(Oid typid, int32 typmod, Oid collation,
+                                         int typlen, bool typbyval)
+{
+       Node       *result;
+       Oid                     baseTypeId;
+       int32           baseTypeMod = typmod;
+
+       /*
+        * The constant must appear to have the domain's base type/typmod, else
+        * coerce_to_domain() will apply a length coercion which is useless.
+        */
+       baseTypeId = getBaseTypeAndTypmod(typid, &baseTypeMod);
+       result = (Node *) makeConst(baseTypeId,
+                                                               baseTypeMod,
+                                                               collation,
+                                                               typlen,
+                                                               (Datum) 0,
+                                                               true,   /* isnull */
+                                                               typbyval);
+       if (typid != baseTypeId)
+               result = coerce_to_domain(result,
+                                                                 baseTypeId, baseTypeMod,
+                                                                 typid,
+                                                                 COERCION_IMPLICIT,
+                                                                 COERCE_IMPLICIT_CAST,
+                                                                 -1,
+                                                                 false);
+       return result;
+}
+
 /*
  * parser_coercion_errposition - report coercion error location, if possible
  *

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 2b2f2da0f32a6a05aa5d18ff4308fbf59c8a5975..d2a0e501d1e137b42d95a32dd7f45d8669fcff39 100644 (file)
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -997,23 +997,11 @@ rewriteTargetListIU(List *targetList,
                                if (commandType == CMD_INSERT)
                                        new_tle = NULL;
                                else
-                               {
-                                       new_expr = (Node *) makeConst(att_tup->atttypid,
-                                                                                                 -1,
-                                                                                                 att_tup->attcollation,
-                                                                                                 att_tup->attlen,
-                                                                                                 (Datum) 0,
-                                                                                                 true, /* isnull */
-                                                                                                 att_tup->attbyval);
-                                       /* this is to catch a NOT NULL domain constraint */
-                                       new_expr = coerce_to_domain(new_expr,
-                                                                                               InvalidOid, -1,
-                                                                                               att_tup->atttypid,
-                                                                                               COERCION_IMPLICIT,
-                                                                                               COERCE_IMPLICIT_CAST,
-                                                                                               -1,
-                                                                                               false);
-                               }
+                                       new_expr = coerce_null_to_domain(att_tup->atttypid,
+                                                                                                        att_tup->atttypmod,
+                                                                                                        att_tup->attcollation,
+                                                                                                        att_tup->attlen,
+                                                                                                        att_tup->attbyval);
                        }
 
                        if (new_expr)
@@ -1575,21 +1563,11 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
                                                continue;
                                        }
 
-                                       new_expr = (Node *) makeConst(att_tup->atttypid,
-                                                                                                 -1,
-                                                                                                 att_tup->attcollation,
-                                                                                                 att_tup->attlen,
-                                                                                                 (Datum) 0,
-                                                                                                 true, /* isnull */
-                                                                                                 att_tup->attbyval);
-                                       /* this is to catch a NOT NULL domain constraint */
-                                       new_expr = coerce_to_domain(new_expr,
-                                                                                               InvalidOid, -1,
-                                                                                               att_tup->atttypid,
-                                                                                               COERCION_IMPLICIT,
-                                                                                               COERCE_IMPLICIT_CAST,
-                                                                                               -1,
-                                                                                               false);
+                                       new_expr = coerce_null_to_domain(att_tup->atttypid,
+                                                                                                        att_tup->atttypmod,
+                                                                                                        att_tup->attcollation,
+                                                                                                        att_tup->attlen,
+                                                                                                        att_tup->attbyval);
                                }
                                newList = lappend(newList, new_expr);
                        }

diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 8c8067ba721daa94724723fb5eb3b5135b3157e5..6d4985abad9ed775afa8ed2113c5f1f73738d462 100644 (file)
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -22,6 +22,7 @@
 #include "parser/parse_relation.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteManip.h"
+#include "utils/lsyscache.h"
 
 
 typedef struct
@@ -1466,20 +1467,21 @@ ReplaceVarsFromTargetList_callback(Var *var,
                                return (Node *) var;
 
                        case REPLACEVARS_SUBSTITUTE_NULL:
-
-                               /*
-                                * If Var is of domain type, we should add a CoerceToDomain
-                                * node, in case there is a NOT NULL domain constraint.
-                                */
-                               return coerce_to_domain((Node *) makeNullConst(var->vartype,
-                                                                                                                          var->vartypmod,
-                                                                                                                          var->varcollid),
-                                                                               InvalidOid, -1,
-                                                                               var->vartype,
-                                                                               COERCION_IMPLICIT,
-                                                                               COERCE_IMPLICIT_CAST,
-                                                                               -1,
-                                                                               false);
+                               {
+                                       /*
+                                        * If Var is of domain type, we must add a CoerceToDomain
+                                        * node, in case there is a NOT NULL domain constraint.
+                                        */
+                                       int16           vartyplen;
+                                       bool            vartypbyval;
+
+                                       get_typlenbyval(var->vartype, &vartyplen, &vartypbyval);
+                                       return coerce_null_to_domain(var->vartype,
+                                                                                                var->vartypmod,
+                                                                                                var->varcollid,
+                                                                                                vartyplen,
+                                                                                                vartypbyval);
+                               }
                }
                elog(ERROR, "could not find replacement targetlist entry for attno %d",
                         var->varattno);

diff --git a/src/include/parser/parse_coerce.h b/src/include/parser/parse_coerce.h
index b105c7da90070cd2933a630213a8e5d675a89029..3369db13fa4bf7729f3673635c1f68b6d3eb6ae9 100644 (file)
--- a/src/include/parser/parse_coerce.h
+++ b/src/include/parser/parse_coerce.h
@@ -61,6 +61,9 @@ extern Node *coerce_to_specific_type_typmod(ParseState *pstate, Node *node,
                                                                                        Oid targetTypeId, int32 targetTypmod,
                                                                                        const char *constructName);
 
+extern Node *coerce_null_to_domain(Oid typid, int32 typmod, Oid collation,
+                                                                  int typlen, bool typbyval);
+
 extern int     parser_coercion_errposition(ParseState *pstate,
                                                                                int coerce_location,
                                                                                Node *input_expr);