Thread: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr
Eval_const_expressions() doesn't know about ScalarArrayOpExpr. We
simplify the arguments, but if all the arguments are booleans, we don't
take the obvious step of replacing the whole expression with a boolean
Const. For example:
postgres=# explain select * from foo where 1 IN (1,2,3);
QUERY PLAN
-------------------------------------------------------------
Result (cost=0.00..35.50 rows=2550 width=4)
One-Time Filter: (1 = ANY ('{1,2,3}'::integer[]))
-> Seq Scan on foo (cost=0.00..35.50 rows=2550 width=4)
(3 rows)
I would've expected that to be fully evaluated at plan-time, and
optimized away.
That seems like an oversight. I guess that scenario doesn't happen very
often in practice, but there's no reason not to do it when it does.
Patch attached.
On a side-note, I find it a bit awkward that ScalarArrayOpExpr uses a
2-element List to hold the scalar and array arguments. Wouldn't it be
much more straightforward to have explicit "Expr *scalararg" and "Expr
*arrayarg" fields?
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Eval_const_expressions() doesn't know about ScalarArrayOpExpr.
> ...
> That seems like an oversight. I guess that scenario doesn't happen very
> often in practice, but there's no reason not to do it when it does.
> Patch attached.
Yeah, I think it's a lack-of-round-tuits situation. Your patch reminds
me of a more ambitious attempt I made awhile back to reduce the amount
of duplicative boilerplate in eval_const_expressions. I think I wrote
it during last year's beta period, stashed it because I didn't feel like
arguing for applying it right then, and then forgot about it. Blowing
the dust off, it's attached. It fixes ArrayRef and RowExpr as well as
ScalarArrayOpExpr, with a net growth of only 20 lines (largely comments).
> On a side-note, I find it a bit awkward that ScalarArrayOpExpr uses a
> 2-element List to hold the scalar and array arguments. Wouldn't it be
> much more straightforward to have explicit "Expr *scalararg" and "Expr
> *arrayarg" fields?
I think it's modeled on OpExpr, which also uses a list though you could
argue for separate leftarg and rightarg fields instead.
regards, tom lane
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index a1dafc8..95489f4 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** static List *find_nonnullable_vars_walke
*** 115,120 ****
--- 115,123 ----
static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
static Node *eval_const_expressions_mutator(Node *node,
eval_const_expressions_context *context);
+ static bool contain_non_const_walker(Node *node, void *context);
+ static bool ece_function_is_safe(Oid funcid,
+ eval_const_expressions_context *context);
static List *simplify_or_arguments(List *args,
eval_const_expressions_context *context,
bool *haveNull, bool *forceTrue);
*************** estimate_expression_value(PlannerInfo *r
*** 2443,2448 ****
--- 2446,2482 ----
return eval_const_expressions_mutator(node, &context);
}
+ /*
+ * The generic case in eval_const_expressions_mutator is to recurse using
+ * expression_tree_mutator, which will copy the given node unchanged but
+ * const-simplify its arguments (if any) as far as possible. If the node
+ * itself does immutable processing, and each of its arguments were reduced
+ * to a Const, we can then reduce it to a Const using evaluate_expr. (Some
+ * node types need more complicated logic; for example, a CASE expression
+ * might be reducible to a constant even if not all its subtrees are.)
+ */
+ #define ece_generic_processing(node) \
+ expression_tree_mutator((Node *) (node), eval_const_expressions_mutator, \
+ (void *) context)
+
+ /*
+ * Check whether all arguments of the given node were reduced to Consts.
+ * By going directly to expression_tree_walker, contain_non_const_walker
+ * is not applied to the node itself, only to its children.
+ */
+ #define ece_all_arguments_const(node) \
+ (!expression_tree_walker((Node *) (node), contain_non_const_walker, NULL))
+
+ /* Generic macro for applying evaluate_expr */
+ #define ece_evaluate_expr(node) \
+ ((Node *) evaluate_expr((Expr *) (node), \
+ exprType((Node *) (node)), \
+ exprTypmod((Node *) (node)), \
+ exprCollation((Node *) (node))))
+
+ /*
+ * Recursive guts of eval_const_expressions/estimate_expression_value
+ */
static Node *
eval_const_expressions_mutator(Node *node,
eval_const_expressions_context *context)
*************** eval_const_expressions_mutator(Node *nod
*** 2758,2763 ****
--- 2792,2816 ----
newexpr->location = expr->location;
return (Node *) newexpr;
}
+ case T_ScalarArrayOpExpr:
+ {
+ ScalarArrayOpExpr *saop;
+
+ /* Copy the node and const-simplify its arguments */
+ saop = (ScalarArrayOpExpr *) ece_generic_processing(node);
+
+ /* Make sure we know underlying function */
+ set_sa_opfuncid(saop);
+
+ /*
+ * If all arguments are Consts, and it's a safe function, we
+ * can fold to a constant
+ */
+ if (ece_all_arguments_const(saop) &&
+ ece_function_is_safe(saop->opfuncid, context))
+ return ece_evaluate_expr(saop);
+ return (Node *) saop;
+ }
case T_BoolExpr:
{
BoolExpr *expr = (BoolExpr *) node;
*************** eval_const_expressions_mutator(Node *nod
*** 2982,3022 ****
}
case T_ArrayCoerceExpr:
{
! ArrayCoerceExpr *expr = (ArrayCoerceExpr *) node;
! Expr *arg;
! ArrayCoerceExpr *newexpr;
!
! /*
! * Reduce constants in the ArrayCoerceExpr's argument, then
! * build a new ArrayCoerceExpr.
! */
! arg = (Expr *) eval_const_expressions_mutator((Node *) expr->arg,
! context);
! newexpr = makeNode(ArrayCoerceExpr);
! newexpr->arg = arg;
! newexpr->elemfuncid = expr->elemfuncid;
! newexpr->resulttype = expr->resulttype;
! newexpr->resulttypmod = expr->resulttypmod;
! newexpr->resultcollid = expr->resultcollid;
! newexpr->isExplicit = expr->isExplicit;
! newexpr->coerceformat = expr->coerceformat;
! newexpr->location = expr->location;
/*
! * If constant argument and it's a binary-coercible or
! * immutable conversion, we can simplify it to a constant.
*/
! if (arg && IsA(arg, Const) &&
! (!OidIsValid(newexpr->elemfuncid) ||
! func_volatile(newexpr->elemfuncid) == PROVOLATILE_IMMUTABLE))
! return (Node *) evaluate_expr((Expr *) newexpr,
! newexpr->resulttype,
! newexpr->resulttypmod,
! newexpr->resultcollid);
!
! /* Else we must return the partially-simplified node */
! return (Node *) newexpr;
}
case T_CollateExpr:
{
--- 3035,3054 ----
}
case T_ArrayCoerceExpr:
{
! ArrayCoerceExpr *acexpr;
! /* Copy the node and const-simplify its arguments */
! acexpr = (ArrayCoerceExpr *) ece_generic_processing(node);
/*
! * If all arguments are Consts, and it's a binary-coercible or
! * safe conversion function, we can fold to a constant
*/
! if (ece_all_arguments_const(acexpr) &&
! (!OidIsValid(acexpr->elemfuncid) ||
! ece_function_is_safe(acexpr->elemfuncid, context)))
! return ece_evaluate_expr(acexpr);
! return (Node *) acexpr;
}
case T_CollateExpr:
{
*************** eval_const_expressions_mutator(Node *nod
*** 3208,3248 ****
else
return copyObject(node);
}
case T_ArrayExpr:
{
! ArrayExpr *arrayexpr = (ArrayExpr *) node;
! ArrayExpr *newarray;
! bool all_const = true;
! List *newelems;
! ListCell *element;
!
! newelems = NIL;
! foreach(element, arrayexpr->elements)
! {
! Node *e;
!
! e = eval_const_expressions_mutator((Node *) lfirst(element),
! context);
! if (!IsA(e, Const))
! all_const = false;
! newelems = lappend(newelems, e);
! }
!
! newarray = makeNode(ArrayExpr);
! newarray->array_typeid = arrayexpr->array_typeid;
! newarray->array_collid = arrayexpr->array_collid;
! newarray->element_typeid = arrayexpr->element_typeid;
! newarray->elements = newelems;
! newarray->multidims = arrayexpr->multidims;
! newarray->location = arrayexpr->location;
!
! if (all_const)
! return (Node *) evaluate_expr((Expr *) newarray,
! newarray->array_typeid,
! exprTypmod(node),
! newarray->array_collid);
! return (Node *) newarray;
}
case T_CoalesceExpr:
{
--- 3240,3262 ----
else
return copyObject(node);
}
+ case T_ArrayRef:
case T_ArrayExpr:
+ case T_RowExpr:
+ case T_BooleanTest:
{
! /*
! * Generic handling for node types whose own processing is
! * known to be immutable, and for which we need no smarts
! * beyond "simplify if all inputs are constants".
! */
! /* Copy the node and const-simplify its arguments */
! node = ece_generic_processing(node);
! /* If all arguments are Consts, we can fold to a constant */
! if (ece_all_arguments_const(node))
! return ece_evaluate_expr(node);
! return node;
}
case T_CoalesceExpr:
{
*************** eval_const_expressions_mutator(Node *nod
*** 3319,3325 ****
* simple Var. (This case won't be generated directly by the
* parser, because ParseComplexProjection short-circuits it.
* But it can arise while simplifying functions.) Also, we
! * can optimize field selection from a RowExpr construct.
*
* However, replacing a whole-row Var in this way has a
* pitfall: if we've already built the rel targetlist for the
--- 3333,3340 ----
* simple Var. (This case won't be generated directly by the
* parser, because ParseComplexProjection short-circuits it.
* But it can arise while simplifying functions.) Also, we
! * can optimize field selection from a RowExpr construct, or
! * of course from a constant.
*
* However, replacing a whole-row Var in this way has a
* pitfall: if we've already built the rel targetlist for the
*************** eval_const_expressions_mutator(Node *nod
*** 3334,3339 ****
--- 3349,3356 ----
* We must also check that the declared type of the field is
* still the same as when the FieldSelect was created --- this
* can change if someone did ALTER COLUMN TYPE on the rowtype.
+ * If it isn't, we skip the optimization; the case will
+ * probably fail at runtime, but that's not our problem here.
*/
FieldSelect *fselect = (FieldSelect *) node;
FieldSelect *newfselect;
*************** eval_const_expressions_mutator(Node *nod
*** 3384,3389 ****
--- 3401,3417 ----
newfselect->resulttype = fselect->resulttype;
newfselect->resulttypmod = fselect->resulttypmod;
newfselect->resultcollid = fselect->resultcollid;
+ if (arg && IsA(arg, Const))
+ {
+ Const *con = (Const *) arg;
+
+ if (rowtype_field_matches(con->consttype,
+ newfselect->fieldnum,
+ newfselect->resulttype,
+ newfselect->resulttypmod,
+ newfselect->resultcollid))
+ return ece_evaluate_expr(newfselect);
+ }
return (Node *) newfselect;
}
case T_NullTest:
*************** eval_const_expressions_mutator(Node *nod
*** 3477,3535 ****
newntest->location = ntest->location;
return (Node *) newntest;
}
- case T_BooleanTest:
- {
- BooleanTest *btest = (BooleanTest *) node;
- BooleanTest *newbtest;
- Node *arg;
-
- arg = eval_const_expressions_mutator((Node *) btest->arg,
- context);
- if (arg && IsA(arg, Const))
- {
- Const *carg = (Const *) arg;
- bool result;
-
- switch (btest->booltesttype)
- {
- case IS_TRUE:
- result = (!carg->constisnull &&
- DatumGetBool(carg->constvalue));
- break;
- case IS_NOT_TRUE:
- result = (carg->constisnull ||
- !DatumGetBool(carg->constvalue));
- break;
- case IS_FALSE:
- result = (!carg->constisnull &&
- !DatumGetBool(carg->constvalue));
- break;
- case IS_NOT_FALSE:
- result = (carg->constisnull ||
- DatumGetBool(carg->constvalue));
- break;
- case IS_UNKNOWN:
- result = carg->constisnull;
- break;
- case IS_NOT_UNKNOWN:
- result = !carg->constisnull;
- break;
- default:
- elog(ERROR, "unrecognized booltesttype: %d",
- (int) btest->booltesttype);
- result = false; /* keep compiler quiet */
- break;
- }
-
- return makeBoolConst(result, false);
- }
-
- newbtest = makeNode(BooleanTest);
- newbtest->arg = (Expr *) arg;
- newbtest->booltesttype = btest->booltesttype;
- newbtest->location = btest->location;
- return (Node *) newbtest;
- }
case T_PlaceHolderVar:
/*
--- 3505,3510 ----
*************** eval_const_expressions_mutator(Node *nod
*** 3552,3565 ****
}
/*
! * For any node type not handled above, we recurse using
! * expression_tree_mutator, which will copy the node unchanged but try to
! * simplify its arguments (if any) using this routine. For example: we
! * cannot eliminate an ArrayRef node, but we might be able to simplify
! * constant expressions in its subscripts.
*/
! return expression_tree_mutator(node, eval_const_expressions_mutator,
! (void *) context);
}
/*
--- 3527,3583 ----
}
/*
! * For any node type not handled above, copy the node unchanged but
! * const-simplify its subexpressions. This is the correct thing for node
! * types whose behavior might change between planning and execution, such
! * as CoerceToDomain. It's also a safe default for new node types not
! * known to this routine.
*/
! return ece_generic_processing(node);
! }
!
! /*
! * Subroutine for eval_const_expressions: check for non-Const nodes.
! *
! * We can abort recursion immediately on finding a non-Const node. This is
! * critical for performance, else eval_const_expressions_mutator would take
! * O(N^2) time on non-simplifiable trees. However, we do need to descend
! * into List nodes since expression_tree_walker sometimes invokes the walker
! * function directly on List subtrees.
! */
! static bool
! contain_non_const_walker(Node *node, void *context)
! {
! if (node == NULL)
! return false;
! if (IsA(node, Const))
! return false;
! if (IsA(node, List))
! return expression_tree_walker(node, contain_non_const_walker, context);
! /* Otherwise, abort the tree traversal and return true */
! return true;
! }
!
! /*
! * Subroutine for eval_const_expressions: check if a function is OK to evaluate
! */
! static bool
! ece_function_is_safe(Oid funcid, eval_const_expressions_context *context)
! {
! char provolatile = func_volatile(funcid);
!
! /*
! * 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;
}
/*
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index 43b36f6..a4bac8e 100644
*** a/src/test/regress/expected/rowtypes.out
--- b/src/test/regress/expected/rowtypes.out
*************** ERROR: cannot compare dissimilar column
*** 307,316 ****
explain (costs off)
select * from int8_tbl i8
where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
! QUERY PLAN
! -----------------------------------------------------------------------------------------------------------------
Seq Scan on int8_tbl i8
! Filter: (i8.* = ANY (ARRAY[ROW('123'::bigint, '456'::bigint)::int8_tbl, '(4567890123456789,123)'::int8_tbl]))
(2 rows)
select * from int8_tbl i8
--- 307,316 ----
explain (costs off)
select * from int8_tbl i8
where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
! QUERY PLAN
! -------------------------------------------------------------------------------
Seq Scan on int8_tbl i8
! Filter: (i8.* = ANY ('{"(123,456)","(4567890123456789,123)"}'::int8_tbl[]))
(2 rows)
select * from int8_tbl i8
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/11/2017 06:21 PM, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: >> Eval_const_expressions() doesn't know about ScalarArrayOpExpr. >> ... >> That seems like an oversight. I guess that scenario doesn't happen very >> often in practice, but there's no reason not to do it when it does. >> Patch attached. > > Yeah, I think it's a lack-of-round-tuits situation. Your patch reminds > me of a more ambitious attempt I made awhile back to reduce the amount > of duplicative boilerplate in eval_const_expressions. I think I wrote > it during last year's beta period, stashed it because I didn't feel like > arguing for applying it right then, and then forgot about it. Hmph, now we're almost in beta period again. :-(. > Blowing the dust off, it's attached. It fixes ArrayRef and RowExpr as > well as ScalarArrayOpExpr, with a net growth of only 20 lines > (largely comments). Nice! >> On a side-note, I find it a bit awkward that ScalarArrayOpExpr uses a >> 2-element List to hold the scalar and array arguments. Wouldn't it be >> much more straightforward to have explicit "Expr *scalararg" and "Expr >> *arrayarg" fields? > > I think it's modeled on OpExpr, which also uses a list though you could > argue for separate leftarg and rightarg fields instead. Yeah, I think that would be better for OpExpr, too. (For an unary operator, rightarg would be unused.) - Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 05/11/2017 06:21 PM, Tom Lane wrote:
>>> On a side-note, I find it a bit awkward that ScalarArrayOpExpr uses a
>>> 2-element List to hold the scalar and array arguments. Wouldn't it be
>>> much more straightforward to have explicit "Expr *scalararg" and "Expr
>>> *arrayarg" fields?
>> I think it's modeled on OpExpr, which also uses a list though you could
>> argue for separate leftarg and rightarg fields instead.
> Yeah, I think that would be better for OpExpr, too. (For an unary
> operator, rightarg would be unused.)
I should think leftarg is the one that would go unused, for a normal
prefix unary operator.
But it seems like changing this would be way more invasive than it's
really worth. We'd save a couple of List cells per operator, which
is certainly something, but I'm afraid you'd be touching a heck of
a lot of code. And there are a nontrivial number of places that
share argument-processing with FuncExprs, which such a change would
break.
regards, tom lane
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 05/11/2017 06:21 PM, Tom Lane wrote:
>> Yeah, I think it's a lack-of-round-tuits situation. Your patch reminds
>> me of a more ambitious attempt I made awhile back to reduce the amount
>> of duplicative boilerplate in eval_const_expressions. I think I wrote
>> it during last year's beta period, stashed it because I didn't feel like
>> arguing for applying it right then, and then forgot about it.
> Hmph, now we're almost in beta period again. :-(.
Actually, I now remember that I wrote that while at Salesforce (because
they'd run into the problem that constant ArrayRef expressions weren't
simplified). So that means it's been languishing in a git branch for
*two* years. I'm kind of inclined to push it before it gets forgotten
again ;-)
Looking at the patch, it still seems solid, but I remember that one
thing I was concerned about was whether the more generalized code
was any slower. Not sure about a good test case to measure that
though --- const-simplification isn't a large chunk of most workloads.
regards, tom lane
On Fri, May 12, 2017 at 12:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: >> On 05/11/2017 06:21 PM, Tom Lane wrote: >>> Yeah, I think it's a lack-of-round-tuits situation. Your patch reminds >>> me of a more ambitious attempt I made awhile back to reduce the amount >>> of duplicative boilerplate in eval_const_expressions. I think I wrote >>> it during last year's beta period, stashed it because I didn't feel like >>> arguing for applying it right then, and then forgot about it. > >> Hmph, now we're almost in beta period again. :-(. > > Actually, I now remember that I wrote that while at Salesforce (because > they'd run into the problem that constant ArrayRef expressions weren't > simplified). So that means it's been languishing in a git branch for > *two* years. I'm kind of inclined to push it before it gets forgotten > again ;-) You will probably not be surprised to hear that I think this is a feature and thus subject to the feature freeze currently in effect. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 12, 2017 at 12:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Actually, I now remember that I wrote that while at Salesforce (because
>> they'd run into the problem that constant ArrayRef expressions weren't
>> simplified). So that means it's been languishing in a git branch for
>> *two* years. I'm kind of inclined to push it before it gets forgotten
>> again ;-)
> You will probably not be surprised to hear that I think this is a
> feature and thus subject to the feature freeze currently in effect.
[ shrug ] Sure. I'll do what I should have done then, which is stick
it into the next CF list.
If you intend to also object to my proposed get_attstatsslot refactoring,
please do that promptly. That one I think is bug-proofing.
regards, tom lane
On Fri, May 12, 2017 at 12:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Fri, May 12, 2017 at 12:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Actually, I now remember that I wrote that while at Salesforce (because >>> they'd run into the problem that constant ArrayRef expressions weren't >>> simplified). So that means it's been languishing in a git branch for >>> *two* years. I'm kind of inclined to push it before it gets forgotten >>> again ;-) > >> You will probably not be surprised to hear that I think this is a >> feature and thus subject to the feature freeze currently in effect. > > [ shrug ] Sure. I'll do what I should have done then, which is stick > it into the next CF list. Thanks. > If you intend to also object to my proposed get_attstatsslot refactoring, > please do that promptly. That one I think is bug-proofing. I wasn't planning to express an opinion on that one. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
I wrote:
> Looking at the patch, it still seems solid, but I remember that one
> thing I was concerned about was whether the more generalized code
> was any slower. Not sure about a good test case to measure that
> though --- const-simplification isn't a large chunk of most workloads.
This patch no longer applies cleanly on HEAD, so here's a rebased version
(no substantive changes). As before, I think the most useful review task
would be to quantify whether it makes planning noticeably slower.
regards, tom lane
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 93add27..bc86cd7 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** static List *find_nonnullable_vars_walke
*** 115,120 ****
--- 115,123 ----
static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
static Node *eval_const_expressions_mutator(Node *node,
eval_const_expressions_context *context);
+ static bool contain_non_const_walker(Node *node, void *context);
+ static bool ece_function_is_safe(Oid funcid,
+ eval_const_expressions_context *context);
static List *simplify_or_arguments(List *args,
eval_const_expressions_context *context,
bool *haveNull, bool *forceTrue);
*************** estimate_expression_value(PlannerInfo *r
*** 2464,2469 ****
--- 2467,2503 ----
return eval_const_expressions_mutator(node, &context);
}
+ /*
+ * The generic case in eval_const_expressions_mutator is to recurse using
+ * expression_tree_mutator, which will copy the given node unchanged but
+ * const-simplify its arguments (if any) as far as possible. If the node
+ * itself does immutable processing, and each of its arguments were reduced
+ * to a Const, we can then reduce it to a Const using evaluate_expr. (Some
+ * node types need more complicated logic; for example, a CASE expression
+ * might be reducible to a constant even if not all its subtrees are.)
+ */
+ #define ece_generic_processing(node) \
+ expression_tree_mutator((Node *) (node), eval_const_expressions_mutator, \
+ (void *) context)
+
+ /*
+ * Check whether all arguments of the given node were reduced to Consts.
+ * By going directly to expression_tree_walker, contain_non_const_walker
+ * is not applied to the node itself, only to its children.
+ */
+ #define ece_all_arguments_const(node) \
+ (!expression_tree_walker((Node *) (node), contain_non_const_walker, NULL))
+
+ /* Generic macro for applying evaluate_expr */
+ #define ece_evaluate_expr(node) \
+ ((Node *) evaluate_expr((Expr *) (node), \
+ exprType((Node *) (node)), \
+ exprTypmod((Node *) (node)), \
+ exprCollation((Node *) (node))))
+
+ /*
+ * Recursive guts of eval_const_expressions/estimate_expression_value
+ */
static Node *
eval_const_expressions_mutator(Node *node,
eval_const_expressions_context *context)
*************** eval_const_expressions_mutator(Node *nod
*** 2779,2784 ****
--- 2813,2837 ----
newexpr->location = expr->location;
return (Node *) newexpr;
}
+ case T_ScalarArrayOpExpr:
+ {
+ ScalarArrayOpExpr *saop;
+
+ /* Copy the node and const-simplify its arguments */
+ saop = (ScalarArrayOpExpr *) ece_generic_processing(node);
+
+ /* Make sure we know underlying function */
+ set_sa_opfuncid(saop);
+
+ /*
+ * If all arguments are Consts, and it's a safe function, we
+ * can fold to a constant
+ */
+ if (ece_all_arguments_const(saop) &&
+ ece_function_is_safe(saop->opfuncid, context))
+ return ece_evaluate_expr(saop);
+ return (Node *) saop;
+ }
case T_BoolExpr:
{
BoolExpr *expr = (BoolExpr *) node;
*************** eval_const_expressions_mutator(Node *nod
*** 3003,3043 ****
}
case T_ArrayCoerceExpr:
{
! ArrayCoerceExpr *expr = (ArrayCoerceExpr *) node;
! Expr *arg;
! ArrayCoerceExpr *newexpr;
!
! /*
! * Reduce constants in the ArrayCoerceExpr's argument, then
! * build a new ArrayCoerceExpr.
! */
! arg = (Expr *) eval_const_expressions_mutator((Node *) expr->arg,
! context);
! newexpr = makeNode(ArrayCoerceExpr);
! newexpr->arg = arg;
! newexpr->elemfuncid = expr->elemfuncid;
! newexpr->resulttype = expr->resulttype;
! newexpr->resulttypmod = expr->resulttypmod;
! newexpr->resultcollid = expr->resultcollid;
! newexpr->isExplicit = expr->isExplicit;
! newexpr->coerceformat = expr->coerceformat;
! newexpr->location = expr->location;
/*
! * If constant argument and it's a binary-coercible or
! * immutable conversion, we can simplify it to a constant.
*/
! if (arg && IsA(arg, Const) &&
! (!OidIsValid(newexpr->elemfuncid) ||
! func_volatile(newexpr->elemfuncid) == PROVOLATILE_IMMUTABLE))
! return (Node *) evaluate_expr((Expr *) newexpr,
! newexpr->resulttype,
! newexpr->resulttypmod,
! newexpr->resultcollid);
!
! /* Else we must return the partially-simplified node */
! return (Node *) newexpr;
}
case T_CollateExpr:
{
--- 3056,3075 ----
}
case T_ArrayCoerceExpr:
{
! ArrayCoerceExpr *acexpr;
! /* Copy the node and const-simplify its arguments */
! acexpr = (ArrayCoerceExpr *) ece_generic_processing(node);
/*
! * If all arguments are Consts, and it's a binary-coercible or
! * safe conversion function, we can fold to a constant
*/
! if (ece_all_arguments_const(acexpr) &&
! (!OidIsValid(acexpr->elemfuncid) ||
! ece_function_is_safe(acexpr->elemfuncid, context)))
! return ece_evaluate_expr(acexpr);
! return (Node *) acexpr;
}
case T_CollateExpr:
{
*************** eval_const_expressions_mutator(Node *nod
*** 3229,3269 ****
else
return copyObject(node);
}
case T_ArrayExpr:
{
! ArrayExpr *arrayexpr = (ArrayExpr *) node;
! ArrayExpr *newarray;
! bool all_const = true;
! List *newelems;
! ListCell *element;
!
! newelems = NIL;
! foreach(element, arrayexpr->elements)
! {
! Node *e;
!
! e = eval_const_expressions_mutator((Node *) lfirst(element),
! context);
! if (!IsA(e, Const))
! all_const = false;
! newelems = lappend(newelems, e);
! }
!
! newarray = makeNode(ArrayExpr);
! newarray->array_typeid = arrayexpr->array_typeid;
! newarray->array_collid = arrayexpr->array_collid;
! newarray->element_typeid = arrayexpr->element_typeid;
! newarray->elements = newelems;
! newarray->multidims = arrayexpr->multidims;
! newarray->location = arrayexpr->location;
!
! if (all_const)
! return (Node *) evaluate_expr((Expr *) newarray,
! newarray->array_typeid,
! exprTypmod(node),
! newarray->array_collid);
! return (Node *) newarray;
}
case T_CoalesceExpr:
{
--- 3261,3283 ----
else
return copyObject(node);
}
+ case T_ArrayRef:
case T_ArrayExpr:
+ case T_RowExpr:
+ case T_BooleanTest:
{
! /*
! * Generic handling for node types whose own processing is
! * known to be immutable, and for which we need no smarts
! * beyond "simplify if all inputs are constants".
! */
! /* Copy the node and const-simplify its arguments */
! node = ece_generic_processing(node);
! /* If all arguments are Consts, we can fold to a constant */
! if (ece_all_arguments_const(node))
! return ece_evaluate_expr(node);
! return node;
}
case T_CoalesceExpr:
{
*************** eval_const_expressions_mutator(Node *nod
*** 3340,3346 ****
* simple Var. (This case won't be generated directly by the
* parser, because ParseComplexProjection short-circuits it.
* But it can arise while simplifying functions.) Also, we
! * can optimize field selection from a RowExpr construct.
*
* However, replacing a whole-row Var in this way has a
* pitfall: if we've already built the rel targetlist for the
--- 3354,3361 ----
* simple Var. (This case won't be generated directly by the
* parser, because ParseComplexProjection short-circuits it.
* But it can arise while simplifying functions.) Also, we
! * can optimize field selection from a RowExpr construct, or
! * of course from a constant.
*
* However, replacing a whole-row Var in this way has a
* pitfall: if we've already built the rel targetlist for the
*************** eval_const_expressions_mutator(Node *nod
*** 3355,3360 ****
--- 3370,3377 ----
* We must also check that the declared type of the field is
* still the same as when the FieldSelect was created --- this
* can change if someone did ALTER COLUMN TYPE on the rowtype.
+ * If it isn't, we skip the optimization; the case will
+ * probably fail at runtime, but that's not our problem here.
*/
FieldSelect *fselect = (FieldSelect *) node;
FieldSelect *newfselect;
*************** eval_const_expressions_mutator(Node *nod
*** 3405,3410 ****
--- 3422,3438 ----
newfselect->resulttype = fselect->resulttype;
newfselect->resulttypmod = fselect->resulttypmod;
newfselect->resultcollid = fselect->resultcollid;
+ if (arg && IsA(arg, Const))
+ {
+ Const *con = (Const *) arg;
+
+ if (rowtype_field_matches(con->consttype,
+ newfselect->fieldnum,
+ newfselect->resulttype,
+ newfselect->resulttypmod,
+ newfselect->resultcollid))
+ return ece_evaluate_expr(newfselect);
+ }
return (Node *) newfselect;
}
case T_NullTest:
*************** eval_const_expressions_mutator(Node *nod
*** 3498,3556 ****
newntest->location = ntest->location;
return (Node *) newntest;
}
- case T_BooleanTest:
- {
- BooleanTest *btest = (BooleanTest *) node;
- BooleanTest *newbtest;
- Node *arg;
-
- arg = eval_const_expressions_mutator((Node *) btest->arg,
- context);
- if (arg && IsA(arg, Const))
- {
- Const *carg = (Const *) arg;
- bool result;
-
- switch (btest->booltesttype)
- {
- case IS_TRUE:
- result = (!carg->constisnull &&
- DatumGetBool(carg->constvalue));
- break;
- case IS_NOT_TRUE:
- result = (carg->constisnull ||
- !DatumGetBool(carg->constvalue));
- break;
- case IS_FALSE:
- result = (!carg->constisnull &&
- !DatumGetBool(carg->constvalue));
- break;
- case IS_NOT_FALSE:
- result = (carg->constisnull ||
- DatumGetBool(carg->constvalue));
- break;
- case IS_UNKNOWN:
- result = carg->constisnull;
- break;
- case IS_NOT_UNKNOWN:
- result = !carg->constisnull;
- break;
- default:
- elog(ERROR, "unrecognized booltesttype: %d",
- (int) btest->booltesttype);
- result = false; /* keep compiler quiet */
- break;
- }
-
- return makeBoolConst(result, false);
- }
-
- newbtest = makeNode(BooleanTest);
- newbtest->arg = (Expr *) arg;
- newbtest->booltesttype = btest->booltesttype;
- newbtest->location = btest->location;
- return (Node *) newbtest;
- }
case T_PlaceHolderVar:
/*
--- 3526,3531 ----
*************** eval_const_expressions_mutator(Node *nod
*** 3573,3586 ****
}
/*
! * For any node type not handled above, we recurse using
! * expression_tree_mutator, which will copy the node unchanged but try to
! * simplify its arguments (if any) using this routine. For example: we
! * cannot eliminate an ArrayRef node, but we might be able to simplify
! * constant expressions in its subscripts.
*/
! return expression_tree_mutator(node, eval_const_expressions_mutator,
! (void *) context);
}
/*
--- 3548,3604 ----
}
/*
! * For any node type not handled above, copy the node unchanged but
! * const-simplify its subexpressions. This is the correct thing for node
! * types whose behavior might change between planning and execution, such
! * as CoerceToDomain. It's also a safe default for new node types not
! * known to this routine.
*/
! return ece_generic_processing(node);
! }
!
! /*
! * Subroutine for eval_const_expressions: check for non-Const nodes.
! *
! * We can abort recursion immediately on finding a non-Const node. This is
! * critical for performance, else eval_const_expressions_mutator would take
! * O(N^2) time on non-simplifiable trees. However, we do need to descend
! * into List nodes since expression_tree_walker sometimes invokes the walker
! * function directly on List subtrees.
! */
! static bool
! contain_non_const_walker(Node *node, void *context)
! {
! if (node == NULL)
! return false;
! if (IsA(node, Const))
! return false;
! if (IsA(node, List))
! return expression_tree_walker(node, contain_non_const_walker, context);
! /* Otherwise, abort the tree traversal and return true */
! return true;
! }
!
! /*
! * Subroutine for eval_const_expressions: check if a function is OK to evaluate
! */
! static bool
! ece_function_is_safe(Oid funcid, eval_const_expressions_context *context)
! {
! char provolatile = func_volatile(funcid);
!
! /*
! * 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;
}
/*
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index 43b36f6..a4bac8e 100644
*** a/src/test/regress/expected/rowtypes.out
--- b/src/test/regress/expected/rowtypes.out
*************** ERROR: cannot compare dissimilar column
*** 307,316 ****
explain (costs off)
select * from int8_tbl i8
where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
! QUERY PLAN
! -----------------------------------------------------------------------------------------------------------------
Seq Scan on int8_tbl i8
! Filter: (i8.* = ANY (ARRAY[ROW('123'::bigint, '456'::bigint)::int8_tbl, '(4567890123456789,123)'::int8_tbl]))
(2 rows)
select * from int8_tbl i8
--- 307,316 ----
explain (costs off)
select * from int8_tbl i8
where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
! QUERY PLAN
! -------------------------------------------------------------------------------
Seq Scan on int8_tbl i8
! Filter: (i8.* = ANY ('{"(123,456)","(4567890123456789,123)"}'::int8_tbl[]))
(2 rows)
select * from int8_tbl i8
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
> This patch no longer applies cleanly on HEAD, so here's a rebased version
> (no substantive changes). As before, I think the most useful review task
> would be to quantify whether it makes planning noticeably slower.
Rebased again (over the arrays-of-domains patch).
regards, tom lane
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 7961362..c3fab75 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** static List *find_nonnullable_vars_walke
*** 115,120 ****
--- 115,123 ----
static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
static Node *eval_const_expressions_mutator(Node *node,
eval_const_expressions_context *context);
+ static bool contain_non_const_walker(Node *node, void *context);
+ static bool ece_function_is_safe(Oid funcid,
+ eval_const_expressions_context *context);
static List *simplify_or_arguments(List *args,
eval_const_expressions_context *context,
bool *haveNull, bool *forceTrue);
*************** estimate_expression_value(PlannerInfo *r
*** 2472,2477 ****
--- 2475,2511 ----
return eval_const_expressions_mutator(node, &context);
}
+ /*
+ * The generic case in eval_const_expressions_mutator is to recurse using
+ * expression_tree_mutator, which will copy the given node unchanged but
+ * const-simplify its arguments (if any) as far as possible. If the node
+ * itself does immutable processing, and each of its arguments were reduced
+ * to a Const, we can then reduce it to a Const using evaluate_expr. (Some
+ * node types need more complicated logic; for example, a CASE expression
+ * might be reducible to a constant even if not all its subtrees are.)
+ */
+ #define ece_generic_processing(node) \
+ expression_tree_mutator((Node *) (node), eval_const_expressions_mutator, \
+ (void *) context)
+
+ /*
+ * Check whether all arguments of the given node were reduced to Consts.
+ * By going directly to expression_tree_walker, contain_non_const_walker
+ * is not applied to the node itself, only to its children.
+ */
+ #define ece_all_arguments_const(node) \
+ (!expression_tree_walker((Node *) (node), contain_non_const_walker, NULL))
+
+ /* Generic macro for applying evaluate_expr */
+ #define ece_evaluate_expr(node) \
+ ((Node *) evaluate_expr((Expr *) (node), \
+ exprType((Node *) (node)), \
+ exprTypmod((Node *) (node)), \
+ exprCollation((Node *) (node))))
+
+ /*
+ * Recursive guts of eval_const_expressions/estimate_expression_value
+ */
static Node *
eval_const_expressions_mutator(Node *node,
eval_const_expressions_context *context)
*************** eval_const_expressions_mutator(Node *nod
*** 2787,2792 ****
--- 2821,2845 ----
newexpr->location = expr->location;
return (Node *) newexpr;
}
+ case T_ScalarArrayOpExpr:
+ {
+ ScalarArrayOpExpr *saop;
+
+ /* Copy the node and const-simplify its arguments */
+ saop = (ScalarArrayOpExpr *) ece_generic_processing(node);
+
+ /* Make sure we know underlying function */
+ set_sa_opfuncid(saop);
+
+ /*
+ * If all arguments are Consts, and it's a safe function, we
+ * can fold to a constant
+ */
+ if (ece_all_arguments_const(saop) &&
+ ece_function_is_safe(saop->opfuncid, context))
+ return ece_evaluate_expr(saop);
+ return (Node *) saop;
+ }
case T_BoolExpr:
{
BoolExpr *expr = (BoolExpr *) node;
*************** eval_const_expressions_mutator(Node *nod
*** 3011,3057 ****
}
case T_ArrayCoerceExpr:
{
! ArrayCoerceExpr *expr = (ArrayCoerceExpr *) node;
! Expr *arg;
! Expr *elemexpr;
! ArrayCoerceExpr *newexpr;
!
! /*
! * Reduce constants in the ArrayCoerceExpr's argument and
! * per-element expressions, then build a new ArrayCoerceExpr.
! */
! arg = (Expr *) eval_const_expressions_mutator((Node *) expr->arg,
! context);
! elemexpr = (Expr *) eval_const_expressions_mutator((Node *) expr->elemexpr,
! context);
! newexpr = makeNode(ArrayCoerceExpr);
! newexpr->arg = arg;
! newexpr->elemexpr = elemexpr;
! newexpr->resulttype = expr->resulttype;
! newexpr->resulttypmod = expr->resulttypmod;
! newexpr->resultcollid = expr->resultcollid;
! newexpr->coerceformat = expr->coerceformat;
! newexpr->location = expr->location;
/*
! * If constant argument and per-element expression is
* immutable, we can simplify the whole thing to a constant.
* Exception: although contain_mutable_functions considers
* CoerceToDomain immutable for historical reasons, let's not
* do so here; this ensures coercion to an array-over-domain
* does not apply the domain's constraints until runtime.
*/
! if (arg && IsA(arg, Const) &&
! elemexpr && !IsA(elemexpr, CoerceToDomain) &&
! !contain_mutable_functions((Node *) elemexpr))
! return (Node *) evaluate_expr((Expr *) newexpr,
! newexpr->resulttype,
! newexpr->resulttypmod,
! newexpr->resultcollid);
!
! /* Else we must return the partially-simplified node */
! return (Node *) newexpr;
}
case T_CollateExpr:
{
--- 3064,3087 ----
}
case T_ArrayCoerceExpr:
{
! ArrayCoerceExpr *ac;
! /* Copy the node and const-simplify its arguments */
! ac = (ArrayCoerceExpr *) ece_generic_processing(node);
/*
! * If constant argument and the per-element expression is
* immutable, we can simplify the whole thing to a constant.
* Exception: although contain_mutable_functions considers
* CoerceToDomain immutable for historical reasons, let's not
* do so here; this ensures coercion to an array-over-domain
* does not apply the domain's constraints until runtime.
*/
! if (ac->arg && IsA(ac->arg, Const) &&
! ac->elemexpr && !IsA(ac->elemexpr, CoerceToDomain) &&
! !contain_mutable_functions((Node *) ac->elemexpr))
! return ece_evaluate_expr(ac);
! return (Node *) ac;
}
case T_CollateExpr:
{
*************** eval_const_expressions_mutator(Node *nod
*** 3243,3283 ****
else
return copyObject(node);
}
case T_ArrayExpr:
{
! ArrayExpr *arrayexpr = (ArrayExpr *) node;
! ArrayExpr *newarray;
! bool all_const = true;
! List *newelems;
! ListCell *element;
!
! newelems = NIL;
! foreach(element, arrayexpr->elements)
! {
! Node *e;
!
! e = eval_const_expressions_mutator((Node *) lfirst(element),
! context);
! if (!IsA(e, Const))
! all_const = false;
! newelems = lappend(newelems, e);
! }
!
! newarray = makeNode(ArrayExpr);
! newarray->array_typeid = arrayexpr->array_typeid;
! newarray->array_collid = arrayexpr->array_collid;
! newarray->element_typeid = arrayexpr->element_typeid;
! newarray->elements = newelems;
! newarray->multidims = arrayexpr->multidims;
! newarray->location = arrayexpr->location;
!
! if (all_const)
! return (Node *) evaluate_expr((Expr *) newarray,
! newarray->array_typeid,
! exprTypmod(node),
! newarray->array_collid);
! return (Node *) newarray;
}
case T_CoalesceExpr:
{
--- 3273,3295 ----
else
return copyObject(node);
}
+ case T_ArrayRef:
case T_ArrayExpr:
+ case T_RowExpr:
+ case T_BooleanTest:
{
! /*
! * Generic handling for node types whose own processing is
! * known to be immutable, and for which we need no smarts
! * beyond "simplify if all inputs are constants".
! */
! /* Copy the node and const-simplify its arguments */
! node = ece_generic_processing(node);
! /* If all arguments are Consts, we can fold to a constant */
! if (ece_all_arguments_const(node))
! return ece_evaluate_expr(node);
! return node;
}
case T_CoalesceExpr:
{
*************** eval_const_expressions_mutator(Node *nod
*** 3354,3360 ****
* simple Var. (This case won't be generated directly by the
* parser, because ParseComplexProjection short-circuits it.
* But it can arise while simplifying functions.) Also, we
! * can optimize field selection from a RowExpr construct.
*
* However, replacing a whole-row Var in this way has a
* pitfall: if we've already built the rel targetlist for the
--- 3366,3373 ----
* simple Var. (This case won't be generated directly by the
* parser, because ParseComplexProjection short-circuits it.
* But it can arise while simplifying functions.) Also, we
! * can optimize field selection from a RowExpr construct, or
! * of course from a constant.
*
* However, replacing a whole-row Var in this way has a
* pitfall: if we've already built the rel targetlist for the
*************** eval_const_expressions_mutator(Node *nod
*** 3369,3374 ****
--- 3382,3389 ----
* We must also check that the declared type of the field is
* still the same as when the FieldSelect was created --- this
* can change if someone did ALTER COLUMN TYPE on the rowtype.
+ * If it isn't, we skip the optimization; the case will
+ * probably fail at runtime, but that's not our problem here.
*/
FieldSelect *fselect = (FieldSelect *) node;
FieldSelect *newfselect;
*************** eval_const_expressions_mutator(Node *nod
*** 3419,3424 ****
--- 3434,3450 ----
newfselect->resulttype = fselect->resulttype;
newfselect->resulttypmod = fselect->resulttypmod;
newfselect->resultcollid = fselect->resultcollid;
+ if (arg && IsA(arg, Const))
+ {
+ Const *con = (Const *) arg;
+
+ if (rowtype_field_matches(con->consttype,
+ newfselect->fieldnum,
+ newfselect->resulttype,
+ newfselect->resulttypmod,
+ newfselect->resultcollid))
+ return ece_evaluate_expr(newfselect);
+ }
return (Node *) newfselect;
}
case T_NullTest:
*************** eval_const_expressions_mutator(Node *nod
*** 3512,3570 ****
newntest->location = ntest->location;
return (Node *) newntest;
}
- case T_BooleanTest:
- {
- BooleanTest *btest = (BooleanTest *) node;
- BooleanTest *newbtest;
- Node *arg;
-
- arg = eval_const_expressions_mutator((Node *) btest->arg,
- context);
- if (arg && IsA(arg, Const))
- {
- Const *carg = (Const *) arg;
- bool result;
-
- switch (btest->booltesttype)
- {
- case IS_TRUE:
- result = (!carg->constisnull &&
- DatumGetBool(carg->constvalue));
- break;
- case IS_NOT_TRUE:
- result = (carg->constisnull ||
- !DatumGetBool(carg->constvalue));
- break;
- case IS_FALSE:
- result = (!carg->constisnull &&
- !DatumGetBool(carg->constvalue));
- break;
- case IS_NOT_FALSE:
- result = (carg->constisnull ||
- DatumGetBool(carg->constvalue));
- break;
- case IS_UNKNOWN:
- result = carg->constisnull;
- break;
- case IS_NOT_UNKNOWN:
- result = !carg->constisnull;
- break;
- default:
- elog(ERROR, "unrecognized booltesttype: %d",
- (int) btest->booltesttype);
- result = false; /* keep compiler quiet */
- break;
- }
-
- return makeBoolConst(result, false);
- }
-
- newbtest = makeNode(BooleanTest);
- newbtest->arg = (Expr *) arg;
- newbtest->booltesttype = btest->booltesttype;
- newbtest->location = btest->location;
- return (Node *) newbtest;
- }
case T_PlaceHolderVar:
/*
--- 3538,3543 ----
*************** eval_const_expressions_mutator(Node *nod
*** 3587,3600 ****
}
/*
! * For any node type not handled above, we recurse using
! * expression_tree_mutator, which will copy the node unchanged but try to
! * simplify its arguments (if any) using this routine. For example: we
! * cannot eliminate an ArrayRef node, but we might be able to simplify
! * constant expressions in its subscripts.
*/
! return expression_tree_mutator(node, eval_const_expressions_mutator,
! (void *) context);
}
/*
--- 3560,3616 ----
}
/*
! * For any node type not handled above, copy the node unchanged but
! * const-simplify its subexpressions. This is the correct thing for node
! * types whose behavior might change between planning and execution, such
! * as CoerceToDomain. It's also a safe default for new node types not
! * known to this routine.
*/
! return ece_generic_processing(node);
! }
!
! /*
! * Subroutine for eval_const_expressions: check for non-Const nodes.
! *
! * We can abort recursion immediately on finding a non-Const node. This is
! * critical for performance, else eval_const_expressions_mutator would take
! * O(N^2) time on non-simplifiable trees. However, we do need to descend
! * into List nodes since expression_tree_walker sometimes invokes the walker
! * function directly on List subtrees.
! */
! static bool
! contain_non_const_walker(Node *node, void *context)
! {
! if (node == NULL)
! return false;
! if (IsA(node, Const))
! return false;
! if (IsA(node, List))
! return expression_tree_walker(node, contain_non_const_walker, context);
! /* Otherwise, abort the tree traversal and return true */
! return true;
! }
!
! /*
! * Subroutine for eval_const_expressions: check if a function is OK to evaluate
! */
! static bool
! ece_function_is_safe(Oid funcid, eval_const_expressions_context *context)
! {
! char provolatile = func_volatile(funcid);
!
! /*
! * 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;
}
/*
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index 43b36f6..a4bac8e 100644
*** a/src/test/regress/expected/rowtypes.out
--- b/src/test/regress/expected/rowtypes.out
*************** ERROR: cannot compare dissimilar column
*** 307,316 ****
explain (costs off)
select * from int8_tbl i8
where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
! QUERY PLAN
! -----------------------------------------------------------------------------------------------------------------
Seq Scan on int8_tbl i8
! Filter: (i8.* = ANY (ARRAY[ROW('123'::bigint, '456'::bigint)::int8_tbl, '(4567890123456789,123)'::int8_tbl]))
(2 rows)
select * from int8_tbl i8
--- 307,316 ----
explain (costs off)
select * from int8_tbl i8
where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
! QUERY PLAN
! -------------------------------------------------------------------------------
Seq Scan on int8_tbl i8
! Filter: (i8.* = ANY ('{"(123,456)","(4567890123456789,123)"}'::int8_tbl[]))
(2 rows)
select * from int8_tbl i8
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Oct 2, 2017 at 7:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> This patch no longer applies cleanly on HEAD, so here's a rebased version >> (no substantive changes). As before, I think the most useful review task >> would be to quantify whether it makes planning noticeably slower. > > Rebased again (over the arrays-of-domains patch). Nobody has showed up with the courage to review this patch. So moved to next CF. The patch still applies cleanly. -- Michael
> On 12 September 2017 at 15:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> This patch no longer applies cleanly on HEAD, so here's a rebased version
> (no substantive changes). As before, I think the most useful review task
> would be to quantify whether it makes planning noticeably slower.
I tried to experiment a bit with this patch, hope it may be helpful. Basically
I've made some number of pgbench runs over an instance with/without the patch,
assuming, that since the only difference would be in the planner, this will
show performance impact from the patch. Also, I've manually collected some
statistics for "Planning time" from explain analyze (I suppose it's also a valid
test). As a test target I've used queries like `select * from test where 1 in
(1, 2)` (as a side note - an involved table was empty) for different number of
elements in an array, and for situations when this condition is true/false.
So, for:
pgbench -c 50 -s 100 -j 50 -n -t 1000 -f const_eval.sql
I've got this data (avg latency):
type w/ patch w/o patch
short array 5.203 5.564
long array 9.293 9.667
Just out of curiosity I've also made the same test for constant arrays instead
of integers, but in this case numbers for with and without the patch were
almost identical.
For explain analyze (avg planning time):
type w/ patch w/o patch
short array 0.214 0.226
long array 0.374 0.320
I'm not sure why for the long array case with the patch I have smaller latency
(I thought, that more complexity should have negative impact on the
performance), but a bit longer planning time. But at the end it looks that
the difference is not that big.
Speaking about the code, everything looks fine. As for me, some variables could
be named in a more self-explanatory way (e.g `ece_evaluate_expr`, where `ece`
is `eval_const_expresisions`, or `saop`, which is `ScalarArrayOp`), but it's
minor.
I wonder if it makes sense in this patch also deal with the following
situation?
explain analyze select * from test where 1 in (select 1);
QUERY PLAN
-----------------------------------------------------------------------------------------------
Result (cost=0.02..35.52 rows=2550 width=4) (actual time=0.036..0.036 rows=0 loops=1)
One-Time Filter: (hashed SubPlan 1)
-> Seq Scan on test (cost=0.02..35.52 rows=2550 width=4) (actual time=0.009..0.009 rows=0 loops=1)
SubPlan 1
-> Result (cost=0.00..0.01 rows=1 width=4) (actual time=0.002..0.002 rows=1 loops=1)
Planning time: 0.381 ms
Execution time: 0.360 ms
It looks quite similar from a user perspective (although since a subplan is
involved, I assume it may be quite different in terms of implementation).
Dmitry Dolgov <9erthalion6@gmail.com> writes:
> I tried to experiment a bit with this patch, hope it may be helpful.
Thanks for reviewing!
I took your idea of just running pgbench results, and adapted it to these
test cases:
select * from test where 1 = 1 or 1 = 2;
select * from test where 1 = 1 and 1 = 2;
select * from test where 1 in (1, 2);
select * from test where id in (1,2);
where the test table is just created with
create table test (id int);
and contains no data.
For me, the first, second, and fourth cases are within 1% of the
same speed with or without the patch; some a bit slower, some a
bit faster, but really all of those results are barely above the
noise floor. The third case is distinctly faster (~8%) with patch;
but since we're emitting a better plan, with no runtime WHERE test,
that's probably more about reduced executor overhead than anything else.
I did find one case where the patch makes things significantly slower:
select * from test where true is true
and true is true
and true is true
and true is true
... (100 times altogether)
That's a full 25% slower with the patch. Investigation confirms
that the extra cost can be blamed on using evaluate_expr() to
simplify BooleanTest nodes, in place of the specialized logic
that's there now. I don't feel bad about the other places where
evaluate_expr() is used: either they were like that before, or
the case wasn't const-simplified at all, so that even with some
overhead we can be pretty sure life is better with the patch.
But there's room to argue that BooleanTest occurs often enough that
it's worth having bespoke logic to simplify it, if that logic isn't too
complicated which it really isn't. So I'm inclined to undo the part
of the patch that turns BooleanTest processing into a generic case,
as per attached updated patch. Otherwise I think we can figure that
performance isn't a problem.
> Speaking about the code, everything looks fine. As for me, some
> variables could be named in a more self-explanatory way (e.g
> `ece_evaluate_expr`, where `ece` is `eval_const_expresisions`, or
> `saop`, which is `ScalarArrayOp`), but it's minor.
I'm disinclined to change the usage of "saop" here --- that's already
in use in lots of code touching ScalarArrayOp, so even if we had a better
idea, this patch isn't the place to change it. I'd be fine with changing
the "ece_" prefix, but I don't want to spell out eval_const_expressions
altogether; do you have a different abbreviation suggestion?
> I wonder if it makes sense in this patch also deal with the following
> situation?
> explain analyze select * from test where 1 in (select 1);
No; lots of people depend on the fact that a sub-select is an optimization
fence in that way. If we were going to change that it'd need to be
carefully thought through, and I don't think it should happen as a side
effect of what's mostly a refactoring patch. The code changes wouldn't
be anywhere near this patch, either ...
regards, tom lane
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 9ca384d..cba40b2 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** static List *find_nonnullable_vars_walke
*** 115,120 ****
--- 115,123 ----
static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
static Node *eval_const_expressions_mutator(Node *node,
eval_const_expressions_context *context);
+ static bool contain_non_const_walker(Node *node, void *context);
+ static bool ece_function_is_safe(Oid funcid,
+ eval_const_expressions_context *context);
static List *simplify_or_arguments(List *args,
eval_const_expressions_context *context,
bool *haveNull, bool *forceTrue);
*************** estimate_expression_value(PlannerInfo *r
*** 2502,2507 ****
--- 2505,2541 ----
return eval_const_expressions_mutator(node, &context);
}
+ /*
+ * The generic case in eval_const_expressions_mutator is to recurse using
+ * expression_tree_mutator, which will copy the given node unchanged but
+ * const-simplify its arguments (if any) as far as possible. If the node
+ * itself does immutable processing, and each of its arguments were reduced
+ * to a Const, we can then reduce it to a Const using evaluate_expr. (Some
+ * node types need more complicated logic; for example, a CASE expression
+ * might be reducible to a constant even if not all its subtrees are.)
+ */
+ #define ece_generic_processing(node) \
+ expression_tree_mutator((Node *) (node), eval_const_expressions_mutator, \
+ (void *) context)
+
+ /*
+ * Check whether all arguments of the given node were reduced to Consts.
+ * By going directly to expression_tree_walker, contain_non_const_walker
+ * is not applied to the node itself, only to its children.
+ */
+ #define ece_all_arguments_const(node) \
+ (!expression_tree_walker((Node *) (node), contain_non_const_walker, NULL))
+
+ /* Generic macro for applying evaluate_expr */
+ #define ece_evaluate_expr(node) \
+ ((Node *) evaluate_expr((Expr *) (node), \
+ exprType((Node *) (node)), \
+ exprTypmod((Node *) (node)), \
+ exprCollation((Node *) (node))))
+
+ /*
+ * Recursive guts of eval_const_expressions/estimate_expression_value
+ */
static Node *
eval_const_expressions_mutator(Node *node,
eval_const_expressions_context *context)
*************** eval_const_expressions_mutator(Node *nod
*** 2830,2835 ****
--- 2864,2888 ----
newexpr->location = expr->location;
return (Node *) newexpr;
}
+ case T_ScalarArrayOpExpr:
+ {
+ ScalarArrayOpExpr *saop;
+
+ /* Copy the node and const-simplify its arguments */
+ saop = (ScalarArrayOpExpr *) ece_generic_processing(node);
+
+ /* Make sure we know underlying function */
+ set_sa_opfuncid(saop);
+
+ /*
+ * If all arguments are Consts, and it's a safe function, we
+ * can fold to a constant
+ */
+ if (ece_all_arguments_const(saop) &&
+ ece_function_is_safe(saop->opfuncid, context))
+ return ece_evaluate_expr(saop);
+ return (Node *) saop;
+ }
case T_BoolExpr:
{
BoolExpr *expr = (BoolExpr *) node;
*************** eval_const_expressions_mutator(Node *nod
*** 3054,3100 ****
}
case T_ArrayCoerceExpr:
{
! ArrayCoerceExpr *expr = (ArrayCoerceExpr *) node;
! Expr *arg;
! Expr *elemexpr;
! ArrayCoerceExpr *newexpr;
!
! /*
! * Reduce constants in the ArrayCoerceExpr's argument and
! * per-element expressions, then build a new ArrayCoerceExpr.
! */
! arg = (Expr *) eval_const_expressions_mutator((Node *) expr->arg,
! context);
! elemexpr = (Expr *) eval_const_expressions_mutator((Node *) expr->elemexpr,
! context);
! newexpr = makeNode(ArrayCoerceExpr);
! newexpr->arg = arg;
! newexpr->elemexpr = elemexpr;
! newexpr->resulttype = expr->resulttype;
! newexpr->resulttypmod = expr->resulttypmod;
! newexpr->resultcollid = expr->resultcollid;
! newexpr->coerceformat = expr->coerceformat;
! newexpr->location = expr->location;
/*
! * If constant argument and per-element expression is
* immutable, we can simplify the whole thing to a constant.
* Exception: although contain_mutable_functions considers
* CoerceToDomain immutable for historical reasons, let's not
* do so here; this ensures coercion to an array-over-domain
* does not apply the domain's constraints until runtime.
*/
! if (arg && IsA(arg, Const) &&
! elemexpr && !IsA(elemexpr, CoerceToDomain) &&
! !contain_mutable_functions((Node *) elemexpr))
! return (Node *) evaluate_expr((Expr *) newexpr,
! newexpr->resulttype,
! newexpr->resulttypmod,
! newexpr->resultcollid);
!
! /* Else we must return the partially-simplified node */
! return (Node *) newexpr;
}
case T_CollateExpr:
{
--- 3107,3130 ----
}
case T_ArrayCoerceExpr:
{
! ArrayCoerceExpr *ac;
! /* Copy the node and const-simplify its arguments */
! ac = (ArrayCoerceExpr *) ece_generic_processing(node);
/*
! * If constant argument and the per-element expression is
* immutable, we can simplify the whole thing to a constant.
* Exception: although contain_mutable_functions considers
* CoerceToDomain immutable for historical reasons, let's not
* do so here; this ensures coercion to an array-over-domain
* does not apply the domain's constraints until runtime.
*/
! if (ac->arg && IsA(ac->arg, Const) &&
! ac->elemexpr && !IsA(ac->elemexpr, CoerceToDomain) &&
! !contain_mutable_functions((Node *) ac->elemexpr))
! return ece_evaluate_expr(ac);
! return (Node *) ac;
}
case T_CollateExpr:
{
*************** eval_const_expressions_mutator(Node *nod
*** 3286,3326 ****
else
return copyObject(node);
}
case T_ArrayExpr:
{
! ArrayExpr *arrayexpr = (ArrayExpr *) node;
! ArrayExpr *newarray;
! bool all_const = true;
! List *newelems;
! ListCell *element;
!
! newelems = NIL;
! foreach(element, arrayexpr->elements)
! {
! Node *e;
!
! e = eval_const_expressions_mutator((Node *) lfirst(element),
! context);
! if (!IsA(e, Const))
! all_const = false;
! newelems = lappend(newelems, e);
! }
!
! newarray = makeNode(ArrayExpr);
! newarray->array_typeid = arrayexpr->array_typeid;
! newarray->array_collid = arrayexpr->array_collid;
! newarray->element_typeid = arrayexpr->element_typeid;
! newarray->elements = newelems;
! newarray->multidims = arrayexpr->multidims;
! newarray->location = arrayexpr->location;
!
! if (all_const)
! return (Node *) evaluate_expr((Expr *) newarray,
! newarray->array_typeid,
! exprTypmod(node),
! newarray->array_collid);
! return (Node *) newarray;
}
case T_CoalesceExpr:
{
--- 3316,3337 ----
else
return copyObject(node);
}
+ case T_ArrayRef:
case T_ArrayExpr:
+ case T_RowExpr:
{
! /*
! * Generic handling for node types whose own processing is
! * known to be immutable, and for which we need no smarts
! * beyond "simplify if all inputs are constants".
! */
! /* Copy the node and const-simplify its arguments */
! node = ece_generic_processing(node);
! /* If all arguments are Consts, we can fold to a constant */
! if (ece_all_arguments_const(node))
! return ece_evaluate_expr(node);
! return node;
}
case T_CoalesceExpr:
{
*************** eval_const_expressions_mutator(Node *nod
*** 3397,3403 ****
* simple Var. (This case won't be generated directly by the
* parser, because ParseComplexProjection short-circuits it.
* But it can arise while simplifying functions.) Also, we
! * can optimize field selection from a RowExpr construct.
*
* However, replacing a whole-row Var in this way has a
* pitfall: if we've already built the rel targetlist for the
--- 3408,3415 ----
* simple Var. (This case won't be generated directly by the
* parser, because ParseComplexProjection short-circuits it.
* But it can arise while simplifying functions.) Also, we
! * can optimize field selection from a RowExpr construct, or
! * of course from a constant.
*
* However, replacing a whole-row Var in this way has a
* pitfall: if we've already built the rel targetlist for the
*************** eval_const_expressions_mutator(Node *nod
*** 3412,3417 ****
--- 3424,3431 ----
* We must also check that the declared type of the field is
* still the same as when the FieldSelect was created --- this
* can change if someone did ALTER COLUMN TYPE on the rowtype.
+ * If it isn't, we skip the optimization; the case will
+ * probably fail at runtime, but that's not our problem here.
*/
FieldSelect *fselect = (FieldSelect *) node;
FieldSelect *newfselect;
*************** eval_const_expressions_mutator(Node *nod
*** 3462,3467 ****
--- 3476,3492 ----
newfselect->resulttype = fselect->resulttype;
newfselect->resulttypmod = fselect->resulttypmod;
newfselect->resultcollid = fselect->resultcollid;
+ if (arg && IsA(arg, Const))
+ {
+ Const *con = (Const *) arg;
+
+ if (rowtype_field_matches(con->consttype,
+ newfselect->fieldnum,
+ newfselect->resulttype,
+ newfselect->resulttypmod,
+ newfselect->resultcollid))
+ return ece_evaluate_expr(newfselect);
+ }
return (Node *) newfselect;
}
case T_NullTest:
*************** eval_const_expressions_mutator(Node *nod
*** 3557,3562 ****
--- 3582,3594 ----
}
case T_BooleanTest:
{
+ /*
+ * This case could be folded into the generic handling used
+ * for ArrayRef etc. But because the simplification logic is
+ * so trivial, applying evaluate_expr() to perform it would be
+ * a heavy overhead. BooleanTest is probably common enough to
+ * justify keeping this bespoke implementation.
+ */
BooleanTest *btest = (BooleanTest *) node;
BooleanTest *newbtest;
Node *arg;
*************** eval_const_expressions_mutator(Node *nod
*** 3630,3643 ****
}
/*
! * For any node type not handled above, we recurse using
! * expression_tree_mutator, which will copy the node unchanged but try to
! * simplify its arguments (if any) using this routine. For example: we
! * cannot eliminate an ArrayRef node, but we might be able to simplify
! * constant expressions in its subscripts.
*/
! return expression_tree_mutator(node, eval_const_expressions_mutator,
! (void *) context);
}
/*
--- 3662,3718 ----
}
/*
! * For any node type not handled above, copy the node unchanged but
! * const-simplify its subexpressions. This is the correct thing for node
! * types whose behavior might change between planning and execution, such
! * as CoerceToDomain. It's also a safe default for new node types not
! * known to this routine.
*/
! return ece_generic_processing(node);
! }
!
! /*
! * Subroutine for eval_const_expressions: check for non-Const nodes.
! *
! * We can abort recursion immediately on finding a non-Const node. This is
! * critical for performance, else eval_const_expressions_mutator would take
! * O(N^2) time on non-simplifiable trees. However, we do need to descend
! * into List nodes since expression_tree_walker sometimes invokes the walker
! * function directly on List subtrees.
! */
! static bool
! contain_non_const_walker(Node *node, void *context)
! {
! if (node == NULL)
! return false;
! if (IsA(node, Const))
! return false;
! if (IsA(node, List))
! return expression_tree_walker(node, contain_non_const_walker, context);
! /* Otherwise, abort the tree traversal and return true */
! return true;
! }
!
! /*
! * Subroutine for eval_const_expressions: check if a function is OK to evaluate
! */
! static bool
! ece_function_is_safe(Oid funcid, eval_const_expressions_context *context)
! {
! char provolatile = func_volatile(funcid);
!
! /*
! * 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;
}
/*
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index 43b36f6..a4bac8e 100644
*** a/src/test/regress/expected/rowtypes.out
--- b/src/test/regress/expected/rowtypes.out
*************** ERROR: cannot compare dissimilar column
*** 307,316 ****
explain (costs off)
select * from int8_tbl i8
where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
! QUERY PLAN
! -----------------------------------------------------------------------------------------------------------------
Seq Scan on int8_tbl i8
! Filter: (i8.* = ANY (ARRAY[ROW('123'::bigint, '456'::bigint)::int8_tbl, '(4567890123456789,123)'::int8_tbl]))
(2 rows)
select * from int8_tbl i8
--- 307,316 ----
explain (costs off)
select * from int8_tbl i8
where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
! QUERY PLAN
! -------------------------------------------------------------------------------
Seq Scan on int8_tbl i8
! Filter: (i8.* = ANY ('{"(123,456)","(4567890123456789,123)"}'::int8_tbl[]))
(2 rows)
select * from int8_tbl i8
> On 2 January 2018 at 20:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I did find one case where the patch makes things significantly slower:
>
> select * from test where true is true
> and true is true
> and true is true
> and true is true
> ... (100 times altogether)
Yes, indeed, I never tested this kind of conditions. I can confirm, this query
is significantly slower with the previous version of the patch, but the current
version solves this.
> I'm disinclined to change the usage of "saop" here --- that's already
> in use in lots of code touching ScalarArrayOp
Oh, ok - I see now, thank you. So, I think no one would object if I'll mark
this patch as ready for committer.
Dmitry Dolgov <9erthalion6@gmail.com> writes:
> Oh, ok - I see now, thank you. So, I think no one would object if I'll mark
> this patch as ready for committer.
Pushed, thanks for reviewing!
regards, tom lane