Обсуждение: crash in plancache with subtransactions

Hi,

A customer was hitting some misbehavior in one of their internal tests and
I tracked it down to plancache not behaving properly with
subtransactions: in particular, a plan is not being marked "dead" when
the subtransaction on which it is planned rolls back.  It was reported
in 8.4, but I can reproduce the problem on 9.0 too with this small
script:

    drop schema alvherre cascade;
    drop schema test cascade;
    create schema test;
    create schema alvherre;
    set search_path = 'alvherre';

    create or replace function dummy(text) returns text language sql
    as $$ SELECT relname::text FROM pg_class c WHERE c.oid = $1::regclass $$;

    create or replace function broken(p_name_table text) returns void
    language plpgsql as $$
    declare
    v_table_full text := alvherre.dummy(p_name_table);
    begin
        return;
    end;
    $$;

    BEGIN;
     create table test.stuffs (stuff text);
     SAVEPOINT a;
     select broken('nonexistant.stuffs');

     ROLLBACK TO a;
     select broken('test.stuffs');

    rollback;


The symptom is that the second call to broken() fails with this error
message:

ERROR:  relation "" does not exist
CONTEXT:  SQL function "dummy" statement 1
PL/pgSQL function "broken" line 3 during statement block local variable initialization

Note that this is totally bogus, because the relation being referenced
does indeed exist.  In fact, if you commit the transaction and call the
function again, it works.

Also, the state after the first call is a bit bogus: if you repeat the
whole sequence starting at the BEGIN line, it causes a crash on 8.4.

I hacked up plancache a bit so that it marks plans as dead when the
subtransaction resource owner releases it.  It adds a new arg to
ReleaseCachedPlan(); if true, the plan is marked dead.  All current
callers, except the one in ResourceOwnerReleaseInternal(), use false
thus preserving the current behavior.  resowner sets this as true when
aborting a (sub)transaction.

I have to admit that it seems somewhat the wrong API, but I don't see a
better way.  (I thought above relcache or syscache inval, but as far as
I can't tell there isn't any here).  I'm open to suggestions.

Patch attached.

--
Álvaro Herrera <alvherre@alvh.no-ip.org>

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> A customer was hitting some misbehavior in one of their internal tests and
> I tracked it down to plancache not behaving properly with
> subtransactions: in particular, a plan is not being marked "dead" when
> the subtransaction on which it is planned rolls back.

I don't believe that it's plancache's fault; the real problem is that
plpgsql is keeping "simple expression" execution trees around longer
than it should.  Your patch masks the problem by forcing those trees to
be rebuilt, but it's the execution trees not the plan trees that contain
stale data.

I'm not immediately sure why plpgsql_subxact_cb is failing to clean up
correctly in this example, but that seems to be where to look.
        regards, tom lane

Excerpts from Tom Lane's message of jue oct 21 19:36:07 -0300 2010:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > A customer was hitting some misbehavior in one of their internal tests and
> > I tracked it down to plancache not behaving properly with
> > subtransactions: in particular, a plan is not being marked "dead" when
> > the subtransaction on which it is planned rolls back.
> 
> I don't believe that it's plancache's fault; the real problem is that
> plpgsql is keeping "simple expression" execution trees around longer
> than it should.  Your patch masks the problem by forcing those trees to
> be rebuilt, but it's the execution trees not the plan trees that contain
> stale data.

Ahh, this probably explains why I wasn't been able to reproduce the
problem without involving subxacts, or prepared plans, that seemed to
follow mostly the same paths around plancache cleanup.

It's also the likely cause that this hasn't ben reported earlier.

> I'm not immediately sure why plpgsql_subxact_cb is failing to clean up
> correctly in this example, but that seems to be where to look.

Will take a look ... if the girls let me ...

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Tom Lane's message of jue oct 21 19:36:07 -0300 2010:

> I'm not immediately sure why plpgsql_subxact_cb is failing to clean up
> correctly in this example, but that seems to be where to look.

I think the reason is that one econtext is pushed for function
execution, and another one for blocks that contain exceptions.  The
example function does not contain exceptions -- the savepoints are
handled by the external SQL code.

I'll have a closer look tomorrow.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Tom Lane's message of jue oct 21 19:36:07 -0300 2010:
>> I don't believe that it's plancache's fault; the real problem is that
>> plpgsql is keeping "simple expression" execution trees around longer
>> than it should.  Your patch masks the problem by forcing those trees to
>> be rebuilt, but it's the execution trees not the plan trees that contain
>> stale data.

> Ahh, this probably explains why I wasn't been able to reproduce the
> problem without involving subxacts, or prepared plans, that seemed to
> follow mostly the same paths around plancache cleanup.

> It's also the likely cause that this hasn't ben reported earlier.

I traced through the details and found that the proximate cause of the
crash is that this bit in fmgr_sql() gets confused:
   /*    * Convert params to appropriate format if starting a fresh execution. (If    * continuing execution, we can
re-useprior params.)    */   if (es && es->status == F_EXEC_START)       postquel_sub_params(fcache, fcinfo);
 

After the error in the first subtransaction, the execution state tree
for the "public.dummy(p_name_table)" expression has a fn_extra link
that is pointing to a SQLFunctionCache that's in F_EXEC_RUN state.
So when the second call of broken() tries to re-use the state tree,
fmgr_sql() thinks it's continuing the execution of a set-returning
function, and doesn't bother to re-initialize its ParamListInfo
struct.  So it merrily tries to execute using a text datum that's
pointing at long-since-pfree'd storage for the original
'nonexistant.stuffs' argument string.

I had always felt a tad uncomfortable with the way that plpgsql re-uses
execution state trees for simple expressions; it seemed to me that it
was entirely unsafe in recursive usage.  But I'd never been able to
prove that it was broken.  Now that I've seen this example, I know how
to break it: recurse indirectly through a SQL function.  For instance,
this will dump core immediately:

create or replace function recurse(float8) returns float8 as
$$
begin raise notice 'recurse(%)', $1; if ($1 < 10) then   return sql_recurse($1 + 1); else   return $1; end if;
end
$$ language plpgsql;

-- "limit" is to prevent this from being inlined
create or replace function sql_recurse(float8) returns float8 as
$$ select recurse($1) limit 1; $$ language sql;

select recurse(0);

Notice the lack of any subtransaction or error condition.  The reason
this fails is *not* plancache misfeasance or failure to clean up after
error.  Rather, it's that the inner execution of recurse() is trying to
re-use an execution state tree that is pointing at an already-active
execution of sql_recurse().  In general, what plpgsql is doing is
entirely unsafe whenever a called function tries to keep changeable
execution state in storage pointed to by fn_extra.  We've managed to
miss the problem because plpgsql doesn't try to use this technique on
functions returning set (see exec_simple_check_node), and the vast
majority of non-SRF functions that use fn_extra at all use it to cache
catalog lookup results, which don't change from call to call.  But
there's no convention that says a function can't keep execution status
data in fn_extra --- in fact, there isn't anyplace else for it to keep
such data.

Right at the moment I'm not seeing any way that the present
exec_eval_simple_expr approach can be fixed to work safely in the
presence of recursion.  What I think we might have to do is give up
on the idea of caching execution state trees across calls, instead
using them just for the duration of a single plpgsql function call.
I'm not sure what sort of runtime penalty might ensue.  The whole design
predates the plancache, and I think it was mostly intended to prevent
having to re-parse-and-plan simple expressions every time.  So a lot of
that overhead has gone away anyway given the plancache, and maybe we
shouldn't sweat too much about paying what remains.  (But on the third
hand, what are we gonna do for back-patching to versions without the
plancache?)

On 22.10.2010 06:10, Tom Lane wrote:
> Right at the moment I'm not seeing any way that the present
> exec_eval_simple_expr approach can be fixed to work safely in the
> presence of recursion.  What I think we might have to do is give up
> on the idea of caching execution state trees across calls, instead
> using them just for the duration of a single plpgsql function call.
> I'm not sure what sort of runtime penalty might ensue.  The whole design
> predates the plancache, and I think it was mostly intended to prevent
> having to re-parse-and-plan simple expressions every time.  So a lot of
> that overhead has gone away anyway given the plancache, and maybe we
> shouldn't sweat too much about paying what remains.

We should test and measure that.

>  (But on the third
> hand, what are we gonna do for back-patching to versions without the
> plancache?)

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.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> On 22.10.2010 06:10, Tom Lane wrote:
>> (But on the third
>> hand, what are we gonna do for back-patching to versions without the
>> plancache?)

> 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'll look into this issue soon,
though probably not today (Red Hat work calls :-().
        regards, tom lane

I wrote:
> Heikki Linnakangas <heikki.linnakangas@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 some time poking at this today, and developed the attached
patch, which gets rid of all the weird assumptions associated with
"simple expressions" in plpgsql, in favor of just doing another
ExecInitExpr per expression in each call of the function.  While this is
a whole lot cleaner than what we have, I'm afraid that it's unacceptably
slow.  I'm seeing an overall slowdown of 2X to 3X on function execution
with examples like:

create or replace function speedtest10(x float8) returns float8 as $$
declare
  z float8 := x;
begin
  z := z * 2 + 1;
  z := z * 2 + 1;
  z := z * 2 + 1;
  z := z * 2 + 1;
  z := z * 2 + 1;
  z := z * 2 + 1;
  z := z * 2 + 1;
  z := z * 2 + 1;
  z := z * 2 + 1;
  z := z * 2 + 1;
  return z;
end
$$
language plpgsql immutable;

Now, this is about the worst case for the patch.  This function's
runtime depends almost entirely on the speed of simple expressions,
and because there's no internal looping, we only get to use the result
of each ExecInitExpr once per function call.  So probably "typical" use
cases wouldn't be quite so bad; but still it seems like we can't go this
route.  We need to be able to use the ExecInitExpr results across
successive calls one way or another.

I'll look into creating an in-use flag next.

            regards, tom lane

diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
index a28c6707e4670be17ed1c947d383f1d11b9c90c5..0963efe1b5b5e7e974a682d2a7f71d6427180e9b 100644
*** a/src/pl/plpgsql/src/gram.y
--- b/src/pl/plpgsql/src/gram.y
*************** decl_statement    : decl_varname decl_const
*** 490,495 ****
--- 490,496 ----
                          curname_def = palloc0(sizeof(PLpgSQL_expr));

                          curname_def->dtype = PLPGSQL_DTYPE_EXPR;
+                         curname_def->expr_num = plpgsql_nExprs++;
                          strcpy(buf, "SELECT ");
                          cp1 = new->refname;
                          cp2 = buf + strlen(buf);
*************** read_sql_construct(int until,
*** 2277,2282 ****
--- 2278,2284 ----
      expr->query            = pstrdup(ds.data);
      expr->plan            = NULL;
      expr->paramnos        = NULL;
+     expr->expr_num      = plpgsql_nExprs++;
      expr->ns            = plpgsql_ns_top();
      pfree(ds.data);

*************** make_execsql_stmt(int firsttoken, int lo
*** 2476,2481 ****
--- 2478,2484 ----
      expr->query            = pstrdup(ds.data);
      expr->plan            = NULL;
      expr->paramnos        = NULL;
+     expr->expr_num      = plpgsql_nExprs++;
      expr->ns            = plpgsql_ns_top();
      pfree(ds.data);

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 3ddcf3b5a595e0847627cc10c4084258f44cc352..4feb14d2bec87de5d8a66071ab3a66da3c2a2d76 100644
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
***************
*** 37,42 ****
--- 37,47 ----

  /* ----------
   * Our own local and global variables
+  *
+  * Ideally these would live in some dynamically-allocated structure, but
+  * it's unpleasant to pass such a thing around in a Bison parser.  As long
+  * as plpgsql function compilation isn't re-entrant, it's okay to use
+  * static storage for these.
   * ----------
   */
  PLpgSQL_stmt_block *plpgsql_parse_result;
*************** int            plpgsql_nDatums;
*** 46,51 ****
--- 51,58 ----
  PLpgSQL_datum **plpgsql_Datums;
  static int    datums_last = 0;

+ int            plpgsql_nExprs;
+
  char       *plpgsql_error_funcname;
  bool        plpgsql_DumpExecTree = false;
  bool        plpgsql_check_syntax = false;
*************** do_compile(FunctionCallInfo fcinfo,
*** 367,372 ****
--- 374,381 ----
                                       sizeof(PLpgSQL_datum *) * datums_alloc);
      datums_last = 0;

+     plpgsql_nExprs = 0;
+
      switch (is_trigger)
      {
          case false:
*************** do_compile(FunctionCallInfo fcinfo,
*** 693,698 ****
--- 702,708 ----
      function->datums = palloc(sizeof(PLpgSQL_datum *) * plpgsql_nDatums);
      for (i = 0; i < plpgsql_nDatums; i++)
          function->datums[i] = plpgsql_Datums[i];
+     function->nexprs = plpgsql_nExprs;

      /* Debug dump for completed functions */
      if (plpgsql_DumpExecTree)
*************** plpgsql_compile_inline(char *proc_source
*** 788,793 ****
--- 798,804 ----
      plpgsql_nDatums = 0;
      plpgsql_Datums = palloc(sizeof(PLpgSQL_datum *) * datums_alloc);
      datums_last = 0;
+     plpgsql_nExprs = 0;

      /* Set up as though in a function returning VOID */
      function->fn_rettype = VOIDOID;
*************** plpgsql_compile_inline(char *proc_source
*** 838,843 ****
--- 849,855 ----
      function->datums = palloc(sizeof(PLpgSQL_datum *) * plpgsql_nDatums);
      for (i = 0; i < plpgsql_nDatums; i++)
          function->datums[i] = plpgsql_Datums[i];
+     function->nexprs = plpgsql_nExprs;

      /*
       * Pop the error context stack
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 9929e04e57bbc7d08938765b7f506c05c07b772b..613b96c0cc43b60ce770ea239184f686287a996c 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** typedef struct
*** 48,80 ****
      bool       *freevals;        /* which arguments are pfree-able */
  } PreparedParamsData;

- /*
-  * 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).    Furthermore, any exception
-  * block within a function has to have its own eval_econtext separate from
-  * the containing function's, so that we can clean up ExprContext callbacks
-  * properly at subtransaction exit.  We maintain a stack that tracks the
-  * individual econtexts so that we can clean up correctly at subxact exit.
-  *
-  * 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 SimpleEcontextStackEntry
- {
-     ExprContext *stack_econtext;    /* a stacked econtext */
-     SubTransactionId xact_subxid;        /* ID for current subxact */
-     struct SimpleEcontextStackEntry *next;        /* next stack entry up */
- } SimpleEcontextStackEntry;
-
- static EState *simple_eval_estate = NULL;
- static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
-
  /************************************************************
   * Local function forward declarations
   ************************************************************/
--- 48,53 ----
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1013,1019 ****
--- 986,994 ----
           */
          MemoryContext oldcontext = CurrentMemoryContext;
          ResourceOwner oldowner = CurrentResourceOwner;
+         EState *old_eval_estate = estate->eval_estate;
          ExprContext *old_eval_econtext = estate->eval_econtext;
+         ExprState **old_eval_exprtrees = estate->eval_exprtrees;
          ErrorData *save_cur_error = estate->cur_error;

          estate->err_text = gettext_noop("during statement block entry");
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1026,1034 ****
          {
              /*
               * 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);

--- 1001,1012 ----
          {
              /*
               * We need to run the block's statements with a new eval_econtext
!              * that is private to the current subtransaction.  If we try to
!              * share the outer econtext we won't be able to call ExprContext
!              * shutdown callbacks for just the exprs used inside the
!              * subtransaction.  Also, we need to be able to release the
!              * exprtrees on subtransaction failure, since they might contain
!              * stale execution state in that case.
               */
              plpgsql_create_econtext(estate);

*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1058,1075 ****
                                             resTypByVal, resTypLen);
              }

              /* Commit the inner transaction, return to outer xact context */
              ReleaseCurrentSubTransaction();
              MemoryContextSwitchTo(oldcontext);
              CurrentResourceOwner = oldowner;

              /*
-              * Revert to outer eval_econtext.  (The inner one was
-              * automatically cleaned up during subxact exit.)
-              */
-             estate->eval_econtext = old_eval_econtext;
-
-             /*
               * AtEOSubXact_SPI() should not have popped any SPI context, but
               * just in case it did, make sure we remain connected.
               */
--- 1036,1053 ----
                                             resTypByVal, resTypLen);
              }

+             /* Clean up the inner eval_econtext, and restore the outer */
+             plpgsql_destroy_econtext(estate);
+             estate->eval_estate = old_eval_estate;
+             estate->eval_econtext = old_eval_econtext;
+             estate->eval_exprtrees = old_eval_exprtrees;
+
              /* Commit the inner transaction, return to outer xact context */
              ReleaseCurrentSubTransaction();
              MemoryContextSwitchTo(oldcontext);
              CurrentResourceOwner = oldowner;

              /*
               * AtEOSubXact_SPI() should not have popped any SPI context, but
               * just in case it did, make sure we remain connected.
               */
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1092,1099 ****
--- 1070,1088 ----
              MemoryContextSwitchTo(oldcontext);
              CurrentResourceOwner = oldowner;

+             /*
+              * We don't want to run cleanup hooks for the inner eval_econtext,
+              * but we do want to free the memory it used.  Beware of
+              * possibility that we failed during sub-commit after the inner
+              * econtext was already cleaned up.
+              */
+             if (estate->eval_estate != old_eval_estate)
+                 MemoryContextDelete(estate->eval_estate->es_query_cxt);
+
              /* Revert to outer eval_econtext */
+             estate->eval_estate = old_eval_estate;
              estate->eval_econtext = old_eval_econtext;
+             estate->eval_exprtrees = old_eval_exprtrees;

              /*
               * If AtEOSubXact_SPI() popped any SPI context of the subxact, it
*************** plpgsql_estate_setup(PLpgSQL_execstate *
*** 2670,2679 ****
      estate->datums = palloc(sizeof(PLpgSQL_datum *) * estate->ndatums);
      /* caller is expected to fill the datums array */

      estate->eval_tuptable = NULL;
      estate->eval_processed = 0;
      estate->eval_lastoid = InvalidOid;
-     estate->eval_econtext = NULL;
      estate->cur_expr = NULL;

      estate->err_stmt = NULL;
--- 2659,2671 ----
      estate->datums = palloc(sizeof(PLpgSQL_datum *) * estate->ndatums);
      /* caller is expected to fill the datums array */

+     estate->eval_estate = NULL;
+     estate->eval_econtext = NULL;
+     estate->eval_exprtrees = NULL;
+
      estate->eval_tuptable = NULL;
      estate->eval_processed = 0;
      estate->eval_lastoid = InvalidOid;
      estate->cur_expr = NULL;

      estate->err_stmt = NULL;
*************** plpgsql_estate_setup(PLpgSQL_execstate *
*** 2682,2688 ****
      estate->plugin_info = NULL;

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

--- 2674,2680 ----
      estate->plugin_info = NULL;

      /*
!      * Initialize state for evaluation of simple expressions.
       */
      plpgsql_create_econtext(estate);

*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 4514,4520 ****
                        Oid *rettype)
  {
      ExprContext *econtext = estate->eval_econtext;
!     LocalTransactionId curlxid = MyProc->lxid;
      CachedPlanSource *plansource;
      CachedPlan *cplan;
      ParamListInfo paramLI;
--- 4506,4512 ----
                        Oid *rettype)
  {
      ExprContext *econtext = estate->eval_econtext;
!     ExprState **exprtree_p;
      CachedPlanSource *plansource;
      CachedPlan *cplan;
      ParamListInfo paramLI;
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 4546,4551 ****
--- 4538,4545 ----
              ReleaseCachedPlan(cplan, true);
              return false;
          }
+         /* Force rebuild of execution tree below */
+         estate->eval_exprtrees[expr->expr_num] = NULL;
      }

      /*
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 4555,4568 ****

      /*
       * Prepare the expression for execution, if it's not been done already in
!      * the current transaction.  (This will be forced to happen if we called
!      * exec_simple_check_plan above.)
       */
!     if (expr->expr_simple_lxid != curlxid)
      {
!         expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr,
!                                                   simple_eval_estate);
!         expr->expr_simple_lxid = curlxid;
      }

      /*
--- 4549,4571 ----

      /*
       * Prepare the expression for execution, if it's not been done already in
!      * the current function.
       */
!     exprtree_p = &estate->eval_exprtrees[expr->expr_num];
!     if (*exprtree_p == NULL)
      {
! #if 0
!         *exprtree_p = ExecPrepareExpr(expr->expr_simple_expr,
!                                       estate->eval_estate);
! #else
!         MemoryContext oldcontext;
!
!         oldcontext = MemoryContextSwitchTo(estate->eval_estate->es_query_cxt);
!
!         *exprtree_p = ExecInitExpr(expr->expr_simple_expr, NULL);
!
!         MemoryContextSwitchTo(oldcontext);
! #endif
      }

      /*
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 4599,4605 ****
      /*
       * Finally we can call the executor to evaluate the expression
       */
!     *result = ExecEvalExpr(expr->expr_simple_state,
                             econtext,
                             isNull,
                             NULL);
--- 4602,4608 ----
      /*
       * Finally we can call the executor to evaluate the expression
       */
!     *result = ExecEvalExpr(*exprtree_p,
                             econtext,
                             isNull,
                             NULL);
*************** exec_simple_check_plan(PLpgSQL_expr *exp
*** 5336,5348 ****
          return;

      /*
!      * Yes - this is a simple expression.  Mark it as such, and initialize
!      * state to "not valid in current transaction".
       */
      expr->expr_simple_expr = tle->expr;
-     expr->expr_simple_state = NULL;
-     expr->expr_simple_lxid = InvalidLocalTransactionId;
-     /* Also stash away the expression result type */
      expr->expr_simple_type = exprType((Node *) tle->expr);
  }

--- 5339,5348 ----
          return;

      /*
!      * Yes - this is a simple expression.  Mark it as such, and also
!      * remember the expression's result type.
       */
      expr->expr_simple_expr = tle->expr;
      expr->expr_simple_type = exprType((Node *) tle->expr);
  }

*************** exec_set_found(PLpgSQL_execstate *estate
*** 5363,5491 ****

  /*
   * plpgsql_create_econtext --- create an eval_econtext for the current function
-  *
-  * We may need to create a new simple_eval_estate too, if there's not one
-  * already for the current transaction.  The EState will be cleaned up at
-  * transaction end.
   */
  static void
  plpgsql_create_econtext(PLpgSQL_execstate *estate)
  {
-     SimpleEcontextStackEntry *entry;
-
      /*
!      * 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 a child econtext for the current function.
       */
!     estate->eval_econtext = CreateExprContext(simple_eval_estate);

      /*
!      * Make a stack entry so we can clean up the econtext at subxact end.
!      * Stack entries are kept in TopTransactionContext for simplicity.
       */
!     entry = (SimpleEcontextStackEntry *)
!         MemoryContextAlloc(TopTransactionContext,
!                            sizeof(SimpleEcontextStackEntry));
!
!     entry->stack_econtext = estate->eval_econtext;
!     entry->xact_subxid = GetCurrentSubTransactionId();
!
!     entry->next = simple_econtext_stack;
!     simple_econtext_stack = entry;
  }

  /*
   * plpgsql_destroy_econtext --- destroy function's econtext
-  *
-  * We check that it matches the top stack entry, and destroy the stack
-  * entry along with the context.
   */
  static void
  plpgsql_destroy_econtext(PLpgSQL_execstate *estate)
  {
!     SimpleEcontextStackEntry *next;
!
!     Assert(simple_econtext_stack != NULL);
!     Assert(simple_econtext_stack->stack_econtext == estate->eval_econtext);
!
!     next = simple_econtext_stack->next;
!     pfree(simple_econtext_stack);
!     simple_econtext_stack = next;
!
!     FreeExprContext(estate->eval_econtext, true);
      estate->eval_econtext = NULL;
! }
!
! /*
!  * 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)
! {
!     /*
!      * 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_ABORT)
!     {
!         /* Shouldn't be any econtext stack entries left at commit */
!         Assert(simple_econtext_stack == NULL);
!
!         if (simple_eval_estate)
!             FreeExecutorState(simple_eval_estate);
!         simple_eval_estate = NULL;
!     }
!     else
!     {
!         simple_econtext_stack = NULL;
!         simple_eval_estate = NULL;
!     }
! }
!
! /*
!  * plpgsql_subxact_cb --- post-subtransaction-commit-or-abort cleanup
!  *
!  * Make sure any simple-expression econtexts created in the current
!  * subtransaction get cleaned up.  We have to do this explicitly because
!  * no other code knows which child econtexts of simple_eval_estate belong
!  * to which level of subxact.
!  */
! void
! plpgsql_subxact_cb(SubXactEvent event, SubTransactionId mySubid,
!                    SubTransactionId parentSubid, void *arg)
! {
!     if (event == SUBXACT_EVENT_START_SUB)
!         return;
!
!     while (simple_econtext_stack != NULL &&
!            simple_econtext_stack->xact_subxid == mySubid)
!     {
!         SimpleEcontextStackEntry *next;
!
!         FreeExprContext(simple_econtext_stack->stack_econtext,
!                         (event == SUBXACT_EVENT_COMMIT_SUB));
!         next = simple_econtext_stack->next;
!         pfree(simple_econtext_stack);
!         simple_econtext_stack = next;
!     }
  }

  /*
--- 5363,5402 ----

  /*
   * plpgsql_create_econtext --- create an eval_econtext for the current function
   */
  static void
  plpgsql_create_econtext(PLpgSQL_execstate *estate)
  {
      /*
!      * Create an EState and an ExprContext for evaluation of simple
!      * expressions.  These are children of the function's SPI-procedure memory
!      * context, so they will live only until function exit.
       */
!     estate->eval_estate = CreateExecutorState();
!     estate->eval_econtext = CreateExprContext(estate->eval_estate);

      /*
!      * Create space for per-PLpgSQL_expr expression evaluation state trees.
!      * Only "simple" expressions will actually use their slots, but it's
!      * easiest to allocate a slot for each one anyway.  All storage for the
!      * state trees is in the eval_estate's "per-query" context.
       */
!     estate->eval_exprtrees = (ExprState **)
!         MemoryContextAllocZero(estate->eval_estate->es_query_cxt,
!                                estate->func->nexprs * sizeof(ExprState *));
  }

  /*
   * plpgsql_destroy_econtext --- destroy function's econtext
   */
  static void
  plpgsql_destroy_econtext(PLpgSQL_execstate *estate)
  {
!     /* This frees the exprcontext and the eval state trees as well: */
!     FreeExecutorState(estate->eval_estate);
!     estate->eval_estate = NULL;
      estate->eval_econtext = NULL;
!     estate->eval_exprtrees = NULL;
  }

  /*
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index ee49d71d23f9d53ab686f29b89de8e85379b8368..f6485653bb296c728a2bac382e97fa010c8ab44e 100644
*** a/src/pl/plpgsql/src/pl_handler.c
--- b/src/pl/plpgsql/src/pl_handler.c
*************** _PG_init(void)
*** 68,75 ****
      EmitWarningsOnPlaceholders("plpgsql");

      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");
--- 68,73 ----
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 5b174c49b8bcac4658ffd54fc4493e43221533e7..bf67ca45f98f640d4475e94343b76083c13c1cfe 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** typedef struct PLpgSQL_expr
*** 200,205 ****
--- 200,206 ----
      char       *query;
      SPIPlanPtr    plan;
      Bitmapset  *paramnos;        /* all dnos referenced by this query */
+     int            expr_num;        /* index in func's eval_exprtrees[] */

      /* function containing this expr (not set until we first parse query) */
      struct PLpgSQL_function *func;
*************** typedef struct PLpgSQL_expr
*** 211,224 ****
      Expr       *expr_simple_expr;        /* NULL means not a simple expr */
      int            expr_simple_generation; /* plancache generation we checked */
      Oid            expr_simple_type;        /* result type Oid, if simple */
-
-     /*
-      * 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;


--- 212,217 ----
*************** typedef struct PLpgSQL_function
*** 675,680 ****
--- 668,675 ----
      PLpgSQL_datum **datums;
      PLpgSQL_stmt_block *action;

+     int            nexprs;            /* number of PLpgSQL_expr's in function */
+
      /* these fields change when the function is used */
      struct PLpgSQL_execstate *cur_estate;
      unsigned long use_count;
*************** typedef struct PLpgSQL_execstate
*** 709,719 ****
      int            ndatums;
      PLpgSQL_datum **datums;

      /* temporary state for results from evaluation of query or expr */
      SPITupleTable *eval_tuptable;
      uint32        eval_processed;
      Oid            eval_lastoid;
-     ExprContext *eval_econtext; /* for executing simple expressions */
      PLpgSQL_expr *cur_expr;        /* current query/expr being evaluated */

      /* status information for error context reporting */
--- 704,718 ----
      int            ndatums;
      PLpgSQL_datum **datums;

+     /* state for evaluation of "simple" expressions */
+     EState       *eval_estate;    /* EState for simple expressions */
+     ExprContext *eval_econtext; /* ExprContext for simple expressions */
+     ExprState **eval_exprtrees;    /* array containing func->nexprs elements */
+
      /* temporary state for results from evaluation of query or expr */
      SPITupleTable *eval_tuptable;
      uint32        eval_processed;
      Oid            eval_lastoid;
      PLpgSQL_expr *cur_expr;        /* current query/expr being evaluated */

      /* status information for error context reporting */
*************** extern PLpgSQL_stmt_block *plpgsql_parse
*** 815,820 ****
--- 814,821 ----
  extern int    plpgsql_nDatums;
  extern PLpgSQL_datum **plpgsql_Datums;

+ extern int    plpgsql_nExprs;
+
  extern char *plpgsql_error_funcname;

  extern PLpgSQL_function *plpgsql_curr_compile;
*************** extern Datum plpgsql_exec_function(PLpgS
*** 875,883 ****
                        FunctionCallInfo fcinfo);
  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);
  extern Oid exec_get_datum_type(PLpgSQL_execstate *estate,
                      PLpgSQL_datum *datum);
  extern Oid exec_get_rec_fieldtype(PLpgSQL_rec *rec, const char *fieldname,
--- 876,881 ----

I wrote:
>> Heikki Linnakangas <heikki.linnakangas@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;

Excerpts from Tom Lane's message of mié oct 27 18:18:06 -0300 2010:
> I wrote:
> >> Heikki Linnakangas <heikki.linnakangas@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.

I tried the original test cases that were handed to me (quite different
from what I submitted here) and they are fixed also.  Thanks.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Tom Lane's message of mié oct 27 18:18:06 -0300 2010:
>> 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.

> I tried the original test cases that were handed to me (quite different
> from what I submitted here) and they are fixed also.  Thanks.

It'd be interesting to know if there's any noticeable slowdown on
affected real-world cases.  (Of course, if they invariably crashed
before, there might not be a way to measure their previous speed...)
        regards, tom lane

Excerpts from Tom Lane's message of vie oct 29 12:54:52 -0300 2010:
> Alvaro Herrera <alvherre@commandprompt.com> writes:

> > I tried the original test cases that were handed to me (quite different
> > from what I submitted here) and they are fixed also.  Thanks.
> 
> It'd be interesting to know if there's any noticeable slowdown on
> affected real-world cases.  (Of course, if they invariably crashed
> before, there might not be a way to measure their previous speed...)

Yeah, the cases that were reported failed with one of

ERROR: invalid memory alloc request size 18446744073482534916
ERROR: could not open relation with OID 0
ERROR: buffer 228 is not owned by resource owner Portal

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Oct 29, 2010, at 10:54 AM, Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
>> Excerpts from Tom Lane's message of mié oct 27 18:18:06 -0300 2010:
>>> 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.
>
>> I tried the original test cases that were handed to me (quite different
>> from what I submitted here) and they are fixed also.  Thanks.
>
> It'd be interesting to know if there's any noticeable slowdown on
> affected real-world cases.  (Of course, if they invariably crashed
> before, there might not be a way to measure their previous speed...)

I should be able to get Alvaro something he can use to test the performance. Our patch framework uses a recursive
functionto follow patch dependencies (of course that can go away in 8.4 thanks to WITH). I know we've got some other
recursivecalls but I don't think any are critical (it'd be nice if there was a way to find out if a function was
recursive,I guess theoretically that could be discovered during compilation but I don't know how hairy it would be). 

One question: What happens if you have multiple paths to the same function within another function? For example, we
havean assert function that's used all over the place; it will definitely be called from multiple places in a call
stack.

FWIW, I definitely run into cases where recursion makes for cleaner code than looping, so it'd be great to avoid making
itslower than it needs to be. But I've always assumed that recursion is slower than looping so I avoid it for anything
Iknow could be performance sensitive. 

(looking at original case)... the original bug wasn't actually recursive. It's not clear to me how it actually got into
thiscase. The original error report is: 

psql:sql/code.lookup_table_dynamic.sql:23: ERROR:  buffer 2682 is not owned by resource owner Portal
CONTEXT:  SQL function "table_schema_and_name" statement 1
SQL function "table_full_name" statement 1
PL/pgSQL function "getsert" line 9 during statement block local variable initialization
server closed the connection unexpectedly This probably means the server terminated abnormally before or while
processingthe request. 

Line 23 is:
   SELECT code.getsert( 'test.stuffs', 'stuff' );

The functions are below. The duplicity of full_name_table and table_full_name is because the function was originally
calledfull_name_table, but I decided to rename it after creating other table functions. In any case, I don't see any
obviousrecursion or re-entry, unless perhaps tools.table_schema_and_name ends up getting called twice by
tools.table_full_name?

-[ RECORD 1
]-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------
Schema              | code
Name                | getsert
Result data type    | void
Argument data types | p_table_name text, p_lookup text
Volatility          | volatile
Owner               | cnuadmin
Language            | plpgsql
Source code         |                    : DECLARE                   :     v_object_class text := 'getsert';
      :     v_function_name text := p_table_name || '__' || v_object_class;                   :                    :
v_table_full text := tools.full_name_table( p_table_name );                   :     v_schema text;                   :
  v_table text;                   :                    : BEGIN                   :     SELECT INTO v_schema, v_table *
FROMtools.split_schema( v_table_full );                   :                    :     PERFORM
code_internal.create_object(v_function_name, 'FUNCTION', v_object_class, array[ ['schema', v_schema], ['table',
v_table],['lookup', p_lookup] ] );                   : END;                   :  
Description         | Creates a function that will lookup an ID based on a text lookup value (p_lookup). If no record
exists,one will be created.                   :                    : Parameters:                   :     p_table_name
Nameof the table to lookup the value in                   :     p_lookup Name of the field to use for the lookup value
                :                    : Results:                   : Creates function %p_table_name%__getsert(
%p_lookup%with a type matching the p_lookup field in p_table_name ). The function returns an ID as an int.
    : Revokes all on the function from public and grants execute to cnuapp_role.                   :  

test_us@workbook.local=# \df+ tools.full_name_table
List of functions
-[ RECORD 1 ]-------+-----------------------------------
Schema              | tools
Name                | full_name_table
Result data type    | text
Argument data types | p_table_name text
Volatility          | volatile
Owner               | cnuadmin
Language            | sql
Source code         | SELECT tools.table_full_name( $1 )
Description         |

test_us@workbook.local=# \df+ tools.table_full_name
List of functions
-[ RECORD 1 ]-------+-------------------------------------------------------------------------------
Schema              | tools
Name                | table_full_name
Result data type    | text
Argument data types | p_table_name text
Volatility          | volatile
Owner               | su
Language            | sql
Source code         | SELECT schema_name || '.' || table_name FROM tools.table_schema_and_name( $1 )
Description         |

test_us@workbook.local=# \df+ tools.table_schema_and_name
List of functions
-[ RECORD 1 ]-------+------------------------------------------------------------------------------
Schema              | tools
Name                | table_schema_and_name
Result data type    | record
Argument data types | p_table_name text, OUT schema_name text, OUT table_name text
Volatility          | volatile
Owner               | su
Language            | sql
Source code         |                    : SELECT quote_ident(nspname),  quote_ident(relname)                   :
FROMpg_class c                   :     JOIN pg_namespace n ON n.oid = c.relnamespace                   :   WHERE c.oid
=$1::regclass                   :     AND tools.assert( relkind = 'r', 'Relation ' || $1 || ' is not a table' )
         :  
Description         |


--
Jim C. Nasby, Database Architect                   jim@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net

Jim Nasby <Jim@Nasby.net> writes:
> (looking at original case)... the original bug wasn't actually
> recursive.

No, there are two different cases being dealt with here.  If the first
call of an expression results in an error, and then we come back and try
to re-use the expression state tree, we can have trouble because the
state tree contains partially-updated internal state for the called
function.  This doesn't require any recursion but it leads to pretty
much the same problems as the recursive case.
        regards, tom lane