Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

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, );
  }
  
+ /*
+  * 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 && 

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

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, );
  }
  
+ /*
+  * 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,
! 			

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Robert Haas]

On Fri, May 12, 2017 at 12:55 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, May 12, 2017 at 12:04 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

Robert Haas  writes:
> On Fri, May 12, 2017 at 12:04 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Robert Haas]

On Fri, May 12, 2017 at 12:04 PM, Tom Lane  wrote:
> Heikki Linnakangas  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

Heikki Linnakangas  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

Heikki Linnakangas  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Heikki Linnakangas]

On 05/11/2017 06:21 PM, Tom Lane wrote:

Heikki Linnakangas  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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

Heikki Linnakangas  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, );
  }
  
+ /*
+  * 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 = 

[HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Heikki Linnakangas]

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



0001-Const-eval-ScalarArrayOpExprs.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers