Hello Andrew,
Thank you for the review of the patch.
On Fri, 04 May 2018 08:37:31 +0100
Andrew Gierth <andrew@tao11.riddles.org.uk> wrote:
> From an implementation point of view your patch is obviously broken in
> many ways (starting with not checking varattno anywhere, and not
> actually checking anywhere if the expression is volatile).
The actual checking if the expression volatile or not is done inside
evaluate_function(). This is called through few more function in
eval_const_experssion(). If it's volatile, the eval_const_expression()
will return FuncExpr node, Const otherwise. It also checks are arguments
immutable or not.
I agree about varattno, it should be checked. Even in case of SRF not
replaced, it is better to be sure that Var points to first (and the
only) attribute.
> But more importantly the plans that you showed seem _worse_ in that
> you've created extra calls to to_tsquery even though the query has
> been written to try and avoid that.
>
> I think you need to take a step back and explain more precisely what
> you think you're trying to do and what the benefit (and cost) is.
Sure, the first version is some kind of prototype and attempt to
improve execution of the certain type of queries. The best solution I
see is to use the result of the function where it is required and remove
the nested loop in case of immutable functions. In this case, the
planner can choose a better plan if function result is used for tuples
selecting or as a join filter. But it will increase planning time due to
the execution of immutable functions.
It looks like I did something wrong with plans in the example, because
attached patch replaces function-relation usage with result value, not
function call. But nested loop is still in the plan.
I'm open to any suggestions/notices/critics about the idea and approach.
--
Aleksandr Parfenov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company