On 2013-06-21 16:45:28 +0200, Andres Freund wrote:
> On 2013-06-21 09:51:05 -0400, Noah Misch wrote:
> > That being said, if we discover a simple-enough fix that performs well, we
> > may
> > as well incorporate it.
>
> What about passing another parameter down eval_const_expressions_mutator
> (which is static, so changing the API isn't a problem) that basically
> tells us whether we actually should evaluate expressions or just perform
> the transformation?
> There's seems to be basically a couple of places where we call dangerous
> things:
> * simplify_function (via ->evaluate_function->evaluate_expr) which is
> called in a bunch of places
> * evaluate_expr which is directly called in T_ArrayExpr
> T_ArrayCoerceExpr
>
> All places I've inspected so far need to deal with simplify_function
> returning NULL anyway, so that seems like a viable fix.
*Prototype* patch - that seems simple enough - attached. Opinions?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 1349f8e4a8133eebbf66753b1aa3787f88ee3e33 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Sat, 22 Jun 2013 16:53:39 +0200
Subject: [PATCH] Don't evaluate potentially unreachable parts of CASE during
planning
---
src/backend/optimizer/util/clauses.c | 31 +++++++++++++++++++++++++++++--
src/test/regress/expected/case.out | 13 ++++++++++---
src/test/regress/sql/case.sql | 4 ++--
3 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 6d5b204..94b52f6 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -63,6 +63,7 @@ typedef struct
List *active_fns;
Node *case_val;
bool estimate;
+ bool doevil;
} eval_const_expressions_context;
typedef struct
@@ -2202,6 +2203,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.doevil = true; /* allowed to evaluate const expressions */
return eval_const_expressions_mutator(node, &context);
}
@@ -2233,6 +2235,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.doevil = true; /* allowed to evaluate const expressions */
return eval_const_expressions_mutator(node, &context);
}
@@ -2751,7 +2754,7 @@ eval_const_expressions_mutator(Node *node,
* If constant argument and it's a binary-coercible or
* immutable conversion, we can simplify it to a constant.
*/
- if (arg && IsA(arg, Const) &&
+ if (context->doevil && arg && IsA(arg, Const) &&
(!OidIsValid(newexpr->elemfuncid) ||
func_volatile(newexpr->elemfuncid) == PROVOLATILE_IMMUTABLE))
return (Node *) evaluate_expr((Expr *) newexpr,
@@ -2843,16 +2846,20 @@ eval_const_expressions_mutator(Node *node,
CaseExpr *caseexpr = (CaseExpr *) node;
CaseExpr *newcase;
Node *save_case_val;
+ bool save_doevil;
Node *newarg;
List *newargs;
bool const_true_cond;
Node *defresult = NULL;
ListCell *arg;
+ save_doevil = context->doevil;
+
/* Simplify the test expression, if any */
newarg = eval_const_expressions_mutator((Node *) caseexpr->arg,
context);
+
/* Set up for contained CaseTestExpr nodes */
save_case_val = context->case_val;
if (newarg && IsA(newarg, Const))
@@ -2894,6 +2901,17 @@ eval_const_expressions_mutator(Node *node,
const_true_cond = true;
}
+ /*
+ * We can only evaluate expressions as long we are sure the
+ * the expression will be reached at runtime - otherwise we
+ * might cause spurious errors to be thrown. The first
+ * eval_const_expression above can always evaluate
+ * expressions (unless we are in a nested CaseExpr) since
+ * it will always be reached at runtime.
+ */
+ if (!const_true_cond)
+ context->doevil = false;
+
/* Simplify this alternative's result value */
caseresult = eval_const_expressions_mutator((Node *) oldcasewhen->result,
context);
@@ -2926,6 +2944,8 @@ eval_const_expressions_mutator(Node *node,
context->case_val = save_case_val;
+ context->doevil = save_doevil;
+
/*
* If no non-FALSE alternatives, CASE reduces to the default
* result
@@ -2982,7 +3002,7 @@ eval_const_expressions_mutator(Node *node,
newarray->multidims = arrayexpr->multidims;
newarray->location = arrayexpr->location;
- if (all_const)
+ if (all_const && context->doevil)
return (Node *) evaluate_expr((Expr *) newarray,
newarray->array_typeid,
exprTypmod(node),
@@ -3940,6 +3960,13 @@ evaluate_function(Oid funcid, Oid result_type, int32 result_typmod,
result_collid);
/*
+ * don't evaluate if we aren't allowed to - only check here because it's
+ * legal to optimize cases like the above strict optimization.
+ */
+ if (!context->doevil)
+ return NULL;
+
+ /*
* Otherwise, can simplify only if all inputs are constants. (For a
* non-strict function, constant NULL inputs are treated the same as
* constant non-NULL inputs.)
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c564eed..84046a8 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -85,10 +85,17 @@ 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
+-- Constant-expression folding shouldn't evaluate potentially reachable
+-- subexpressions
SELECT CASE WHEN i > 100 THEN 1/0 ELSE 0 END FROM case_tbl;
-ERROR: division by zero
+ case
+------
+ 0
+ 0
+ 0
+ 0
+(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/sql/case.sql b/src/test/regress/sql/case.sql
index 5f41753..179c3cb 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -62,8 +62,8 @@ SELECT '6' AS "One",
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
+-- Constant-expression folding shouldn't evaluate potentially reachable
+-- subexpressions
SELECT CASE WHEN i > 100 THEN 1/0 ELSE 0 END FROM case_tbl;
-- Test for cases involving untyped literals in test expression
--
1.8.3.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
