Re: Making CASE error handling less surprising

Andres Freund <andres@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