On Wed, Feb 19, 2020 at 3:56 PM Amit Langote <amitlangot...@gmail.com> wrote:
> On Wed, Feb 19, 2020 at 3:38 PM Pavel Stehule <pavel.steh...@gmail.com> wrote:
> > st 19. 2. 2020 v 7:30 odesílatel Pavel Stehule <pavel.steh...@gmail.com>
> > napsal:
> >> út 18. 2. 2020 v 17:08 odesílatel Amit Langote <amitlangot...@gmail.com>
> >> napsal:
> >>> > 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.
> >>>
> >>> I polished it a bit.
> >>
> >>
> >> the performance looks very interesting - on my comp the execution time of
> >> 100000000 iterations was decreased from 34 sec to 15 sec,
> >>
> >> So it is interesting speedup
> >
> > but regress tests fails
>
> Oops, I failed to check src/pl/plpgsql tests.
>
> Fixed in the attached.
Added a regression test based on examples discussed here too.
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/expected/plpgsql_varprops.out
b/src/pl/plpgsql/src/expected/plpgsql_varprops.out
index 18f03d75b4..84a4ea7d03 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_varprops.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_varprops.out
@@ -76,8 +76,7 @@ begin
raise notice 'x = %', x;
end$$;
ERROR: division by zero
-CONTEXT: SQL statement "SELECT 1/0"
-PL/pgSQL function inline_code_block line 3 during statement block local
variable initialization
+CONTEXT: PL/pgSQL function inline_code_block line 3 during statement block
local variable initialization
do $$
declare x bigint[] := array[1,3,5];
begin
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 5acf604f63..07d468ce5d 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;
@@ -6094,28 +6094,39 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
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.
+ * Revalidate cached plan if one would have been used (due to
inline-able)
+ * functions being found in the expression), 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);
+ 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 +6203,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.
*/
@@ -7893,6 +7899,8 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
* exec_simple_check_plan - Check if a plan is simple enough to
* be evaluated by
ExecEvalExpr() instead
* of SPI.
+ *
+ * If it is, set expr->expr_simple_expr.
* ----------
*/
static void
@@ -7901,8 +7909,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 +7979,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. Although, if it contains inline-able
+ * SQL functions, better pass it through the planner to get a simpler
+ * plan.
*/
- oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
- cplan = SPI_plan_get_cached_plan(expr->plan);
- MemoryContextSwitchTo(oldcontext);
+ tle_expr = castNode(TargetEntry, linitial(query->targetList))->expr;
+ if (contain_inlinable_functions((Node *) tle_expr))
+ {
+ CachedPlan *cplan = NULL;
+ MemoryContext oldcontext;
+
+ expr->expr_contains_inline_func = true;
+
+ /*
+ * 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);
- /* Can't fail, because we checked for a single CachedPlanSource above */
- Assert(cplan != NULL);
+ /*
+ * Can't fail, because we checked for a single CachedPlanSource
+ * above.
+ */
+ Assert(cplan != NULL);
- /* Share the remaining work with replan code path */
- exec_save_simple_expr(expr, cplan);
+ exec_save_simple_expr(expr, cplan);
- /* Release our plan refcount */
- ReleaseCachedPlan(cplan, true);
+ /* 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..05559c927b 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;
@@ -5525,3 +5523,32 @@ END; $$ LANGUAGE plpgsql;
ERROR: "x" is not a scalar variable
LINE 3: GET DIAGNOSTICS x = ROW_COUNT;
^
+--
+-- Check simple expressions are handled correctly
+--
+DO LANGUAGE plpgsql $$
+DECLARE i bigint = 0;
+BEGIN
+ WHILE i < 100
+ LOOP
+ i := i + 1;
+ END LOOP;
+ RAISE NOTICE '%', i;
+END
+$$;
+NOTICE: 100
+-- Case where the expression is simple but contains inline-able function
+CREATE FUNCTION sql_incr (a bigint) RETURNS bigint AS $$
+SELECT a + 1;
+$$ IMMUTABLE LANGUAGE SQL;
+DO LANGUAGE plpgsql $$
+DECLARE i bigint = 0;
+BEGIN
+ WHILE i < 100
+ LOOP
+ i := sql_incr(i);
+ END LOOP;
+ RAISE NOTICE '%', i;
+END
+$$;
+NOTICE: 100
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index d841d8c0f9..bf2bb3c861 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4519,3 +4519,33 @@ BEGIN
GET DIAGNOSTICS x = ROW_COUNT;
RETURN;
END; $$ LANGUAGE plpgsql;
+
+--
+-- Check simple expressions are handled correctly
+--
+DO LANGUAGE plpgsql $$
+DECLARE i bigint = 0;
+BEGIN
+ WHILE i < 100
+ LOOP
+ i := i + 1;
+ END LOOP;
+ RAISE NOTICE '%', i;
+END
+$$;
+
+-- Case where the expression is simple but contains inline-able function
+CREATE FUNCTION sql_incr (a bigint) RETURNS bigint AS $$
+SELECT a + 1;
+$$ IMMUTABLE LANGUAGE SQL;
+
+DO LANGUAGE plpgsql $$
+DECLARE i bigint = 0;
+BEGIN
+ WHILE i < 100
+ LOOP
+ i := sql_incr(i);
+ END LOOP;
+ RAISE NOTICE '%', i;
+END
+$$;
