Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

Dmitry Dolgov <9erthali...@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

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

Dmitry Dolgov <9erthali...@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, );
  }
  
+ /*
+  * 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))

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Dmitry Dolgov]

> On 12 September 2017 at 15:29, Tom Lane  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):

typew/ patchw/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):

typew/ patchw/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).

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Michael Paquier]

On Mon, Oct 2, 2017 at 7:52 AM, Tom Lane  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