I wrote:
> 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.
Here's another example that we can't possibly fix with Param substitution
hacking, because there are no Params involved in the first place:
select f1, case when f1 = 42 then 1/i else null end
from (select f1, 0 as i from int4_tbl) ss;
Pulling up the subquery results in "1/0", so this fails today,
even though "f1 = 42" is never true.
Attached is a v3 patch that incorporates the leakproofness idea.
As shown in the new case.sql tests, this does fix both the SQL
function and subquery-pullup cases.
To keep the join regression test results the same, I marked int48()
as leakproof, which is surely safe enough. Probably we should make
a push to mark all unconditionally-safe implicit coercions as
leakproof, but that's a separate matter.
regards, tom lane
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index e04b144072..48e31b16d6 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -61,6 +61,16 @@ typedef struct
AggClauseCosts *costs;
} get_agg_clause_costs_context;
+typedef enum
+{
+ /* Ordering is important here! */
+ ece_eval_nothing, /* be unconditionally safe */
+ ece_eval_leakproof, /* eval leakproof immutable functions */
+ ece_eval_immutable, /* eval immutable functions too */
+ ece_eval_stable, /* eval stable functions too */
+ ece_eval_volatile /* eval volatile functions too */
+} ece_eval_mode;
+
typedef struct
{
ParamListInfo boundParams;
@@ -68,6 +78,7 @@ typedef struct
List *active_fns;
Node *case_val;
bool estimate;
+ ece_eval_mode eval_mode;
} eval_const_expressions_context;
typedef struct
@@ -119,6 +130,8 @@ static Node *eval_const_expressions_mutator(Node *node,
static bool contain_non_const_walker(Node *node, void *context);
static bool ece_function_is_safe(Oid funcid,
eval_const_expressions_context *context);
+static bool ece_function_form_is_safe(Form_pg_proc func_form,
+ eval_const_expressions_context *context);
static Node *apply_const_relabel(Node *arg, Oid rtype,
int32 rtypmod, Oid rcollid,
CoercionForm rformat, int rlocation);
@@ -2264,6 +2277,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.eval_mode = ece_eval_immutable; /* eval immutable functions */
return eval_const_expressions_mutator(node, &context);
}
@@ -2280,8 +2294,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 +2312,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.eval_mode = ece_eval_stable; /* eval stable functions */
return eval_const_expressions_mutator(node, &context);
}
@@ -2961,7 +2979,22 @@ eval_const_expressions_mutator(Node *node,
* opportunity to reduce constant test conditions. For
* example this allows
* CASE 0 WHEN 0 THEN 1 ELSE 1/0 END
- * to reduce to 1 rather than drawing a divide-by-0 error.
+ * to reduce to 1 rather than drawing a divide-by-0 error,
+ * since we'll throw away the ELSE without processing it.
+ *
+ * Even in not-all-constant cases, the user might be expecting
+ * the CASE to prevent run-time errors, for example in
+ * CASE WHEN j > 0 THEN 1 ELSE 1/0 END
+ * Since division is immutable, we'd ordinarily simplify the
+ * division and hence draw the divide-by-zero error at plan
+ * time, even if j is never zero at run time. To avoid that,
+ * reduce eval_mode to ece_eval_leakproof while processing any
+ * test condition or result value that will not certainly be
+ * evaluated at run-time. We expect that leakproof immutable
+ * functions will not throw any errors (except perhaps in
+ * corner cases such as OOM, which we need not guard against
+ * for this purpose).
+ *
* Note that when the test expression is constant, we don't
* have to include it in the resulting CASE; for example
* CASE 0 WHEN x THEN y ELSE z END
@@ -2977,6 +3010,7 @@ eval_const_expressions_mutator(Node *node,
*/
CaseExpr *caseexpr = (CaseExpr *) node;
CaseExpr *newcase;
+ ece_eval_mode save_eval_mode = context->eval_mode;
Node *save_case_val;
Node *newarg;
List *newargs;
@@ -3027,6 +3061,16 @@ 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 become more
+ * conservative about which functions to execute. This
+ * change will also apply to subsequent test conditions
+ * and result values.
+ */
+ if (!const_true_cond)
+ context->eval_mode = ece_eval_leakproof;
+
/* Simplify this alternative's result value */
caseresult = eval_const_expressions_mutator((Node *) oldcasewhen->result,
context);
@@ -3058,6 +3102,7 @@ eval_const_expressions_mutator(Node *node,
context);
context->case_val = save_case_val;
+ context->eval_mode = save_eval_mode;
/*
* If no non-FALSE alternatives, CASE reduces to the default
@@ -3113,6 +3158,7 @@ eval_const_expressions_mutator(Node *node,
{
CoalesceExpr *coalesceexpr = (CoalesceExpr *) node;
CoalesceExpr *newcoalesce;
+ ece_eval_mode save_eval_mode = context->eval_mode;
List *newargs;
ListCell *arg;
@@ -3137,13 +3183,25 @@ eval_const_expressions_mutator(Node *node,
if (((Const *) e)->constisnull)
continue; /* drop null constant */
if (newargs == NIL)
+ {
+ context->eval_mode = save_eval_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, so as in CASE, become
+ * more conservative about which functions to execute.
+ */
+ context->eval_mode = ece_eval_leakproof;
}
+ context->eval_mode = save_eval_mode;
+
/*
* If all the arguments were constant null, the result is just
* null
@@ -3163,13 +3221,12 @@ eval_const_expressions_mutator(Node *node,
case T_SQLValueFunction:
{
/*
- * All variants of SQLValueFunction are stable, so if we are
- * estimating the expression's value, we should evaluate the
- * current function value. Otherwise just copy.
+ * All variants of SQLValueFunction are stable, so evaluate if
+ * we are evaluating stable functions. Otherwise just copy.
*/
SQLValueFunction *svf = (SQLValueFunction *) node;
- if (context->estimate)
+ if (context->eval_mode >= ece_eval_stable)
return (Node *) evaluate_expr((Expr *) svf,
svf->type,
svf->typmod,
@@ -3565,20 +3622,42 @@ contain_non_const_walker(Node *node, void *context)
static bool
ece_function_is_safe(Oid funcid, eval_const_expressions_context *context)
{
- char provolatile = func_volatile(funcid);
+ bool result;
+ HeapTuple tp;
- /*
- * Ordinarily we are only allowed to simplify immutable functions. But for
- * purposes of estimation, we consider it okay to simplify functions that
- * are merely stable; the risk that the result might change from planning
- * time to execution time is worth taking in preference to not being able
- * to estimate the value at all.
- */
- if (provolatile == PROVOLATILE_IMMUTABLE)
- return true;
- if (context->estimate && provolatile == PROVOLATILE_STABLE)
- return true;
- return false;
+ tp = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+ if (!HeapTupleIsValid(tp))
+ elog(ERROR, "cache lookup failed for function %u", funcid);
+
+ result = ece_function_form_is_safe((Form_pg_proc) GETSTRUCT(tp), context);
+ ReleaseSysCache(tp);
+ return result;
+}
+
+/*
+ * Same, when we have the pg_proc entry directly at hand
+ */
+static bool
+ece_function_form_is_safe(Form_pg_proc func_form,
+ eval_const_expressions_context *context)
+{
+ ece_eval_mode f_mode;
+
+ /* Must map the function properties to an ordered enum */
+ if (func_form->provolatile == PROVOLATILE_IMMUTABLE)
+ {
+ if (func_form->proleakproof)
+ f_mode = ece_eval_leakproof;
+ else
+ f_mode = ece_eval_immutable;
+ }
+ else if (func_form->provolatile == PROVOLATILE_STABLE)
+ f_mode = ece_eval_stable;
+ else
+ f_mode = ece_eval_volatile;
+
+ /* Now, does eval_mode allow evaluation of this function? */
+ return (context->eval_mode >= f_mode);
}
/*
@@ -4238,9 +4317,8 @@ recheck_cast_function_args(List *args, Oid result_type, HeapTuple func_tuple)
* evaluate_function: try to pre-evaluate a function call
*
* We can do this if the function is strict and has any constant-null inputs
- * (just return a null constant), or if the function is immutable and has all
- * constant inputs (call it and return the result as a Const node). In
- * estimation mode we are willing to pre-evaluate stable functions too.
+ * (just return a null constant), or if the function is safe to evaluate and
+ * has all constant inputs (call it and return the result as a Const node).
*
* Returns a simplified expression if successful, or NULL if cannot
* simplify the function.
@@ -4293,7 +4371,7 @@ evaluate_function(Oid funcid, Oid result_type, int32 result_typmod,
* If the function is strict and has a constant-NULL input, it will never
* be called at all, so we can replace the call by a NULL constant, even
* if there are other inputs that aren't constant, and even if the
- * function is not otherwise immutable.
+ * function is not otherwise safe to evaluate.
*/
if (funcform->proisstrict && has_null_input)
return (Expr *) makeNullConst(result_type, result_typmod,
@@ -4308,17 +4386,9 @@ evaluate_function(Oid funcid, Oid result_type, int32 result_typmod,
return NULL;
/*
- * Ordinarily we are only allowed to simplify immutable functions. But for
- * purposes of estimation, we consider it okay to simplify functions that
- * are merely stable; the risk that the result might change from planning
- * time to execution time is worth taking in preference to not being able
- * to estimate the value at all.
+ * Are we permitted to evaluate this function in the current context?
*/
- if (funcform->provolatile == PROVOLATILE_IMMUTABLE)
- /* okay */ ;
- else if (context->estimate && funcform->provolatile == PROVOLATILE_STABLE)
- /* okay */ ;
- else
+ if (!ece_function_form_is_safe(funcform, context))
return NULL;
/*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 4b5af32440..5a3fdcc136 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1338,7 +1338,7 @@
proname => 'int4', prorettype => 'int4', proargtypes => 'int8',
prosrc => 'int84' },
{ oid => '481', descr => 'convert int4 to int8',
- proname => 'int8', prorettype => 'int8', proargtypes => 'int4',
+ proname => 'int8', proleakproof => 't', prorettype => 'int8', proargtypes => 'int4',
prosrc => 'int48' },
{ oid => '482', descr => 'convert int8 to float8',
proname => 'float8', prorettype => 'float8', proargtypes => 'int8',
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..0b23057ee4 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -93,10 +93,62 @@ SELECT CASE 1 WHEN 0 THEN 1/0 WHEN 1 THEN 1 ELSE 2/0 END;
1
(1 row)
--- However we do not currently suppress folding of potentially
--- reachable subexpressions
SELECT CASE WHEN i > 100 THEN 1/0 ELSE 0 END FROM case_tbl;
+ case
+------
+ 0
+ 0
+ 0
+ 0
+(4 rows)
+
+-- However, that guarantee doesn't extend into sub-selects
+SELECT CASE WHEN i > 100 THEN (select 1/0) ELSE 0 END FROM case_tbl;
ERROR: division by zero
+-- Inline-ing a SQL function shouldn't cause a failure
+CREATE FUNCTION case_func(double precision, integer)
+RETURNS integer
+LANGUAGE SQL AS $$
+ SELECT CASE WHEN $1 = 42 THEN 1/$2 ELSE NULL END
+$$;
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT case_func(f, 0) FROM case_tbl;
+ QUERY PLAN
+--------------------------------------------------------------------------------------
+ Seq Scan on public.case_tbl
+ Output: CASE WHEN (f = '42'::double precision) THEN (1 / 0) ELSE NULL::integer END
+(2 rows)
+
+SELECT case_func(f, 0) FROM case_tbl;
+ case_func
+-----------
+
+
+
+
+(4 rows)
+
+DROP FUNCTION case_func(double precision, integer);
+-- Subquery flattening shouldn't result in such errors, either
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT f, CASE WHEN f = 42 THEN 1/j ELSE NULL END
+FROM (SELECT f, 0 AS j FROM case_tbl) ss;
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Seq Scan on public.case_tbl
+ Output: case_tbl.f, CASE WHEN (case_tbl.f = '42'::double precision) THEN (1 / 0) ELSE NULL::integer END
+(2 rows)
+
+SELECT f, CASE WHEN f = 42 THEN 1/j ELSE NULL END
+FROM (SELECT f, 0 AS j FROM case_tbl) ss;
+ f | case
+-------+------
+ 10.1 |
+ 20.2 |
+ -30.3 |
+ |
+(4 rows)
+
-- Test for cases involving untyped literals in test expression
SELECT CASE 'a' WHEN 'a' THEN 1 ELSE 2 END;
case
diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out
index 27056d70d3..7b576b0208 100644
--- a/src/test/regress/expected/opr_sanity.out
+++ b/src/test/regress/expected/opr_sanity.out
@@ -634,6 +634,7 @@ int84lt(bigint,integer)
int84gt(bigint,integer)
int84le(bigint,integer)
int84ge(bigint,integer)
+int8(integer)
oidvectorne(oidvector,oidvector)
namelt(name,name)
namele(name,name)
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..07ee4f8651 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -66,11 +66,33 @@ SELECT '7' AS "None",
-- Constant-expression folding shouldn't evaluate unreachable subexpressions
SELECT CASE WHEN 1=0 THEN 1/0 WHEN 1=1 THEN 1 ELSE 2/0 END;
SELECT CASE 1 WHEN 0 THEN 1/0 WHEN 1 THEN 1 ELSE 2/0 END;
-
--- However we do not currently suppress folding of potentially
--- reachable subexpressions
SELECT CASE WHEN i > 100 THEN 1/0 ELSE 0 END FROM case_tbl;
+-- However, that guarantee doesn't extend into sub-selects
+SELECT CASE WHEN i > 100 THEN (select 1/0) ELSE 0 END FROM case_tbl;
+
+-- Inline-ing a SQL function shouldn't cause a failure
+CREATE FUNCTION case_func(double precision, integer)
+RETURNS integer
+LANGUAGE SQL AS $$
+ SELECT CASE WHEN $1 = 42 THEN 1/$2 ELSE NULL END
+$$;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT case_func(f, 0) FROM case_tbl;
+
+SELECT case_func(f, 0) FROM case_tbl;
+
+DROP FUNCTION case_func(double precision, integer);
+
+-- Subquery flattening shouldn't result in such errors, either
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT f, CASE WHEN f = 42 THEN 1/j ELSE NULL END
+FROM (SELECT f, 0 AS j FROM case_tbl) ss;
+
+SELECT f, CASE WHEN f = 42 THEN 1/j ELSE NULL END
+FROM (SELECT f, 0 AS j FROM case_tbl) ss;
+
-- Test for cases involving untyped literals in test expression
SELECT CASE 'a' WHEN 'a' THEN 1 ELSE 2 END;
