Re: crash in plancache with subtransactions - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: crash in plancache with subtransactions |
Date | |
Msg-id | 2736.1288126126@sss.pgh.pa.us Whole thread Raw |
In response to | Re: crash in plancache with subtransactions (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: crash in plancache with subtransactions
|
List | pgsql-hackers |
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 ----
pgsql-hackers by date: