Andres Freund <and...@anarazel.de> writes:
> I'm a bit worried about a case like:
> CREATE FUNCTION yell(int, int)
> RETURNS int
> IMMUTABLE
> LANGUAGE SQL AS $$
> SELECT CASE WHEN $1 != 0 THEN 17 / $2 ELSE NULL END
> $$;
> EXPLAIN SELECT yell(g.i, 0) FROM generate_series(1, 10) g(i);
> I don't think the parameters here would have been handled before
> inlining, right?
Ah, I see what you mean. Yeah, that throws an error today, and it
still would with the patch I was envisioning (attached), because
inlining does Param substitution in a different way. I'm not
sure that we could realistically fix the inlining case with this
sort of approach.
I think this bears out the comment I made before that this approach
still leaves us with a very complicated behavior. Maybe we should
stick with the previous approach, possibly supplemented with a
leakproofness exception.
regards, tom lane
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index e04b144072..4ff7a69908 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -61,6 +61,13 @@ typedef struct
AggClauseCosts *costs;
} get_agg_clause_costs_context;
+typedef enum
+{
+ ece_param_never, /* never inject values for Params */
+ ece_param_const, /* inject values for Params if marked CONST */
+ ece_param_always /* always inject values for Params */
+} ece_p_mode;
+
typedef struct
{
ParamListInfo boundParams;
@@ -68,6 +75,7 @@ typedef struct
List *active_fns;
Node *case_val;
bool estimate;
+ ece_p_mode param_mode;
} eval_const_expressions_context;
typedef struct
@@ -2264,6 +2272,7 @@ eval_const_expressions(PlannerInfo *root, Node *node)
context.active_fns = NIL; /* nothing being recursively simplified */
context.case_val = NULL; /* no CASE being examined */
context.estimate = false; /* safe transformations only */
+ context.param_mode = ece_param_const; /* inject only CONST Params */
return eval_const_expressions_mutator(node, &context);
}
@@ -2280,8 +2289,11 @@ eval_const_expressions(PlannerInfo *root, Node *node)
* available by the caller of planner(), even if the Param isn't marked
* constant. This effectively means that we plan using the first supplied
* value of the Param.
- * 2. Fold stable, as well as immutable, functions to constants.
+ * 2. Fold stable, as well as immutable, functions to constants. The risk
+ * that the result might change from planning time to execution time is
+ * worth taking, as we otherwise couldn't get an estimate at all.
* 3. Reduce PlaceHolderVar nodes to their contained expressions.
+ * 4. Ignore domain constraints, assuming that CoerceToDomain will succeed.
*--------------------
*/
Node *
@@ -2295,6 +2307,7 @@ estimate_expression_value(PlannerInfo *root, Node *node)
context.active_fns = NIL; /* nothing being recursively simplified */
context.case_val = NULL; /* no CASE being examined */
context.estimate = true; /* unsafe transformations OK */
+ context.param_mode = ece_param_always; /* inject all Param values */
return eval_const_expressions_mutator(node, &context);
}
@@ -2372,8 +2385,22 @@ eval_const_expressions_mutator(Node *node,
prm->ptype == param->paramtype)
{
/* OK to substitute parameter value? */
- if (context->estimate ||
- (prm->pflags & PARAM_FLAG_CONST))
+ bool subst = false;
+
+ switch (context->param_mode)
+ {
+ case ece_param_never:
+ /* subst is already false */
+ break;
+ case ece_param_const:
+ subst = (prm->pflags & PARAM_FLAG_CONST) != 0;
+ break;
+ case ece_param_always:
+ subst = true;
+ break;
+ }
+
+ if (subst)
{
/*
* Return a Const representing the param value.
@@ -2973,10 +3000,26 @@ eval_const_expressions_mutator(Node *node,
* expression when executing the CASE, since any contained
* CaseTestExprs that might have referred to it will have been
* replaced by the constant.
+ *
+ * An additional consideration is that the user might be
+ * expecting the CASE to prevent run-time errors, as in
+ * CASE ... THEN 1/$1 ELSE ...
+ * If a constant value of zero is available for $1, we would
+ * end up trying to simplify the division, thus drawing a
+ * divide-by-zero error even if this CASE result would not be
+ * reached at runtime. This is problematic because it can
+ * occur even if the user has not written anything as silly as
+ * a constant "1/0" expression: substitution of values for
+ * Params is standard in plpgsql, for instance. To ameliorate
+ * this problem without giving up const-simplification of CASE
+ * subexpressions entirely, we prevent substitution of Param
+ * values within test condition or result expressions that
+ * will not certainly be evaluated at run-time.
*----------
*/
CaseExpr *caseexpr = (CaseExpr *) node;
CaseExpr *newcase;
+ ece_p_mode save_param_mode = context->param_mode;
Node *save_case_val;
Node *newarg;
List *newargs;
@@ -3027,6 +3070,15 @@ eval_const_expressions_mutator(Node *node,
const_true_cond = true;
}
+ /*
+ * Unless the test condition is constant TRUE, we can't be
+ * sure the result value will be evaluated, so back off
+ * the Param safety level. This change will also apply to
+ * subsequent test conditions and result values.
+ */
+ if (!const_true_cond)
+ context->param_mode = ece_param_never;
+
/* Simplify this alternative's result value */
caseresult = eval_const_expressions_mutator((Node *) oldcasewhen->result,
context);
@@ -3058,6 +3110,7 @@ eval_const_expressions_mutator(Node *node,
context);
context->case_val = save_case_val;
+ context->param_mode = save_param_mode;
/*
* If no non-FALSE alternatives, CASE reduces to the default
@@ -3113,6 +3166,7 @@ eval_const_expressions_mutator(Node *node,
{
CoalesceExpr *coalesceexpr = (CoalesceExpr *) node;
CoalesceExpr *newcoalesce;
+ ece_p_mode save_param_mode = context->param_mode;
List *newargs;
ListCell *arg;
@@ -3137,13 +3191,25 @@ eval_const_expressions_mutator(Node *node,
if (((Const *) e)->constisnull)
continue; /* drop null constant */
if (newargs == NIL)
+ {
+ context->param_mode = save_param_mode;
return e; /* first expr */
+ }
newargs = lappend(newargs, e);
break;
}
newargs = lappend(newargs, e);
+
+ /*
+ * Arguments following a non-constant argument may or may
+ * not get evaluated at run-time. As in CASE expressions,
+ * avoid substituting Param values within such arguments.
+ */
+ context->param_mode = ece_param_never;
}
+ context->param_mode = save_param_mode;
+
/*
* If all the arguments were constant null, the result is just
* null
