Fix possible crash at transaction end when a plpgsql function is used and

then modified within the same transaction.  The code was using a linked list
of active PLpgSQL_expr structs, which was OK when it was written because
plpgsql never released any parse data structures for the life of the backend.
But since Neil fixed plpgsql's memory management, elements of the linked list
could be freed, leading to crash when the list is chased.  Per report and test
case from Kris Jurka.

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index e28e74e..efcf86e 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3,7 +3,7 @@
  *                       procedural language
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.160 2006/01/10 18:50:43 neilc Exp $
+ *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.161 2006/03/02 05:34:12 tgl Exp $
  *
  *       This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -64,12 +64,8 @@ static const char *const raise_skip_msg = "RAISE";
  * 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).
- *
- * If a simple PLpgSQL_expr has been used in the current xact, it is
- * linked into the active_simple_exprs list.
  */
 static EState *simple_eval_estate = NULL;
-static PLpgSQL_expr *active_simple_exprs = NULL;
 
 /************************************************************
  * Local function forward declarations
@@ -3783,6 +3779,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 {
        Datum           retval;
        ExprContext *econtext = estate->eval_econtext;
+       TransactionId curxid = GetTopTransactionId();
        ParamListInfo paramLI;
        int                     i;
        Snapshot        saveActiveSnapshot;
@@ -3796,13 +3793,11 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
         * Prepare the expression for execution, if it's not been done already in
         * the current transaction.
         */
-       if (expr->expr_simple_state == NULL)
+       if (expr->expr_simple_xid != curxid)
        {
                expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr,
                                                                                                  simple_eval_estate);
-               /* Add it to list for cleanup */
-               expr->expr_simple_next = active_simple_exprs;
-               active_simple_exprs = expr;
+               expr->expr_simple_xid = curxid;
        }
 
        /*
@@ -4454,11 +4449,11 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
 
        /*
         * Yes - this is a simple expression.  Mark it as such, and initialize
-        * state to "not executing".
+        * state to "not valid in current transaction".
         */
        expr->expr_simple_expr = tle->expr;
        expr->expr_simple_state = NULL;
-       expr->expr_simple_next = NULL;
+       expr->expr_simple_xid = InvalidTransactionId;
        /* Also stash away the expression result type */
        expr->expr_simple_type = exprType((Node *) tle->expr);
 }
@@ -4502,8 +4497,7 @@ exec_set_found(PLpgSQL_execstate *estate, bool state)
  * 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, and we have to mark all active PLpgSQL_expr
- * structs that are using it as no longer active.
+ * 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?
@@ -4512,18 +4506,6 @@ exec_set_found(PLpgSQL_execstate *estate, bool state)
 void
 plpgsql_xact_cb(XactEvent event, void *arg)
 {
-       PLpgSQL_expr *expr;
-       PLpgSQL_expr *enext;
-
-       /* Mark all active exprs as inactive */
-       for (expr = active_simple_exprs; expr; expr = enext)
-       {
-               enext = expr->expr_simple_next;
-               expr->expr_simple_state = NULL;
-               expr->expr_simple_next = NULL;
-       }
-       active_simple_exprs = NULL;
-
        /*
         * If we are doing a clean transaction shutdown, free the EState (so that
         * any remaining resources will be released correctly). In an abort, we

diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index d9e3a89..ab235c7 100644 (file)
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -3,7 +3,7 @@
  *                       procedural language
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.66 2005/11/22 18:17:33 momjian Exp $
+ *       $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.67 2006/03/02 05:34:12 tgl Exp $
  *
  *       This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -200,9 +200,12 @@ typedef struct PLpgSQL_expr
        /* fields for "simple expression" fast-path execution: */
        Expr       *expr_simple_expr;           /* NULL means not a simple expr */
        Oid                     expr_simple_type;
-       /* if expr is simple AND in use in current xact, these fields are set: */
+       /*
+        * 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;
-       struct PLpgSQL_expr *expr_simple_next;
+       TransactionId expr_simple_xid;
        /* params to pass to expr */
        int                     nparams;
        int                     params[1];              /* VARIABLE SIZE ARRAY ... must be last */