From a4d3a504e730c47ccee5082ee703082e42c8b5ce Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 1 Mar 2013 21:33:34 -0500 Subject: [PATCH] Eliminate memory leaks in plperl's spi_prepare() function. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Careless use of TopMemoryContext for I/O function data meant that repeated use of spi_prepare and spi_freeplan would leak memory at the session level, as per report from Christian Schröder. In addition, spi_prepare leaked a lot of transient data within the current plperl function's SPI Proc context, which would be a problem for repeated use of spi_prepare within a single plperl function call; and it wasn't terribly careful about releasing permanent allocations in event of an error, either. In passing, clean up some copy-and-pasteos in query-lookup error messages. Alex Hunsaker and Tom Lane --- src/pl/plperl/plperl.c | 116 +++++++++++++++++++++++++---------------- 1 file changed, 72 insertions(+), 44 deletions(-) diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c index c1e81b4d02..1463618e42 100644 --- a/src/pl/plperl/plperl.c +++ b/src/pl/plperl/plperl.c @@ -184,6 +184,7 @@ typedef struct plperl_call_data typedef struct plperl_query_desc { char qname[24]; + MemoryContext plan_cxt; /* context holding this struct */ SPIPlanPtr plan; int nargs; Oid *argtypes; @@ -3209,33 +3210,57 @@ plperl_spi_cursor_close(char *cursor) SV * plperl_spi_prepare(char *query, int argc, SV **argv) { - plperl_query_desc *qdesc; - plperl_query_entry *hash_entry; - bool found; - SPIPlanPtr plan; - int i; - + volatile SPIPlanPtr plan = NULL; + volatile MemoryContext plan_cxt = NULL; + plperl_query_desc *volatile qdesc = NULL; + plperl_query_entry *volatile hash_entry = NULL; MemoryContext oldcontext = CurrentMemoryContext; ResourceOwner oldowner = CurrentResourceOwner; + MemoryContext work_cxt; + bool found; + int i; check_spi_usage_allowed(); BeginInternalSubTransaction(NULL); MemoryContextSwitchTo(oldcontext); - /************************************************************ - * Allocate the new querydesc structure - ************************************************************/ - qdesc = (plperl_query_desc *) malloc(sizeof(plperl_query_desc)); - MemSet(qdesc, 0, sizeof(plperl_query_desc)); - snprintf(qdesc->qname, sizeof(qdesc->qname), "%p", qdesc); - qdesc->nargs = argc; - qdesc->argtypes = (Oid *) malloc(argc * sizeof(Oid)); - qdesc->arginfuncs = (FmgrInfo *) malloc(argc * sizeof(FmgrInfo)); - qdesc->argtypioparams = (Oid *) malloc(argc * sizeof(Oid)); - PG_TRY(); { + CHECK_FOR_INTERRUPTS(); + + /************************************************************ + * Allocate the new querydesc structure + * + * The qdesc struct, as well as all its subsidiary data, lives in its + * plan_cxt. But note that the SPIPlan does not. + ************************************************************/ + plan_cxt = AllocSetContextCreate(TopMemoryContext, + "PL/Perl spi_prepare query", + ALLOCSET_SMALL_MINSIZE, + ALLOCSET_SMALL_INITSIZE, + ALLOCSET_SMALL_MAXSIZE); + MemoryContextSwitchTo(plan_cxt); + qdesc = (plperl_query_desc *) palloc0(sizeof(plperl_query_desc)); + snprintf(qdesc->qname, sizeof(qdesc->qname), "%p", qdesc); + qdesc->plan_cxt = plan_cxt; + qdesc->nargs = argc; + qdesc->argtypes = (Oid *) palloc(argc * sizeof(Oid)); + qdesc->arginfuncs = (FmgrInfo *) palloc(argc * sizeof(FmgrInfo)); + qdesc->argtypioparams = (Oid *) palloc(argc * sizeof(Oid)); + MemoryContextSwitchTo(oldcontext); + + /************************************************************ + * Do the following work in a short-lived context so that we don't + * leak a lot of memory in the PL/Perl function's SPI Proc context. + ************************************************************/ + work_cxt = AllocSetContextCreate(CurrentMemoryContext, + "PL/Perl spi_prepare workspace", + ALLOCSET_DEFAULT_MINSIZE, + ALLOCSET_DEFAULT_INITSIZE, + ALLOCSET_DEFAULT_MAXSIZE); + MemoryContextSwitchTo(work_cxt); + /************************************************************ * Resolve argument type names and then look them up by oid * in the system cache, and remember the required information @@ -3256,7 +3281,7 @@ plperl_spi_prepare(char *query, int argc, SV **argv) getTypeInputInfo(typId, &typInput, &typIOParam); qdesc->argtypes[i] = typId; - perm_fmgr_info(typInput, &(qdesc->arginfuncs[i])); + fmgr_info_cxt(typInput, &(qdesc->arginfuncs[i]), plan_cxt); qdesc->argtypioparams[i] = typIOParam; } @@ -3280,6 +3305,17 @@ plperl_spi_prepare(char *query, int argc, SV **argv) elog(ERROR, "SPI_keepplan() failed"); qdesc->plan = plan; + /************************************************************ + * Insert a hashtable entry for the plan. + ************************************************************/ + hash_entry = hash_search(plperl_active_interp->query_hash, + qdesc->qname, + HASH_ENTER, &found); + hash_entry->query_data = qdesc; + + /* Get rid of workspace */ + MemoryContextDelete(work_cxt); + /* Commit the inner transaction, return to outer xact context */ ReleaseCurrentSubTransaction(); MemoryContextSwitchTo(oldcontext); @@ -3295,16 +3331,21 @@ plperl_spi_prepare(char *query, int argc, SV **argv) { ErrorData *edata; - free(qdesc->argtypes); - free(qdesc->arginfuncs); - free(qdesc->argtypioparams); - free(qdesc); - /* Save error info */ MemoryContextSwitchTo(oldcontext); edata = CopyErrorData(); FlushErrorState(); + /* Drop anything we managed to allocate */ + if (hash_entry) + hash_search(plperl_active_interp->query_hash, + qdesc->qname, + HASH_REMOVE, NULL); + if (plan_cxt) + MemoryContextDelete(plan_cxt); + if (plan) + SPI_freeplan(plan); + /* Abort the inner transaction */ RollbackAndReleaseCurrentSubTransaction(); MemoryContextSwitchTo(oldcontext); @@ -3326,14 +3367,8 @@ plperl_spi_prepare(char *query, int argc, SV **argv) PG_END_TRY(); /************************************************************ - * Insert a hashtable entry for the plan and return - * the key to the caller. + * Return the query's hash key to the caller. ************************************************************/ - - hash_entry = hash_search(plperl_active_interp->query_hash, qdesc->qname, - HASH_ENTER, &found); - hash_entry->query_data = qdesc; - return cstr2sv(qdesc->qname); } @@ -3368,16 +3403,14 @@ plperl_spi_exec_prepared(char *query, HV *attr, int argc, SV **argv) /************************************************************ * Fetch the saved plan descriptor, see if it's o.k. ************************************************************/ - hash_entry = hash_search(plperl_active_interp->query_hash, query, HASH_FIND, NULL); if (hash_entry == NULL) elog(ERROR, "spi_exec_prepared: Invalid prepared query passed"); qdesc = hash_entry->query_data; - if (qdesc == NULL) - elog(ERROR, "spi_exec_prepared: panic - plperl query_hash value vanished"); + elog(ERROR, "spi_exec_prepared: plperl query_hash value vanished"); if (qdesc->nargs != argc) elog(ERROR, "spi_exec_prepared: expected %d argument(s), %d passed", @@ -3509,12 +3542,11 @@ plperl_spi_query_prepared(char *query, int argc, SV **argv) hash_entry = hash_search(plperl_active_interp->query_hash, query, HASH_FIND, NULL); if (hash_entry == NULL) - elog(ERROR, "spi_exec_prepared: Invalid prepared query passed"); + elog(ERROR, "spi_query_prepared: Invalid prepared query passed"); qdesc = hash_entry->query_data; - if (qdesc == NULL) - elog(ERROR, "spi_query_prepared: panic - plperl query_hash value vanished"); + elog(ERROR, "spi_query_prepared: plperl query_hash value vanished"); if (qdesc->nargs != argc) elog(ERROR, "spi_query_prepared: expected %d argument(s), %d passed", @@ -3619,12 +3651,12 @@ plperl_spi_freeplan(char *query) hash_entry = hash_search(plperl_active_interp->query_hash, query, HASH_FIND, NULL); if (hash_entry == NULL) - elog(ERROR, "spi_exec_prepared: Invalid prepared query passed"); + elog(ERROR, "spi_freeplan: Invalid prepared query passed"); qdesc = hash_entry->query_data; - if (qdesc == NULL) - elog(ERROR, "spi_exec_freeplan: panic - plperl query_hash value vanished"); + elog(ERROR, "spi_freeplan: plperl query_hash value vanished"); + plan = qdesc->plan; /* * free all memory before SPI_freeplan, so if it dies, nothing will be @@ -3633,11 +3665,7 @@ plperl_spi_freeplan(char *query) hash_search(plperl_active_interp->query_hash, query, HASH_REMOVE, NULL); - plan = qdesc->plan; - free(qdesc->argtypes); - free(qdesc->arginfuncs); - free(qdesc->argtypioparams); - free(qdesc); + MemoryContextDelete(qdesc->plan_cxt); SPI_freeplan(plan); } -- 2.40.0