SEO

The following testcase(extracted from a much much larger production code
sample) results in

WARNING: TupleDesc reference leak: TupleDesc 0xb3573b88 (2249,1) still
referenced
CONTEXT: PL/pgSQL function "foo" line 4 at block variables initialization
ERROR: tupdesc reference 0xb3573b88 is not owned by resource owner Portal
CONTEXT: PL/pgSQL function "foo" while casting return value to
function's return type

on 8.2 and -HEAD.

8.1 seems to work fine.

Stefan


CREATE OR REPLACE FUNCTION public.foo() RETURNS INTEGER AS $$
DECLARE
v_var INTEGER;
BEGIN
BEGIN
v_var := (bar()).error_code;
EXCEPTION WHEN others THEN
RETURN 0;
END;
RETURN 0;
END;
$$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION public.bar(OUT error_code INTEGER, OUT new_id
INTEGER) RETURNS RECORD AS $$
BEGIN
error_code := 1;
new_id := 1;
RETURN;
END;
$$ LANGUAGE plpgsql;

SELECT * FROM public.foo();

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> writes:
> The following testcase(extracted from a much much larger production code
> sample) results in

> WARNING: TupleDesc reference leak: TupleDesc 0xb3573b88 (2249,1) still
> referenced
> CONTEXT: PL/pgSQL function "foo" line 4 at block variables initialization
> ERROR: tupdesc reference 0xb3573b88 is not owned by resource owner Portal
> CONTEXT: PL/pgSQL function "foo" while casting return value to
> function's return type

Hmm. What's happening is that the record-function call creates a
reference-counted TupleDesc, and tracking of the TupleDesc is
assigned to the subtransaction resource owner because we're inside
an EXCEPTION-block subtransaction. But the pointer is held by the
function's eval_context which lives throughout the function call,
and so the free happens long after exiting the subtransaction, and
the resource owner code quite properly complains about this.

In this particular case the worst consequence would be a short-term
memory leak, but I think there are probably variants with worse
problems, because anything done by a RegisterExprContextCallback()
callback is equally at risk.

I think the proper fix is probably to establish a new eval_context
when we enter an EXCEPTION block, and destroy it again on the way out.
Slightly annoying, but probably small next to the other overhead of
a subtransaction. Comments?

BTW, both of the CONTEXT lines are misleading. The WARNING happens
during exit from the begin-block, not entry to it; and the ERROR
happens after we've finished fooling with the result value. I'm
tempted to add a few more assignments to err_text to make this nicer.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Tom Lane wrote:
> Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> writes:
>> The following testcase(extracted from a much much larger production code
>> sample) results in
>
>> WARNING: TupleDesc reference leak: TupleDesc 0xb3573b88 (2249,1) still
>> referenced
>> CONTEXT: PL/pgSQL function "foo" line 4 at block variables initialization
>> ERROR: tupdesc reference 0xb3573b88 is not owned by resource owner Portal
>> CONTEXT: PL/pgSQL function "foo" while casting return value to
>> function's return type
>
> Hmm. What's happening is that the record-function call creates a
> reference-counted TupleDesc, and tracking of the TupleDesc is
> assigned to the subtransaction resource owner because we're inside
> an EXCEPTION-block subtransaction. But the pointer is held by the
> function's eval_context which lives throughout the function call,
> and so the free happens long after exiting the subtransaction, and
> the resource owner code quite properly complains about this.
>
> In this particular case the worst consequence would be a short-term
> memory leak, but I think there are probably variants with worse
> problems, because anything done by a RegisterExprContextCallback()
> callback is equally at risk.
>
> I think the proper fix is probably to establish a new eval_context
> when we enter an EXCEPTION block, and destroy it again on the way out.
> Slightly annoying, but probably small next to the other overhead of
> a subtransaction. Comments?

we use exception blocks heavily here so anything that makes them slower
is not nice but if it fixes the issue at hand I'm all for it ...

>
> BTW, both of the CONTEXT lines are misleading. The WARNING happens
> during exit from the begin-block, not entry to it; and the ERROR
> happens after we've finished fooling with the result value. I'm
> tempted to add a few more assignments to err_text to make this nicer.

yeah wondered about that too when I tried to produce a simple testcase -
the errors did't seem to make much sense in the context of what
triggered them. Improving that would be a very godd thing to do.


Stefan

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> writes:
> Tom Lane wrote:
>> I think the proper fix is probably to establish a new eval_context
>> when we enter an EXCEPTION block, and destroy it again on the way out.
>> Slightly annoying, but probably small next to the other overhead of
>> a subtransaction. Comments?

> we use exception blocks heavily here so anything that makes them slower
> is not nice but if it fixes the issue at hand I'm all for it ...

This turned out a bit uglier than I thought --- the real problem is that
plpgsql's "simple eval econtext" management is much too stupid to
survive in a subtransaction world. There was a comment in the code
worrying about this, but I guess we never investigated closely.

The attached patch (against 8.2) appears to fix the reported problem,
but it could use some more testing before I push it into the stable
branches. Can you try it in the production situation that exposed the
problem? Aside from not failing, do you see any performance loss?

regards, tom lane


Index: pl_exec.c
================================================== =================
RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.180
diff -c -r1.180 pl_exec.c
*** pl_exec.c 4 Oct 2006 00:30:13 -0000 1.180
--- pl_exec.c 24 Jan 2007 21:46:33 -0000
***************
*** 37,51 ****

static const char *const raise_skip_msg = "RAISE";

-
/*
! * All plpgsql function executions within a single transaction share
! * the same executor EState for evaluating "simple" expressions. Each
! * function call creates its own "eval_econtext" ExprContext within this
! * estate. We destroy the estate at transaction shutdown to ensure there
! * is no permanent leakage of memory (especially for xact abort case).
! */
! static EState *simple_eval_estate = NULL;

/************************************************** **********
* Local function forward declarations
--- 37,69 ----

static const char *const raise_skip_msg = "RAISE";

/*
! * All plpgsql function executions within a single transaction share the same
! * executor EState for evaluating "simple" expressions. Each function call
! * creates its own "eval_econtext" ExprContext within this estate for
! * per-evaluation workspace. eval_econtext is freed at normal function exit,
! * and the EState is freed at transaction end (in case of error, we assume
! * that the abort mechanisms clean it all up). In order to be sure
! * ExprContext callbacks are handled properly, each subtransaction has to have
! * its own such EState; hence we need a stack. We use a simple counter to
! * distinguish different instantiations of the EState, so that we can tell
! * whether we have a current copy of a prepared expression.
! *
! * This arrangement is a bit tedious to maintain, but it's worth the trouble
! * so that we don't have to re-prepare simple expressions on each trip through
! * a function. (We assume the case to optimize is many repetitions of a
! * function within a transaction.)
! */
! typedef struct SimpleEstateStackEntry
! {
! EState *xact_eval_estate; /* EState for current xact level */
! long int xact_estate_simple_id; /* ID for xact_eval_estate */
! SubTransactionId xact_subxid; /* ID for current subxact */
! struct SimpleEstateStackEntry *next; /* next stack entry up */
! } SimpleEstateStackEntry;
!
! static SimpleEstateStackEntry *simple_estate_stack = NULL;
! static long int simple_estate_id_counter = 0;

/************************************************** **********
* Local function forward declarations
***************
*** 154,159 ****
--- 172,178 ----
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
static bool compatible_tupdesc(TupleDesc td1, TupleDesc td2);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
+ static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
static void free_var(PLpgSQL_var *var);


***************
*** 892,897 ****
--- 911,919 ----
*/
MemoryContext oldcontext = CurrentMemoryContext;
ResourceOwner oldowner = CurrentResourceOwner;
+ ExprContext *old_eval_econtext = estate->eval_econtext;
+ EState *old_eval_estate = estate->eval_estate;
+ long int old_eval_estate_simple_id = estate->eval_estate_simple_id;

BeginInternalSubTransaction(NULL);
/* Want to run statements inside function's memory context */
***************
*** 899,904 ****
--- 921,935 ----

PG_TRY();
{
+ /*
+ * We need to run the block's statements with a new eval_econtext
+ * that belongs to the current subtransaction; if we try to use
+ * the outer econtext then ExprContext shutdown callbacks will be
+ * called at the wrong times.
+ */
+ plpgsql_create_econtext(estate);
+
+ /* Run the block's statements */
rc = exec_stmts(estate, block->body);

/* Commit the inner transaction, return to outer xact context */
***************
*** 906,911 ****
--- 937,947 ----
MemoryContextSwitchTo(oldcontext);
CurrentResourceOwner = oldowner;

+ /* Revert to outer eval_econtext */
+ estate->eval_econtext = old_eval_econtext;
+ estate->eval_estate = old_eval_estate;
+ estate->eval_estate_simple_id = old_eval_estate_simple_id;
+
/*
* AtEOSubXact_SPI() should not have popped any SPI context, but
* just in case it did, make sure we remain connected.
***************
*** 927,932 ****
--- 963,973 ----
MemoryContextSwitchTo(oldcontext);
CurrentResourceOwner = oldowner;

+ /* Revert to outer eval_econtext */
+ estate->eval_econtext = old_eval_econtext;
+ estate->eval_estate = old_eval_estate;
+ estate->eval_estate_simple_id = old_eval_estate_simple_id;
+
/*
* If AtEOSubXact_SPI() popped any SPI context of the subxact, it
* will have left us in a disconnected state. We need this hack
***************
*** 2139,2162 ****
estate->err_text = NULL;

/*
! * Create an EState for evaluation of simple expressions, if there's not
! * one already in the current transaction. The EState is made a child of
! * TopTransactionContext so it will have the right lifespan.
*/
! if (simple_eval_estate == NULL)
! {
! MemoryContext oldcontext;
!
! oldcontext = MemoryContextSwitchTo(TopTransactionContext);
! simple_eval_estate = CreateExecutorState();
! MemoryContextSwitchTo(oldcontext);
! }
!
! /*
! * Create an expression context for simple expressions. This must be a
! * child of simple_eval_estate.
! */
! estate->eval_econtext = CreateExprContext(simple_eval_estate);

/*
* Let the plugin see this function before we initialize any local
--- 2180,2188 ----
estate->err_text = NULL;

/*
! * Create an EState and ExprContext for evaluation of simple expressions.
*/
! plpgsql_create_econtext(estate);

/*
* Let the plugin see this function before we initialize any local
***************
*** 3917,3923 ****
{
Datum retval;
ExprContext *econtext = estate->eval_econtext;
- TransactionId curxid = GetTopTransactionId();
ParamListInfo paramLI;
int i;
Snapshot saveActiveSnapshot;
--- 3943,3948 ----
***************
*** 3929,3941 ****

/*
* Prepare the expression for execution, if it's not been done already in
! * the current transaction.
*/
! if (expr->expr_simple_xid != curxid)
{
expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr,
! simple_eval_estate);
! expr->expr_simple_xid = curxid;
}

/*
--- 3954,3966 ----

/*
* Prepare the expression for execution, if it's not been done already in
! * the current eval_estate.
*/
! if (expr->expr_simple_id != estate->eval_estate_simple_id)
{
expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr,
! estate->eval_estate);
! expr->expr_simple_id = estate->eval_estate_simple_id;
}

/*
***************
*** 4600,4606 ****
*/
expr->expr_simple_expr = tle->expr;
expr->expr_simple_state = NULL;
! expr->expr_simple_xid = InvalidTransactionId;
/* Also stash away the expression result type */
expr->expr_simple_type = exprType((Node *) tle->expr);
}
--- 4625,4631 ----
*/
expr->expr_simple_expr = tle->expr;
expr->expr_simple_state = NULL;
! expr->expr_simple_id = -1;
/* Also stash away the expression result type */
expr->expr_simple_type = exprType((Node *) tle->expr);
}
***************
*** 4641,4654 ****
}

/*
* plpgsql_xact_cb --- post-transaction-commit-or-abort cleanup
*
! * If a simple_eval_estate was created in the current transaction,
* it has to be cleaned up.
- *
- * XXX Do we need to do anything at subtransaction events?
- * Maybe subtransactions need to have their own simple_eval_estate?
- * It would get a lot messier, so for now let's assume we don't need that.
*/
void
plpgsql_xact_cb(XactEvent event, void *arg)
--- 4666,4720 ----
}

/*
+ * plpgsql_create_econtext --- create an eval_econtext for the current function
+ *
+ * We may need to create a new eval_estate too, if there's not one already
+ * for the current (sub) transaction. The EState will be cleaned up at
+ * (sub) transaction end.
+ */
+ static void
+ plpgsql_create_econtext(PLpgSQL_execstate *estate)
+ {
+ SubTransactionId my_subxid = GetCurrentSubTransactionId();
+ SimpleEstateStackEntry *entry = simple_estate_stack;
+
+ /* Create new EState if not one for current subxact */
+ if (entry == NULL ||
+ entry->xact_subxid != my_subxid)
+ {
+ MemoryContext oldcontext;
+
+ /* Stack entries are kept in TopTransactionContext for simplicity */
+ entry = (SimpleEstateStackEntry *)
+ MemoryContextAlloc(TopTransactionContext,
+ sizeof(SimpleEstateStackEntry));
+
+ /* But each EState should be a child of its CurTransactionContext */
+ oldcontext = MemoryContextSwitchTo(CurTransactionContext);
+ entry->xact_eval_estate = CreateExecutorState();
+ MemoryContextSwitchTo(oldcontext);
+
+ /* Assign a reasonably-unique ID to this EState */
+ entry->xact_estate_simple_id = simple_estate_id_counter++;
+ entry->xact_subxid = my_subxid;
+
+ entry->next = simple_estate_stack;
+ simple_estate_stack = entry;
+ }
+
+ /* Link plpgsql estate to it */
+ estate->eval_estate = entry->xact_eval_estate;
+ estate->eval_estate_simple_id = entry->xact_estate_simple_id;
+
+ /* And create a child econtext for the current function */
+ estate->eval_econtext = CreateExprContext(estate->eval_estate);
+ }
+
+ /*
* plpgsql_xact_cb --- post-transaction-commit-or-abort cleanup
*
! * If a simple-expression EState was created in the current transaction,
* it has to be cleaned up.
*/
void
plpgsql_xact_cb(XactEvent event, void *arg)
***************
*** 4657,4667 ****
* If we are doing a clean transaction shutdown, free the EState (so that
* any remaining resources will be released correctly). In an abort, we
* expect the regular abort recovery procedures to release everything of
! * interest.
*/
! if (event == XACT_EVENT_COMMIT && simple_eval_estate)
! FreeExecutorState(simple_eval_estate);
! simple_eval_estate = NULL;
}

static void
--- 4723,4770 ----
* If we are doing a clean transaction shutdown, free the EState (so that
* any remaining resources will be released correctly). In an abort, we
* expect the regular abort recovery procedures to release everything of
! * interest. We don't need to free the individual stack entries since
! * TopTransactionContext is about to go away anyway.
! *
! * Note: if plpgsql_subxact_cb is doing its job, there should be at most
! * one stack entry, but we may as well code this as a loop.
*/
! if (event != XACT_EVENT_ABORT)
! {
! while (simple_estate_stack != NULL)
! {
! FreeExecutorState(simple_estate_stack->xact_eval_estate);
! simple_estate_stack = simple_estate_stack->next;
! }
! }
! else
! simple_estate_stack = NULL;
! }
!
! /*
! * plpgsql_subxact_cb --- post-subtransaction-commit-or-abort cleanup
! *
! * If a simple-expression EState was created in the current subtransaction,
! * it has to be cleaned up.
! */
! void
! plpgsql_subxact_cb(SubXactEvent event, SubTransactionId mySubid,
! SubTransactionId parentSubid, void *arg)
! {
! if (event == SUBXACT_EVENT_START_SUB)
! return;
!
! if (simple_estate_stack != NULL &&
! simple_estate_stack->xact_subxid == mySubid)
! {
! SimpleEstateStackEntry *next;
!
! if (event == SUBXACT_EVENT_COMMIT_SUB)
! FreeExecutorState(simple_estate_stack->xact_eval_estate);
! next = simple_estate_stack->next;
! pfree(simple_estate_stack);
! simple_estate_stack = next;
! }
}

static void
Index: pl_handler.c
================================================== =================
RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/pl_handler.c,v
retrieving revision 1.33
diff -c -r1.33 pl_handler.c
*** pl_handler.c 19 Oct 2006 18:32:48 -0000 1.33
--- pl_handler.c 24 Jan 2007 21:46:33 -0000
***************
*** 46,51 ****
--- 46,52 ----

plpgsql_HashTableInit();
RegisterXactCallback(plpgsql_xact_cb, NULL);
+ RegisterSubXactCallback(plpgsql_subxact_cb, NULL);

/* Set up a rendezvous point with optional instrumentation plugin */
plugin_ptr = (PLpgSQL_plugin **) find_rendezvous_variable("PLpgSQL_plugin");
Index: plpgsql.h
================================================== =================
RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/plpgsql.h,v
retrieving revision 1.81
diff -c -r1.81 plpgsql.h
*** plpgsql.h 4 Oct 2006 00:30:14 -0000 1.81
--- plpgsql.h 24 Jan 2007 21:46:34 -0000
***************
*** 180,190 ****
Oid expr_simple_type;

/*
! * if expr is simple AND in use in current xact, expr_simple_state is
! * valid. Test validity by seeing if expr_simple_xid matches current XID.
*/
ExprState *expr_simple_state;
! TransactionId expr_simple_xid;
/* params to pass to expr */
int nparams;
int params[1]; /* VARIABLE SIZE ARRAY ... must be last */
--- 180,192 ----
Oid expr_simple_type;

/*
! * if expr is simple AND prepared in current eval_estate,
! * expr_simple_state is valid. Test validity by seeing if expr_simple_id
! * matches eval_estate_simple_id.
*/
ExprState *expr_simple_state;
! long int expr_simple_id;
!
/* params to pass to expr */
int nparams;
int params[1]; /* VARIABLE SIZE ARRAY ... must be last */
***************
*** 612,618 ****
SPITupleTable *eval_tuptable;
uint32 eval_processed;
Oid eval_lastoid;
! ExprContext *eval_econtext;

/* status information for error context reporting */
PLpgSQL_function *err_func; /* current func */
--- 614,622 ----
SPITupleTable *eval_tuptable;
uint32 eval_processed;
Oid eval_lastoid;
! ExprContext *eval_econtext; /* for executing simple expressions */
! EState *eval_estate; /* EState containing eval_econtext */
! long int eval_estate_simple_id; /* ID for eval_estate */

/* status information for error context reporting */
PLpgSQL_function *err_func; /* current func */
***************
*** 738,743 ****
--- 742,749 ----
extern HeapTuple plpgsql_exec_trigger(PLpgSQL_function *func,
TriggerData *trigdata);
extern void plpgsql_xact_cb(XactEvent event, void *arg);
+ extern void plpgsql_subxact_cb(SubXactEvent event, SubTransactionId mySubid,
+ SubTransactionId parentSubid, void *arg);

/* ----------
* Functions for the dynamic string handling in pl_funcs.c


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Tom Lane wrote:
> Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> writes:
>> Tom Lane wrote:
>>> I think the proper fix is probably to establish a new eval_context
>>> when we enter an EXCEPTION block, and destroy it again on the way out.
>>> Slightly annoying, but probably small next to the other overhead of
>>> a subtransaction. Comments?
>
>> we use exception blocks heavily here so anything that makes them slower
>> is not nice but if it fixes the issue at hand I'm all for it ...
>
> This turned out a bit uglier than I thought --- the real problem is that
> plpgsql's "simple eval econtext" management is much too stupid to
> survive in a subtransaction world. There was a comment in the code
> worrying about this, but I guess we never investigated closely.
>
> The attached patch (against 8.2) appears to fix the reported problem,
> but it could use some more testing before I push it into the stable
> branches. Can you try it in the production situation that exposed the
> problem? Aside from not failing, do you see any performance loss?

thanks - this seems to fix the problem on the development system but it
might take a while to get some performance testing done.


Stefan

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster