I wrote:
>> Heikki Linnakangas <heikki.linnakan...@enterprisedb.com> writes:
>>> One simple idea is to keep a flag along with the executor state to
>>> indicate that the executor state is currently in use. Set it just before
>>> calling ExecEvalExpr, and reset afterwards. If the flag is already set
>>> in the beginning of exec_eval_simple_expr, we have recursed, and must
>>> create a new executor state.
>> Yeah, the same thought occurred to me in the shower this morning.
>> I'm concerned about possible memory leakage during repeated recursion,
>> but maybe that can be dealt with.
I spent quite a bit of time trying to deal with the memory-leakage
problem without adding still more bookkeeping overhead. It wasn't
looking good, and then I had a sudden insight: if we see that the in-use
flag is set, we can simply return FALSE from exec_eval_simple_expr.
That causes exec_eval_expr to run the expression using the "non simple"
code path, which is perfectly safe because it isn't trying to reuse
state that might be dirty. Thus the attached patch, which fixes
both of the failure cases discussed in this thread.
Advantages:
1. The slowdown for "normal" cases (non-recursive, non-error-inducing)
is negligible.
2. It's simple enough to back-patch without fear.
Disadvantages:
1. Recursive cases get noticeably slower, about 4X slower according
to tests with this example:
create or replace function factorial(n int) returns float8 as $$
begin
if (n > 1) then
return n * factorial(n - 1);
end if;
return 1;
end
$$ language plpgsql strict immutable;
The slowdown is only for the particular expression that actually has
invoked a recursive call, so the above is probably much the worst case
in practice. I doubt many people really use plpgsql this way, but ...
2. Cases where we're re-executing an expression that failed earlier in
the same transaction likewise get noticeably slower. This is only a
problem if you're using subtransactions to catch errors, and the
overhead of the subtransaction is going to be large enough to partially
hide the extra eval cost anyway. So I didn't bother to make a timing
test case --- it's not going to be as bad as the example above.
I currently think that we don't have much choice except to use this
patch for the back branches: any better-performing alternative is
going to require enough surgery that back-patching would be dangerous.
Maybe somebody can come up with a better answer for HEAD, but I don't
have one.
Comments?
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 9929e04e57bbc7d08938765b7f506c05c07b772b..1f40f3cf69234ff650ece2ccb09791f8e075ec1a 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** loop_exit:
*** 4491,4497 ****
* a Datum by directly calling ExecEvalExpr().
*
* If successful, store results into *result, *isNull, *rettype and return
! * TRUE. If the expression is not simple (any more), return FALSE.
*
* It is possible though unlikely for a simple expression to become non-simple
* (consider for example redefining a trivial view). We must handle that for
--- 4491,4508 ----
* a Datum by directly calling ExecEvalExpr().
*
* If successful, store results into *result, *isNull, *rettype and return
! * TRUE. If the expression cannot be handled by simple evaluation,
! * return FALSE.
! *
! * Because we only store one execution tree for a simple expression, we
! * can't handle recursion cases. So, if we see the tree is already busy
! * with an evaluation in the current xact, we just return FALSE and let the
! * caller run the expression the hard way. (Other alternatives such as
! * creating a new tree for a recursive call either introduce memory leaks,
! * or add enough bookkeeping to be doubtful wins anyway.) Another case that
! * is covered by the expr_simple_in_use test is where a previous execution
! * of the tree was aborted by an error: the tree may contain bogus state
! * so we dare not re-use it.
*
* It is possible though unlikely for a simple expression to become non-simple
* (consider for example redefining a trivial view). We must handle that for
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 4528,4533 ****
--- 4539,4550 ----
return false;
/*
+ * If expression is in use in current xact, don't touch it.
+ */
+ 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
* also need to hold a refcount while using the plan.) Note that even if
* replanning occurs, the length of plancache_list can't change, since it
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 4562,4567 ****
--- 4579,4585 ----
{
expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr,
simple_eval_estate);
+ expr->expr_simple_in_use = false;
expr->expr_simple_lxid = curlxid;
}
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 4597,4602 ****
--- 4615,4625 ----
econtext->ecxt_param_list_info = paramLI;
/*
+ * Mark expression as busy for the duration of the ExecEvalExpr call.
+ */
+ expr->expr_simple_in_use = true;
+
+ /*
* Finally we can call the executor to evaluate the expression
*/
*result = ExecEvalExpr(expr->expr_simple_state,
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 4605,4610 ****
--- 4628,4635 ----
NULL);
/* Assorted cleanup */
+ expr->expr_simple_in_use = false;
+
estate->cur_expr = save_cur_expr;
if (!estate->readonly_func)
*************** exec_simple_check_plan(PLpgSQL_expr *exp
*** 5341,5346 ****
--- 5366,5372 ----
*/
expr->expr_simple_expr = tle->expr;
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);
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 5b174c49b8bcac4658ffd54fc4493e43221533e7..155796be0f5b2d1ad7850f5734fc6363d3c58c15 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** typedef struct PLpgSQL_expr
*** 214,223 ****
/*
* if expr is simple AND prepared in current transaction,
! * expr_simple_state is valid. Test validity by seeing if expr_simple_lxid
! * matches current LXID.
*/
! ExprState *expr_simple_state;
LocalTransactionId expr_simple_lxid;
} PLpgSQL_expr;
--- 214,225 ----
/*
* if expr is simple AND prepared in current transaction,
! * expr_simple_state and expr_simple_in_use are valid. Test validity by
! * seeing if expr_simple_lxid matches current LXID. (If not,
! * expr_simple_state probably points at garbage!)
*/
! 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;
} PLpgSQL_expr;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
