]> granicus.if.org Git - postgresql/commitdiff
Code review for NextValueExpr expression node type.
authorTom Lane <tgl@sss.pgh.pa.us>
Fri, 14 Jul 2017 19:25:43 +0000 (15:25 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Fri, 14 Jul 2017 19:25:43 +0000 (15:25 -0400)
Add missing infrastructure for this node type, notably in ruleutils.c where
its lack could demonstrably cause EXPLAIN to fail.  Add outfuncs/readfuncs
support.  (outfuncs support is useful today for debugging purposes.  The
readfuncs support may never be needed, since at present it would only
matter for parallel query and NextValueExpr should never appear in a
parallelizable query; but it seems like a bad idea to have a primnode type
that isn't fully supported here.)  Teach planner infrastructure that
NextValueExpr is a volatile, parallel-unsafe, non-leaky expression node
with cost cpu_operator_cost.  Given its limited scope of usage, there
*might* be no live bug today from the lack of that knowledge, but it's
certainly going to bite us on the rear someday.  Teach pg_stat_statements
about the new node type, too.

While at it, also teach cost_qual_eval() that MinMaxExpr, SQLValueFunction,
XmlExpr, and CoerceToDomain should be charged as cpu_operator_cost.
Failing to do this for SQLValueFunction was an oversight in my commit
0bb51aa96.  The others are longer-standing oversights, but no time like the
present to fix them.  (In principle, CoerceToDomain could have cost much
higher than this, but it doesn't presently seem worth trying to examine the
domain's constraints here.)

Modify execExprInterp.c to execute NextValueExpr as an out-of-line
function; it seems quite unlikely to me that it's worth insisting that
it be inlined in all expression eval methods.  Besides, providing the
out-of-line function doesn't stop anyone from inlining if they want to.

Adjust some places where NextValueExpr support had been inserted with the
aid of a dartboard rather than keeping it in the same order as elsewhere.

Discussion: https://postgr.es/m/23862.1499981661@sss.pgh.pa.us

12 files changed:
contrib/pg_stat_statements/pg_stat_statements.c
src/backend/catalog/dependency.c
src/backend/executor/execExprInterp.c
src/backend/nodes/nodeFuncs.c
src/backend/nodes/outfuncs.c
src/backend/nodes/readfuncs.c
src/backend/optimizer/path/costsize.c
src/backend/optimizer/util/clauses.c
src/backend/utils/adt/ruleutils.c
src/include/executor/execExpr.h
src/include/nodes/nodes.h
src/include/nodes/primnodes.h

index b9d4a93690570f683fb108d141c056be9d466380..fa409d72b7b4b0a21b191bc68e62f1e475b75c2d 100644 (file)
@@ -2763,6 +2763,14 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
                                APP_JUMB(ce->cursor_param);
                        }
                        break;
+               case T_NextValueExpr:
+                       {
+                               NextValueExpr *nve = (NextValueExpr *) node;
+
+                               APP_JUMB(nve->seqid);
+                               APP_JUMB(nve->typeId);
+                       }
+                       break;
                case T_InferenceElem:
                        {
                                InferenceElem *ie = (InferenceElem *) node;
index 1a7645d341f9feadf194efa39f0bf8e0607ba2f3..6fffc290fad8a859ec348364642f8a4bdab608f6 100644 (file)
@@ -1791,6 +1791,13 @@ find_expr_references_walker(Node *node,
                add_object_address(OCLASS_TYPE, cd->resulttype, 0,
                                                   context->addrs);
        }
+       else if (IsA(node, NextValueExpr))
+       {
+               NextValueExpr *nve = (NextValueExpr *) node;
+
+               add_object_address(OCLASS_CLASS, nve->seqid, 0,
+                                                  context->addrs);
+       }
        else if (IsA(node, OnConflictExpr))
        {
                OnConflictExpr *onconflict = (OnConflictExpr *) node;
@@ -1942,13 +1949,6 @@ find_expr_references_walker(Node *node,
                                                   context->addrs);
                /* fall through to examine arguments */
        }
-       else if (IsA(node, NextValueExpr))
-       {
-               NextValueExpr *nve = (NextValueExpr *) node;
-
-               add_object_address(OCLASS_CLASS, nve->seqid, 0,
-                                                  context->addrs);
-       }
 
        return expression_tree_walker(node, find_expr_references_walker,
                                                                  (void *) context);
index c227d9bdd99b2bbce4377bb49391470c37864315..f2a52f62135bfcb218a99d1bd552a0a97d9cc1b0 100644 (file)
@@ -1232,21 +1232,11 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
 
                EEO_CASE(EEOP_NEXTVALUEEXPR)
                {
-                       switch (op->d.nextvalueexpr.seqtypid)
-                       {
-                               case INT2OID:
-                                       *op->resvalue = Int16GetDatum((int16) nextval_internal(op->d.nextvalueexpr.seqid, false));
-                                       break;
-                               case INT4OID:
-                                       *op->resvalue = Int32GetDatum((int32) nextval_internal(op->d.nextvalueexpr.seqid, false));
-                                       break;
-                               case INT8OID:
-                                       *op->resvalue = Int64GetDatum((int64) nextval_internal(op->d.nextvalueexpr.seqid, false));
-                                       break;
-                               default:
-                                       elog(ERROR, "unsupported sequence type %u", op->d.nextvalueexpr.seqtypid);
-                       }
-                       *op->resnull = false;
+                       /*
+                        * Doesn't seem worthwhile to have an inline implementation
+                        * efficiency-wise.
+                        */
+                       ExecEvalNextValueExpr(state, op);
 
                        EEO_NEXT();
                }
@@ -1989,6 +1979,32 @@ ExecEvalCurrentOfExpr(ExprState *state, ExprEvalStep *op)
                         errmsg("WHERE CURRENT OF is not supported for this table type")));
 }
 
+/*
+ * Evaluate NextValueExpr.
+ */
+void
+ExecEvalNextValueExpr(ExprState *state, ExprEvalStep *op)
+{
+       int64           newval = nextval_internal(op->d.nextvalueexpr.seqid, false);
+
+       switch (op->d.nextvalueexpr.seqtypid)
+       {
+               case INT2OID:
+                       *op->resvalue = Int16GetDatum((int16) newval);
+                       break;
+               case INT4OID:
+                       *op->resvalue = Int32GetDatum((int32) newval);
+                       break;
+               case INT8OID:
+                       *op->resvalue = Int64GetDatum((int64) newval);
+                       break;
+               default:
+                       elog(ERROR, "unsupported sequence type %u",
+                                op->d.nextvalueexpr.seqtypid);
+       }
+       *op->resnull = false;
+}
+
 /*
  * Evaluate NullTest / IS NULL for rows.
  */
index 97ba25fc72c9a8387968864525895a5d62b70b1c..e3eb0c578877a514216021bfd39c0c1bb5a4f22d 100644 (file)
@@ -1642,10 +1642,10 @@ set_sa_opfuncid(ScalarArrayOpExpr *opexpr)
  * for themselves, in case additional checks should be made, or because they
  * have special rules about which parts of the tree need to be visited.
  *
- * Note: we ignore MinMaxExpr, SQLValueFunction, XmlExpr, and CoerceToDomain
- * nodes, because they do not contain SQL function OIDs.  However, they can
- * invoke SQL-visible functions, so callers should take thought about how to
- * treat them.
+ * Note: we ignore MinMaxExpr, SQLValueFunction, XmlExpr, CoerceToDomain,
+ * and NextValueExpr nodes, because they do not contain SQL function OIDs.
+ * However, they can invoke SQL-visible functions, so callers should take
+ * thought about how to treat them.
  */
 bool
 check_functions_in_node(Node *node, check_function_callback checker,
@@ -1865,12 +1865,12 @@ expression_tree_walker(Node *node,
                case T_Var:
                case T_Const:
                case T_Param:
-               case T_CoerceToDomainValue:
                case T_CaseTestExpr:
+               case T_SQLValueFunction:
+               case T_CoerceToDomainValue:
                case T_SetToDefault:
                case T_CurrentOfExpr:
                case T_NextValueExpr:
-               case T_SQLValueFunction:
                case T_RangeTblRef:
                case T_SortGroupClause:
                        /* primitive node types with no expression subnodes */
@@ -2461,12 +2461,12 @@ expression_tree_mutator(Node *node,
                        }
                        break;
                case T_Param:
-               case T_CoerceToDomainValue:
                case T_CaseTestExpr:
+               case T_SQLValueFunction:
+               case T_CoerceToDomainValue:
                case T_SetToDefault:
                case T_CurrentOfExpr:
                case T_NextValueExpr:
-               case T_SQLValueFunction:
                case T_RangeTblRef:
                case T_SortGroupClause:
                        return (Node *) copyObject(node);
index b0abe9ec10ff1a2a5ca3a39dee7105645830531c..21e39a0b810b7d13d610eb2e0d74fff0fad50446 100644 (file)
@@ -1610,6 +1610,15 @@ _outCurrentOfExpr(StringInfo str, const CurrentOfExpr *node)
        WRITE_INT_FIELD(cursor_param);
 }
 
+static void
+_outNextValueExpr(StringInfo str, const NextValueExpr *node)
+{
+       WRITE_NODE_TYPE("NEXTVALUEEXPR");
+
+       WRITE_OID_FIELD(seqid);
+       WRITE_OID_FIELD(typeId);
+}
+
 static void
 _outInferenceElem(StringInfo str, const InferenceElem *node)
 {
@@ -3872,6 +3881,9 @@ outNode(StringInfo str, const void *obj)
                        case T_CurrentOfExpr:
                                _outCurrentOfExpr(str, obj);
                                break;
+                       case T_NextValueExpr:
+                               _outNextValueExpr(str, obj);
+                               break;
                        case T_InferenceElem:
                                _outInferenceElem(str, obj);
                                break;
index 1380703cbc661aa89c7cab2c39200fe740e660fd..8ab09d74d604d8609de276b9af4e097ffab5392a 100644 (file)
@@ -1202,6 +1202,20 @@ _readCurrentOfExpr(void)
        READ_DONE();
 }
 
+/*
+ * _readNextValueExpr
+ */
+static NextValueExpr *
+_readNextValueExpr(void)
+{
+       READ_LOCALS(NextValueExpr);
+
+       READ_OID_FIELD(seqid);
+       READ_OID_FIELD(typeId);
+
+       READ_DONE();
+}
+
 /*
  * _readInferenceElem
  */
@@ -2517,6 +2531,8 @@ parseNodeString(void)
                return_value = _readSetToDefault();
        else if (MATCH("CURRENTOFEXPR", 13))
                return_value = _readCurrentOfExpr();
+       else if (MATCH("NEXTVALUEEXPR", 13))
+               return_value = _readNextValueExpr();
        else if (MATCH("INFERENCEELEM", 13))
                return_value = _readInferenceElem();
        else if (MATCH("TARGETENTRY", 11))
index eb653cf3bec622f555956051e1fdd0f5c3aec80f..b35acb7bdcf17d2ba3baa728827f55f1449a8905 100644 (file)
@@ -3627,6 +3627,15 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
                                cpu_operator_cost;
                }
        }
+       else if (IsA(node, MinMaxExpr) ||
+                        IsA(node, SQLValueFunction) ||
+                        IsA(node, XmlExpr) ||
+                        IsA(node, CoerceToDomain) ||
+                        IsA(node, NextValueExpr))
+       {
+               /* Treat all these as having cost 1 */
+               context->total.per_tuple += cpu_operator_cost;
+       }
        else if (IsA(node, CurrentOfExpr))
        {
                /* Report high cost to prevent selection of anything but TID scan */
index 8961ed88a814249da1e38c7f1a7c0c751528a21c..8b4425dcf90ddd5762bd2881c262b059b683c2e8 100644 (file)
@@ -902,6 +902,12 @@ contain_mutable_functions_walker(Node *node, void *context)
                return true;
        }
 
+       if (IsA(node, NextValueExpr))
+       {
+               /* NextValueExpr is volatile */
+               return true;
+       }
+
        /*
         * It should be safe to treat MinMaxExpr as immutable, because it will
         * depend on a non-cross-type btree comparison function, and those should
@@ -969,6 +975,12 @@ contain_volatile_functions_walker(Node *node, void *context)
                                                                context))
                return true;
 
+       if (IsA(node, NextValueExpr))
+       {
+               /* NextValueExpr is volatile */
+               return true;
+       }
+
        /*
         * See notes in contain_mutable_functions_walker about why we treat
         * MinMaxExpr, XmlExpr, and CoerceToDomain as immutable, while
@@ -1019,6 +1031,8 @@ contain_volatile_functions_not_nextval_walker(Node *node, void *context)
         * See notes in contain_mutable_functions_walker about why we treat
         * MinMaxExpr, XmlExpr, and CoerceToDomain as immutable, while
         * SQLValueFunction is stable.  Hence, none of them are of interest here.
+        * Also, since we're intentionally ignoring nextval(), presumably we
+        * should ignore NextValueExpr.
         */
 
        /* Recurse to check arguments */
@@ -1146,7 +1160,7 @@ max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context)
         * contain a parallel-unsafe function; but useful constraints probably
         * never would have such, and assuming they do would cripple use of
         * parallel query in the presence of domain types.)  SQLValueFunction
-        * should be safe in all cases.
+        * should be safe in all cases.  NextValueExpr is parallel-unsafe.
         */
        if (IsA(node, CoerceToDomain))
        {
@@ -1154,6 +1168,12 @@ max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context)
                        return true;
        }
 
+       if (IsA(node, NextValueExpr))
+       {
+               if (max_parallel_hazard_test(PROPARALLEL_UNSAFE, context))
+                       return true;
+       }
+
        /*
         * As a notational convenience for callers, look through RestrictInfo.
         */
@@ -1495,6 +1515,7 @@ contain_leaked_vars_walker(Node *node, void *context)
                case T_SQLValueFunction:
                case T_NullTest:
                case T_BooleanTest:
+               case T_NextValueExpr:
                case T_List:
 
                        /*
index 5cfb916684f19d3c109be5d5ed4619f6e80c11cb..d2fb20d5649bbd7365a2dc9e26eccdf83975ad3d 100644 (file)
@@ -7283,6 +7283,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
                case T_MinMaxExpr:
                case T_SQLValueFunction:
                case T_XmlExpr:
+               case T_NextValueExpr:
                case T_NullIfExpr:
                case T_Aggref:
                case T_WindowFunc:
@@ -8612,6 +8613,22 @@ get_rule_expr(Node *node, deparse_context *context,
                        }
                        break;
 
+               case T_NextValueExpr:
+                       {
+                               NextValueExpr *nvexpr = (NextValueExpr *) node;
+
+                               /*
+                                * This isn't exactly nextval(), but that seems close enough
+                                * for EXPLAIN's purposes.
+                                */
+                               appendStringInfoString(buf, "nextval(");
+                               simple_quote_literal(buf,
+                                                                        generate_relation_name(nvexpr->seqid,
+                                                                                                                       NIL));
+                               appendStringInfoChar(buf, ')');
+                       }
+                       break;
+
                case T_InferenceElem:
                        {
                                InferenceElem *iexpr = (InferenceElem *) node;
index 7a65339f01ac422c3e4df73698f64634f5d11608..8ee0496e0106b6b06c7d1fb3ae5ad8d34b32a512 100644 (file)
@@ -362,7 +362,7 @@ typedef struct ExprEvalStep
                        SQLValueFunction *svf;
                }                       sqlvaluefunction;
 
-               /* for EEOP_NEXTVALUEXPR */
+               /* for EEOP_NEXTVALUEEXPR */
                struct
                {
                        Oid                     seqid;
@@ -615,6 +615,7 @@ extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
                                        ExprContext *econtext);
 extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op);
 extern void ExecEvalCurrentOfExpr(ExprState *state, ExprEvalStep *op);
+extern void ExecEvalNextValueExpr(ExprState *state, ExprEvalStep *op);
 extern void ExecEvalRowNull(ExprState *state, ExprEvalStep *op,
                                ExprContext *econtext);
 extern void ExecEvalRowNotNull(ExprState *state, ExprEvalStep *op,
index 01527399b80f3ecd7d135baa9df7b69dbdde5ad7..27bd4f3363e2467bdb006660e1e89daeb7d9e379 100644 (file)
@@ -183,6 +183,7 @@ typedef enum NodeTag
        T_CoerceToDomainValue,
        T_SetToDefault,
        T_CurrentOfExpr,
+       T_NextValueExpr,
        T_InferenceElem,
        T_TargetEntry,
        T_RangeTblRef,
@@ -190,7 +191,6 @@ typedef enum NodeTag
        T_FromExpr,
        T_OnConflictExpr,
        T_IntoClause,
-       T_NextValueExpr,
 
        /*
         * TAGS FOR EXPRESSION STATE NODES (execnodes.h)
index 38015ed540594c5b60abeeccec9a6320fa99858f..8c536a8d38d8c2fb811b4d481125d6be4c522a33 100644 (file)
@@ -1279,6 +1279,20 @@ typedef struct CurrentOfExpr
        int                     cursor_param;   /* refcursor parameter number, or 0 */
 } CurrentOfExpr;
 
+/*
+ * NextValueExpr - get next value from sequence
+ *
+ * This has the same effect as calling the nextval() function, but it does not
+ * check permissions on the sequence.  This is used for identity columns,
+ * where the sequence is an implicit dependency without its own permissions.
+ */
+typedef struct NextValueExpr
+{
+       Expr            xpr;
+       Oid                     seqid;
+       Oid                     typeId;
+} NextValueExpr;
+
 /*
  * InferenceElem - an element of a unique index inference specification
  *
@@ -1294,20 +1308,6 @@ typedef struct InferenceElem
        Oid                     inferopclass;   /* OID of att opclass, or InvalidOid */
 } InferenceElem;
 
-/*
- * NextValueExpr - get next value from sequence
- *
- * This has the same effect as calling the nextval() function, but it does not
- * check permissions on the sequence.  This is used for identity columns,
- * where the sequence is an implicit dependency without its own permissions.
- */
-typedef struct NextValueExpr
-{
-       Expr            xpr;
-       Oid                     seqid;
-       Oid                     typeId;
-} NextValueExpr;
-
 /*--------------------
  * TargetEntry -
  *        a target entry (used in query target lists)