Hi,
On Sun, Feb 16, 2020 at 11:13 PM Pavel Stehule <pavel.steh...@gmail.com> wrote:
> when I do some profiling of plpgsql, usually I surprised how significant
> overhead has expression execution. Any calculations are very slow.
>
> This is not typical example of plpgsql, but it shows cleanly where is a
> overhead
>
> CREATE OR REPLACE FUNCTION public.foo()
> RETURNS void
> LANGUAGE plpgsql
> IMMUTABLE
> AS $function$
> declare i bigint = 0;
> begin
> while i < 100000000
> loop
> i := i + 1;
> end loop;
> end;
> $function$
>
> Profile of development version
>
> 10,04% plpgsql.so [.] exec_eval_simple_expr
> 9,17% postgres [.] AcquireExecutorLocks
> 7,01% postgres [.] ExecInterpExpr
> 5,86% postgres [.]
> OverrideSearchPathMatchesCurrent
> 4,71% postgres [.] GetCachedPlan
> 4,14% postgres [.] AcquirePlannerLocks
> 3,72% postgres [.] RevalidateCachedQuery
> 3,56% postgres [.] MemoryContextReset
> 3,43% plpgsql.so [.] plpgsql_param_eval_var
I was thinking about this overhead many months back and had even
written a patch to avoid going to the planner for "simple"
expressions, which can be handled by the executor. Here is what the
performance looks like:
HEAD:
latency: 31979.393 ms
18.32% postgres postgres [.] ExecInterpExpr
11.37% postgres plpgsql.so [.] exec_eval_expr
8.58% postgres plpgsql.so [.] plpgsql_param_eval_var
8.31% postgres plpgsql.so [.] exec_stmt
6.44% postgres postgres [.] GetCachedPlan
5.47% postgres postgres [.] AcquireExecutorLocks
5.30% postgres postgres [.] RevalidateCachedQuery
4.79% postgres plpgsql.so [.] exec_assign_value
4.41% postgres postgres [.] SPI_plan_get_cached_plan
4.36% postgres postgres [.] MemoryContextReset
4.22% postgres postgres [.] ReleaseCachedPlan
4.03% postgres postgres [.] OverrideSearchPathMatchesCurrent
2.63% postgres plpgsql.so [.] exec_assign_expr
2.11% postgres postgres [.] int84lt
1.95% postgres postgres [.] ResourceOwnerForgetPlanCacheRef
1.71% postgres postgres [.] int84pl
1.57% postgres postgres [.] ResourceOwnerRememberPlanCacheRef
1.38% postgres postgres [.] recomputeNamespacePath
1.35% postgres postgres [.] ScanQueryForLocks
1.24% postgres plpgsql.so [.] exec_cast_value
0.38% postgres postgres [.] ResourceOwnerEnlargePlanCacheRefs
0.05% postgres [kernel.kallsyms] [k] __do_softirq
0.03% postgres postgres [.] GetUserId
Patched:
latency: 21011.871 ms
28.26% postgres postgres [.] ExecInterpExpr
12.26% postgres plpgsql.so [.] plpgsql_param_eval_var
12.02% postgres plpgsql.so [.] exec_stmt
11.10% postgres plpgsql.so [.] exec_eval_expr
10.05% postgres postgres [.] SPI_plan_is_valid
7.09% postgres postgres [.] MemoryContextReset
6.65% postgres plpgsql.so [.] exec_assign_value
3.53% postgres plpgsql.so [.] exec_assign_expr
2.91% postgres postgres [.] int84lt
2.61% postgres postgres [.] int84pl
2.42% postgres plpgsql.so [.] exec_cast_value
0.86% postgres postgres [.] CachedPlanIsValid
0.16% postgres plpgsql.so [.] SPI_plan_is_valid@plt
0.05% postgres [kernel.kallsyms] [k] __do_softirq
0.03% postgres [kernel.kallsyms] [k] finish_task_switch
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.
Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b7c8d663fc..b292948853 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16539,9 +16539,10 @@ CloneRowTriggersToPartition(Relation parent, Relation
partition)
* Internal triggers require careful examination. Ideally, we
don't
* clone them.
*
- * However, if our parent is a partitioned relation, there
might be
- * internal triggers that need cloning. In that case, we must
skip
- * clone it if the trigger on parent depends on another trigger.
+ * However, if our parent is a partition itself, there might be
+ * internal triggers that need cloning. For example, triggers
on the
+ * parent that were in turn cloned from its own parent are
marked
+ * internal, which too must be cloned to the partition.
*
* Note we dare not verify that the other trigger belongs to an
* ancestor relation of our parent, because that creates
deadlock
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 5acf604f63..72f156726f 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -319,8 +319,7 @@ static void exec_eval_cleanup(PLpgSQL_execstate *estate);
static void exec_prepare_plan(PLpgSQL_execstate *estate,
PLpgSQL_expr *expr,
int cursorOptions,
bool keepplan);
-static void exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr
*expr);
-static void exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan);
+static void exec_check_simple_expr(PLpgSQL_execstate *estate, PLpgSQL_expr
*expr);
static void exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno);
static bool contains_target_param(Node *node, int *target_dno);
static bool exec_eval_simple_expr(PLpgSQL_execstate *estate,
@@ -4062,7 +4061,7 @@ exec_prepare_plan(PLpgSQL_execstate *estate,
expr->plan = plan;
/* Check to see if it's a simple expression */
- exec_simple_check_plan(estate, expr);
+ exec_check_simple_expr(estate, expr);
/*
* Mark expression as not using a read-write param. exec_assign_value
has
@@ -5758,9 +5757,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 +6076,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,30 +6092,15 @@ 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);
+ /* Simple expressions aren't planned, so generation wouldn't change. */
+ Assert(expr->expr_simple_generation == 0);
/*
- * 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.
+ * better recheck r/w safety, as it could change due to inlining
+ * XXX - should no longer occur, because planning is not done?
*/
- 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);
- }
+ if (expr->rwparam >= 0)
+ exec_check_rw_parameter(expr, expr->rwparam);
/*
* Pass back previously-determined result type.
@@ -6192,11 +6176,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,19 +7869,20 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
/* ----------
- * exec_simple_check_plan - Check if a plan is simple enough to
+ * exec_check_simple_expr - 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
-exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
+exec_check_simple_expr(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
{
List *plansources;
CachedPlanSource *plansource;
Query *query;
- CachedPlan *cplan;
- MemoryContext oldcontext;
+ Expr *tle_expr;
/*
* Initialize to "not simple".
@@ -7972,95 +7952,12 @@ 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.
- */
- 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);
-
- /* Share the remaining work with replan code path */
- exec_save_simple_expr(expr, cplan);
-
- /* Release our plan refcount */
- ReleaseCachedPlan(cplan, true);
-}
-
-/*
- * exec_save_simple_expr --- extract simple expression from CachedPlan
- */
-static void
-exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan)
-{
- PlannedStmt *stmt;
- Plan *plan;
- Expr *tle_expr;
-
- /*
- * Given the checks that exec_simple_check_plan did, none of the Asserts
- * here should ever fail.
- */
-
- /* Extract the single PlannedStmt */
- Assert(list_length(cplan->stmt_list) == 1);
- stmt = linitial_node(PlannedStmt, cplan->stmt_list);
- Assert(stmt->commandType == CMD_SELECT);
-
- /*
- * Ordinarily, the plan node should be a simple Result. However, if
- * force_parallel_mode is on, the planner might've stuck a Gather node
- * atop that. The simplest way to deal with this is to look through the
- * Gather node. The Gather node's tlist would normally contain a Var
- * referencing the child node's output, but it could also be a Param, or
- * it could be a Const that setrefs.c copied as-is.
- */
- plan = stmt->planTree;
- for (;;)
- {
- /* Extract the single tlist expression */
- Assert(list_length(plan->targetlist) == 1);
- tle_expr = castNode(TargetEntry,
linitial(plan->targetlist))->expr;
-
- if (IsA(plan, Result))
- {
- Assert(plan->lefttree == NULL &&
- plan->righttree == NULL &&
- plan->initPlan == NULL &&
- plan->qual == NULL &&
- ((Result *) plan)->resconstantqual == NULL);
- break;
- }
- else if (IsA(plan, Gather))
- {
- Assert(plan->lefttree != NULL &&
- plan->righttree == NULL &&
- plan->initPlan == NULL &&
- plan->qual == NULL);
- /* If setrefs.c copied up a Const, no need to look
further */
- if (IsA(tle_expr, Const))
- break;
- /* Otherwise, it had better be a Param or an outer Var
*/
- Assert(IsA(tle_expr, Param) ||(IsA(tle_expr, Var) &&
-
((Var *) tle_expr)->varno == OUTER_VAR));
- /* Descend to the child node */
- plan = plan->lefttree;
- }
- else
- elog(ERROR, "unexpected plan node type: %d",
- (int) nodeTag(plan));
- }
-
- /*
- * Save the simple expression, and initialize state to "not valid in
- * current transaction".
+ * We have a simple expression. Save it and initialize state to "not
+ * valid in current transaction".
*/
+ tle_expr = castNode(TargetEntry, linitial(query->targetList))->expr;
expr->expr_simple_expr = tle_expr;
- expr->expr_simple_generation = cplan->generation;
+ expr->expr_simple_generation = plansource->generation;
expr->expr_simple_state = NULL;
expr->expr_simple_in_use = false;
expr->expr_simple_lxid = InvalidLocalTransactionId;
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;
