From 73b06cf893c9d3bb38c11878a12cc29407e78b6c Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 22 Nov 2019 15:02:18 -0500 Subject: [PATCH] Avoid taking a new snapshot for an immutable simple expression in plpgsql. We already used this optimization if the plpgsql function is read-only. But it seems okay to do it even in a read-write function, if the expression contains only immutable functions/operators. There would only be a change of behavior if an "immutable" called function depends on seeing database updates made during the current plpgsql function. That's enough of a violation of the promise of immutability that anyone who complains won't have much of a case. The benefits are significant --- for simple cases like while i < 10000000 loop i := i + 1; end loop; I see net performance improvements around 45%. Of course, real-world cases won't get that much faster, but it ought to be noticeable. At the very least, this removes much of the performance penalty that used to exist for forgetting to mark a plpgsql function non-volatile. Konstantin Knizhnik, reviewed by Pavel Stehule, cosmetic changes by me Discussion: https://postgr.es/m/ed9da20e-01aa-d04b-d085-e6c16b14b9d7@postgrespro.ru --- src/pl/plpgsql/src/pl_exec.c | 20 +++++++++++++++----- src/pl/plpgsql/src/plpgsql.h | 1 + 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index fd799b724da..4a9ed83030a 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -6079,6 +6079,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, LocalTransactionId curlxid = MyProc->lxid; CachedPlan *cplan; void *save_setup_arg; + bool need_snapshot; MemoryContext oldcontext; /* @@ -6150,12 +6151,19 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, /* * We have to do some of the things SPI_execute_plan would do, in - * particular advance the snapshot if we are in a non-read-only function. - * Without this, stable functions within the expression would fail to see - * updates made so far by our own function. + * particular push a new snapshot so that stable functions within the + * expression can see updates made so far by our own function. However, + * we can skip doing that (and just invoke the expression with the same + * snapshot passed to our function) in some cases, which is useful because + * it's quite expensive relative to the cost of a simple expression. We + * can skip it if the expression contains no stable or volatile functions; + * immutable functions shouldn't need to see our updates. Also, if this + * is a read-only function, we haven't made any updates so again it's okay + * to skip. */ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate)); - if (!estate->readonly_func) + need_snapshot = (expr->expr_simple_mutable && !estate->readonly_func); + if (need_snapshot) { CommandCounterIncrement(); PushActiveSnapshot(GetTransactionSnapshot()); @@ -6180,7 +6188,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, estate->paramLI->parserSetupArg = save_setup_arg; - if (!estate->readonly_func) + if (need_snapshot) PopActiveSnapshot(); MemoryContextSwitchTo(oldcontext); @@ -8051,6 +8059,8 @@ exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan) /* 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); } /* diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index f66b2baa70a..6b40fcd1963 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -234,6 +234,7 @@ typedef struct PLpgSQL_expr int expr_simple_generation; /* plancache generation we checked */ Oid expr_simple_type; /* result type Oid, if simple */ int32 expr_simple_typmod; /* result typmod, if simple */ + bool expr_simple_mutable; /* true if simple expr is mutable */ /* * if expr is simple AND prepared in current transaction,