On Tue, Feb 18, 2020 at 2:56 PM Pavel Stehule <pavel.steh...@gmail.com> wrote:
> út 18. 2. 2020 v 6:03 odesílatel Amit Langote <amitlangot...@gmail.com>
> napsal:
>> I didn't send the patch, because it didn't handle the cases where a
>> simple expression consists of an inline-able function(s) in it, which
>> are better handled by a full-fledged planner call backed up by the
>> plan cache. If we don't do that then every evaluation of such
>> "simple" expression needs to invoke the planner. For example:
>>
>> Consider this inline-able SQL function:
>>
>> create or replace function sql_incr(a bigint)
>> returns int
>> immutable language sql as $$
>> select a+1;
>> $$;
>>
>> Then this revised body of your function foo():
>>
>> CREATE OR REPLACE FUNCTION public.foo()
>> RETURNS int
>> LANGUAGE plpgsql
>> IMMUTABLE
>> AS $function$
>> declare i bigint = 0;
>> begin
>> while i < 1000000
>> loop
>> i := sql_incr(i);
>> end loop; return i;
>> end;
>> $function$
>> ;
>>
>> With HEAD `select foo()` finishes in 786 ms, whereas with the patch,
>> it takes 5102 ms.
>>
>> I think the patch might be good idea to reduce the time to compute
>> simple expressions in plpgsql, if we can address the above issue.
>
>
> Your patch is very interesting - minimally it returns performance before 8.2.
> The mentioned issue can be fixed if we disallow SQL functions in this fast
> execution.
I updated the patch to do that.
With the new patch, `select foo()`, with inline-able sql_incr() in it,
runs in 679 ms.
Without any inline-able function, it runs in 330 ms, whereas with
HEAD, it takes 590 ms.
Thanks,
Amit
diff --git a/src/backend/optimizer/util/clauses.c
b/src/backend/optimizer/util/clauses.c
index 2d3ec22407..5ce0079a12 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -108,6 +108,7 @@ static bool
contain_volatile_functions_not_nextval_walker(Node *node, void *cont
static bool max_parallel_hazard_walker(Node *node,
max_parallel_hazard_context *context);
static bool contain_nonstrict_functions_walker(Node *node, void *context);
+static bool contain_inlinable_functions_walker(Node *node, void *context);
static bool contain_context_dependent_node(Node *clause);
static bool contain_context_dependent_node_walker(Node *node, int *flags);
static bool contain_leaked_vars_walker(Node *node, void *context);
@@ -1218,6 +1219,63 @@ contain_nonstrict_functions_walker(Node *node, void
*context)
context);
}
+/*****************************************************************************
+ * Check clauses for inline-able functions
+ *****************************************************************************/
+
+bool
+contain_inlinable_functions(Node *node)
+{
+ return contain_inlinable_functions_walker(node, NULL);
+}
+
+/*
+ * can_inline_function - checks if a function is inline-able
+ */
+static bool
+can_inline_function(HeapTuple func_tuple)
+{
+ Form_pg_proc funcform = (Form_pg_proc) GETSTRUCT(func_tuple);
+
+ /*
+ * Nope if the function is not SQL-language or has other showstopper
+ * properties. (The prokind and nargs checks are just paranoia.)
+ */
+ return funcform->prolang == SQLlanguageId &&
+ funcform->prokind == PROKIND_FUNCTION &&
+ !funcform->prosecdef && !funcform->proretset &&
+ funcform->prorettype != RECORDOID &&
+ heap_attisnull(func_tuple, Anum_pg_proc_proconfig,
NULL);
+}
+
+static bool
+can_inline_function_checker(Oid funcid, void *context)
+{
+ HeapTuple func_tuple;
+ bool result;
+
+ func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+ if (!HeapTupleIsValid(func_tuple))
+ elog(ERROR, "cache lookup failed for function %u", funcid);
+
+ result = can_inline_function(func_tuple);
+ ReleaseSysCache(func_tuple);
+
+ return result;
+}
+
+static bool
+contain_inlinable_functions_walker(Node *node, void *context)
+{
+ if (node == NULL)
+ return false;
+ if (check_functions_in_node(node, can_inline_function_checker, context))
+ return true;
+
+ return expression_tree_walker(node, contain_inlinable_functions_walker,
+ context);
+}
+
/*****************************************************************************
* Check clauses for context-dependent nodes
*****************************************************************************/
@@ -4022,7 +4080,8 @@ simplify_function(Oid funcid, Oid result_type, int32
result_typmod,
Assert(newexpr != (Expr *) &fexpr);
}
- if (!newexpr && allow_non_const)
+ if (!newexpr && allow_non_const &&
+ can_inline_function(func_tuple))
newexpr = inline_function(funcid, result_type, result_collid,
input_collid,
args, funcvariadic,
func_tuple,
context);
@@ -4415,16 +4474,11 @@ inline_function(Oid funcid, Oid result_type, Oid
result_collid,
int i;
/*
- * Forget it if the function is not SQL-language or has other
showstopper
- * properties. (The prokind and nargs checks are just paranoia.)
+ * Caller should already have checked whether the function can be
inlined
+ * using can_function_inline().
*/
- if (funcform->prolang != SQLlanguageId ||
- funcform->prokind != PROKIND_FUNCTION ||
- funcform->prosecdef ||
- funcform->proretset ||
- funcform->prorettype == RECORDOID ||
- !heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL) ||
- funcform->pronargs != list_length(args))
+
+ if (funcform->pronargs != list_length(args))
return NULL;
/* Check for recursive function, and give up trying to expand if so */
diff --git a/src/include/optimizer/optimizer.h
b/src/include/optimizer/optimizer.h
index 5283995df8..4954657147 100644
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -130,6 +130,7 @@ extern Expr *canonicalize_qual(Expr *qual, bool is_check);
extern bool contain_mutable_functions(Node *clause);
extern bool contain_volatile_functions(Node *clause);
extern bool contain_volatile_functions_not_nextval(Node *clause);
+extern bool contain_inlinable_functions(Node *node);
extern Node *eval_const_expressions(PlannerInfo *root, Node *node);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 5acf604f63..244f7b576f 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -5758,9 +5758,10 @@ exec_eval_expr(PLpgSQL_execstate *estate,
Form_pg_attribute attr;
/*
- * If first time through, create a plan for this expression.
+ * Create a plan for this expression, if first time through or the
+ * existing plan is no longer valid.
*/
- if (expr->plan == NULL)
+ if (expr->plan == NULL || !SPI_plan_is_valid(expr->plan))
exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK, true);
/*
@@ -6076,7 +6077,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
{
ExprContext *econtext = estate->eval_econtext;
LocalTransactionId curlxid = MyProc->lxid;
- CachedPlan *cplan;
void *save_setup_arg;
bool need_snapshot;
MemoryContext oldcontext;
@@ -6093,29 +6093,39 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
if (expr->expr_simple_in_use && expr->expr_simple_lxid == curlxid)
return false;
- /*
- * Revalidate cached plan, so that we will notice if it became stale.
(We
- * need to hold a refcount while using the plan, anyway.) If replanning
- * is needed, do that work in the eval_mcontext.
- */
- oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
- cplan = SPI_plan_get_cached_plan(expr->plan);
- MemoryContextSwitchTo(oldcontext);
+ if (expr->expr_contains_inline_func)
+ {
+ CachedPlan *cplan;
- /*
- * We can't get a failure here, because the number of CachedPlanSources
in
- * the SPI plan can't change from what exec_simple_check_plan saw; it's
a
- * property of the raw parsetree generated from the query text.
- */
- Assert(cplan != NULL);
+ /*
+ * Revalidate cached plan, so that we will notice if it became
stale.
+ * (We need to hold a refcount while using the plan, anyway.)
If
+ * replanning is needed, do that work in the eval_mcontext.
+ */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
+ cplan = SPI_plan_get_cached_plan(expr->plan);
+ MemoryContextSwitchTo(oldcontext);
- /* If it got replanned, update our copy of the simple expression */
- if (cplan->generation != expr->expr_simple_generation)
- {
- exec_save_simple_expr(expr, cplan);
- /* better recheck r/w safety, as it could change due to
inlining */
- if (expr->rwparam >= 0)
- exec_check_rw_parameter(expr, expr->rwparam);
+ /*
+ * We can't get a failure here, because the number of
CachedPlanSources
+ * in the SPI plan can't change from what
exec_simple_check_plan saw;
+ * it's a property of the raw parsetree generated from the
query text.
+ */
+ Assert(cplan != NULL);
+
+ /* If it got replanned, update our copy of the simple
expression */
+ if (cplan->generation != expr->expr_simple_generation)
+ {
+ exec_save_simple_expr(expr, cplan);
+ /* better recheck r/w safety, as it could change due to
inlining */
+ if (expr->rwparam >= 0)
+ exec_check_rw_parameter(expr, expr->rwparam);
+ }
+
+ /*
+ * Now we can release our refcount on the cached plan.
+ */
+ ReleaseCachedPlan(cplan, true);
}
/*
@@ -6192,11 +6202,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
MemoryContextSwitchTo(oldcontext);
- /*
- * Now we can release our refcount on the cached plan.
- */
- ReleaseCachedPlan(cplan, true);
-
/*
* That's it.
*/
@@ -7890,9 +7895,11 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
/* ----------
- * exec_simple_check_plan - Check if a plan is simple enough to
+ * exec_simple_check_plan - Check if the expression is simple
enough to
* be evaluated by
ExecEvalExpr() instead
* of SPI.
+ *
+ * If it is, set expr->expr_simple_expr.
* ----------
*/
static void
@@ -7901,8 +7908,7 @@ exec_simple_check_plan(PLpgSQL_execstate *estate,
PLpgSQL_expr *expr)
List *plansources;
CachedPlanSource *plansource;
Query *query;
- CachedPlan *cplan;
- MemoryContext oldcontext;
+ Expr *tle_expr;
/*
* Initialize to "not simple".
@@ -7972,23 +7978,55 @@ exec_simple_check_plan(PLpgSQL_execstate *estate,
PLpgSQL_expr *expr)
return;
/*
- * OK, we can treat it as a simple plan.
- *
- * Get the generic plan for the query. If replanning is needed, do that
- * work in the eval_mcontext.
+ * We have a simple expression. Save it and initialize state to "not
+ * valid in current transaction".
*/
- oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
- cplan = SPI_plan_get_cached_plan(expr->plan);
- MemoryContextSwitchTo(oldcontext);
+ tle_expr = castNode(TargetEntry, linitial(query->targetList))->expr;
- /* Can't fail, because we checked for a single CachedPlanSource above */
- Assert(cplan != NULL);
+ if (contain_inlinable_functions((Node *) tle_expr))
+ {
+ CachedPlan *cplan = NULL;
+ MemoryContext oldcontext;
+
+ expr->expr_contains_inline_func = true;
- /* Share the remaining work with replan code path */
- exec_save_simple_expr(expr, cplan);
+ /*
+ * Get the generic plan for the query. If replanning is
needed, do
+ * that work in the eval_mcontext.
+ */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
+ cplan = SPI_plan_get_cached_plan(expr->plan);
+ MemoryContextSwitchTo(oldcontext);
- /* Release our plan refcount */
- ReleaseCachedPlan(cplan, true);
+ /*
+ * Can't fail, because we checked for a single CachedPlanSource
+ * above.
+ */
+ Assert(cplan != NULL);
+
+ exec_save_simple_expr(expr, cplan);
+
+ /* Release our plan refcount */
+ ReleaseCachedPlan(cplan, true);
+ }
+ else
+ {
+ /*
+ * Save the simple expression, and initialize state to "not
valid in
+ * current transaction".
+ */
+ expr->expr_simple_expr = tle_expr;
+ expr->expr_simple_generation = plansource->generation;
+ expr->expr_simple_state = NULL;
+ expr->expr_simple_in_use = false;
+ expr->expr_simple_lxid = InvalidLocalTransactionId;
+ /* Also stash away the expression result type */
+ expr->expr_simple_type = exprType((Node *) tle_expr);
+ expr->expr_simple_typmod = exprTypmod((Node *) tle_expr);
+ /* We also want to remember if it is immutable or not */
+ expr->expr_simple_mutable = contain_mutable_functions((Node *)
tle_expr);
+ expr->expr_contains_inline_func = false;
+ }
}
/*
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index b599f67fc5..0e02212135 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -245,6 +245,7 @@ typedef struct PLpgSQL_expr
ExprState *expr_simple_state; /* eval tree for expr_simple_expr */
bool expr_simple_in_use; /* true if eval tree is active */
LocalTransactionId expr_simple_lxid;
+ bool expr_contains_inline_func;
} PLpgSQL_expr;
/*
diff --git a/src/test/regress/expected/plpgsql.out
b/src/test/regress/expected/plpgsql.out
index cd2c79f4d5..3abbc7bc57 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -4289,12 +4289,10 @@ end
$$;
select fail();
ERROR: division by zero
-CONTEXT: SQL statement "SELECT 1/0"
-PL/pgSQL function fail() line 3 at RETURN
+CONTEXT: PL/pgSQL function fail() line 3 at RETURN
select fail();
ERROR: division by zero
-CONTEXT: SQL statement "SELECT 1/0"
-PL/pgSQL function fail() line 3 at RETURN
+CONTEXT: PL/pgSQL function fail() line 3 at RETURN
drop function fail();
-- Test handling of string literals.
set standard_conforming_strings = off;
