From 1f653ec31ef0f88af1216c0631605e859b4794a5 Mon Sep 17 00:00:00 2001 From: Bruce Momjian Date: Mon, 14 Oct 2002 04:20:52 +0000 Subject: [PATCH] I have attached two patches as per: 1) pltcl: Add SPI_freetuptable() calls to avoid memory leaks (Me + Neil Conway) Change sprintf()s to snprintf()s (Neil Conway) Remove header files included elsewhere (Neil Conway) 2)plpython: Add SPI_freetuptable() calls to avoid memory leaks Cosemtic change to remove a compiler warning Notes: I have tested pltcl.c for a) the original leak problem reported for the repeated call of spi_exec in a TCL fragment and b) the subsequent report resulting from the use of spi_exec -array in a TCL fragment. The plpython.c patch is exactly the same as that applied to make revision 1.23, the plpython_schema.sql and feature.expected sections of the patch are also the same as last submited, applied and subsequently reversed out. It remains untested by me (other than via make check). However, this should be safe provided PyString_FromString() _copies_ the given string to make a PyObject. Nigel J. Andrews --- src/pl/plpython/feature.expected | 2 +- src/pl/plpython/plpython.c | 13 +++++- src/pl/plpython/plpython_schema.sql | 2 +- src/pl/tcl/pltcl.c | 70 +++++++++++++++++++---------- 4 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/pl/plpython/feature.expected b/src/pl/plpython/feature.expected index 9afd782e7d2..b689c1f6154 100644 --- a/src/pl/plpython/feature.expected +++ b/src/pl/plpython/feature.expected @@ -29,7 +29,7 @@ SELECT global_test_two(); (1 row) SELECT import_fail(); -WARNING: ('import socket failed -- untrusted dynamic module: _socket',) +NOTICE: ('import socket failed -- untrusted dynamic module: _socket',) import_fail -------------------- failed as expected diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c index 86f39cd4d09..005d85e9ce1 100644 --- a/src/pl/plpython/plpython.c +++ b/src/pl/plpython/plpython.c @@ -29,7 +29,7 @@ * MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS. * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/pl/plpython/plpython.c,v 1.24 2002/09/26 05:39:03 momjian Exp $ + * $Header: /cvsroot/pgsql/src/pl/plpython/plpython.c,v 1.25 2002/10/14 04:20:52 momjian Exp $ * ********************************************************************* */ @@ -408,7 +408,9 @@ plpython_call_handler(PG_FUNCTION_ARGS) else PLy_restart_in_progress += 1; if (proc) + { Py_DECREF(proc->me); + } RERAISE_EXC(); } @@ -1841,7 +1843,14 @@ PLy_plan_dealloc(PyObject * arg) * * FIXME -- leaks saved plan on object destruction. can this be * avoided? + * I think so. A function prepares and then execp's a statement. + * When we come to deallocate the 'statement' object we obviously + * no long need the plan. Even if we did, without the object + * we're never going to be able to use it again. + * In the against arguments: SPI_saveplan has stuck this under + * the top context so there must be a reason for doing that. */ + pfree(ob->plan); } if (ob->types) PLy_free(ob->types); @@ -2374,6 +2383,8 @@ PLy_spi_execute_fetch_result(SPITupleTable *tuptable, int rows, int status) PyList_SetItem(result->rows, i, row); } PLy_typeinfo_dealloc(&args); + + SPI_freetuptable(tuptable); } RESTORE_EXC(); } diff --git a/src/pl/plpython/plpython_schema.sql b/src/pl/plpython/plpython_schema.sql index 28ceef55350..2f8766431fa 100644 --- a/src/pl/plpython/plpython_schema.sql +++ b/src/pl/plpython/plpython_schema.sql @@ -20,7 +20,7 @@ CREATE TABLE taxonomy ( CREATE TABLE entry ( accession text not null primary key, - eid serial, + eid serial unique, txid int2 not null references taxonomy(id) ) ; diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c index f404feae9b4..f2bf14ab92b 100644 --- a/src/pl/tcl/pltcl.c +++ b/src/pl/tcl/pltcl.c @@ -31,7 +31,7 @@ * ENHANCEMENTS, OR MODIFICATIONS. * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/pl/tcl/pltcl.c,v 1.64 2002/09/26 05:39:03 momjian Exp $ + * $Header: /cvsroot/pgsql/src/pl/tcl/pltcl.c,v 1.65 2002/10/14 04:20:52 momjian Exp $ * **********************************************************************/ @@ -39,12 +39,8 @@ #include -#include -#include -#include #include #include -#include #include #include "access/heapam.h" @@ -308,6 +304,7 @@ pltcl_init_load_unknown(Tcl_Interp *interp) ************************************************************/ spi_rc = SPI_exec("select 1 from pg_class " "where relname = 'pltcl_modules'", 1); + SPI_freetuptable(SPI_tuptable); if (spi_rc != SPI_OK_SELECT) elog(ERROR, "pltcl_init_load_unknown(): select from pg_class failed"); if (SPI_processed == 0) @@ -334,6 +331,7 @@ pltcl_init_load_unknown(Tcl_Interp *interp) if (SPI_processed == 0) { Tcl_DStringFree(&unknown_src); + SPI_freetuptable(SPI_tuptable); elog(WARNING, "pltcl: Module unknown not found in pltcl_modules"); return; } @@ -359,6 +357,7 @@ pltcl_init_load_unknown(Tcl_Interp *interp) } tcl_rc = Tcl_GlobalEval(interp, Tcl_DStringValue(&unknown_src)); Tcl_DStringFree(&unknown_src); + SPI_freetuptable(SPI_tuptable); } @@ -955,9 +954,11 @@ compile_pltcl_function(Oid fn_oid, bool is_trigger) * Build our internal proc name from the functions Oid ************************************************************/ if (!is_trigger) - sprintf(internal_proname, "__PLTcl_proc_%u", fn_oid); + snprintf(internal_proname, sizeof(internal_proname), + "__PLTcl_proc_%u", fn_oid); else - sprintf(internal_proname, "__PLTcl_proc_%u_trigger", fn_oid); + snprintf(internal_proname, sizeof(internal_proname), + "__PLTcl_proc_%u_trigger", fn_oid); /************************************************************ * Lookup the internal proc name in the hashtable @@ -1127,7 +1128,7 @@ compile_pltcl_function(Oid fn_oid, bool is_trigger) prodesc->arg_is_rel[i] = 1; if (i > 0) strcat(proc_internal_args, " "); - sprintf(buf, "__PLTcl_Tup_%d", i + 1); + snprintf(buf, sizeof(buf), "__PLTcl_Tup_%d", i + 1); strcat(proc_internal_args, buf); ReleaseSysCache(typeTup); continue; @@ -1140,7 +1141,7 @@ compile_pltcl_function(Oid fn_oid, bool is_trigger) if (i > 0) strcat(proc_internal_args, " "); - sprintf(buf, "%d", i + 1); + snprintf(buf, sizeof(buf), "%d", i + 1); strcat(proc_internal_args, buf); ReleaseSysCache(typeTup); @@ -1177,7 +1178,8 @@ compile_pltcl_function(Oid fn_oid, bool is_trigger) { if (!prodesc->arg_is_rel[i]) continue; - sprintf(buf, "array set %d $__PLTcl_Tup_%d\n", i + 1, i + 1); + snprintf(buf, sizeof(buf), "array set %d $__PLTcl_Tup_%d\n", + i + 1, i + 1); Tcl_DStringAppend(&proc_internal_body, buf, -1); } } @@ -1475,6 +1477,7 @@ pltcl_SPI_exec(ClientData cdata, Tcl_Interp *interp, int ntuples; HeapTuple *volatile tuples; volatile TupleDesc tupdesc = NULL; + SPITupleTable *tuptable; sigjmp_buf save_restart; char *usage = "syntax error - 'SPI_exec " @@ -1557,14 +1560,16 @@ pltcl_SPI_exec(ClientData cdata, Tcl_Interp *interp, { case SPI_OK_UTILITY: Tcl_SetResult(interp, "0", TCL_VOLATILE); + SPI_freetuptable(SPI_tuptable); return TCL_OK; case SPI_OK_SELINTO: case SPI_OK_INSERT: case SPI_OK_DELETE: case SPI_OK_UPDATE: - sprintf(buf, "%d", SPI_processed); + snprintf(buf, sizeof(buf), "%d", SPI_processed); Tcl_SetResult(interp, buf, TCL_VOLATILE); + SPI_freetuptable(SPI_tuptable); return TCL_OK; case SPI_OK_SELECT: @@ -1607,7 +1612,7 @@ pltcl_SPI_exec(ClientData cdata, Tcl_Interp *interp, return TCL_ERROR; default: - sprintf(buf, "%d", spi_rc); + snprintf(buf, sizeof(buf), "%d", spi_rc); Tcl_AppendResult(interp, "pltcl: SPI_exec() failed - ", "unknown RC ", buf, NULL); return TCL_ERROR; @@ -1645,12 +1650,15 @@ pltcl_SPI_exec(ClientData cdata, Tcl_Interp *interp, { if (ntuples > 0) pltcl_set_tuple_values(interp, arrayname, 0, tuples[0], tupdesc); - sprintf(buf, "%d", ntuples); + snprintf(buf, sizeof(buf), "%d", ntuples); Tcl_SetResult(interp, buf, TCL_VOLATILE); + SPI_freetuptable(SPI_tuptable); memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart)); return TCL_OK; } + tuptable = SPI_tuptable; + /************************************************************ * There is a loop body - process all tuples and evaluate * the body on each @@ -1668,20 +1676,24 @@ pltcl_SPI_exec(ClientData cdata, Tcl_Interp *interp, continue; if (loop_rc == TCL_RETURN) { + SPI_freetuptable(tuptable); memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart)); return TCL_RETURN; } if (loop_rc == TCL_BREAK) break; + SPI_freetuptable(tuptable); memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart)); return TCL_ERROR; } + SPI_freetuptable(tuptable); + /************************************************************ * Finally return the number of tuples ************************************************************/ memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart)); - sprintf(buf, "%d", ntuples); + snprintf(buf, sizeof(buf), "%d", ntuples); Tcl_SetResult(interp, buf, TCL_VOLATILE); return TCL_OK; } @@ -1690,7 +1702,7 @@ pltcl_SPI_exec(ClientData cdata, Tcl_Interp *interp, /********************************************************************** * pltcl_SPI_prepare() - Builtin support for prepared plans * The Tcl command SPI_prepare - * allways saves the plan using + * always saves the plan using * SPI_saveplan and returns a key for * access. There is no chance to prepare * and not save the plan currently. @@ -1736,7 +1748,7 @@ pltcl_SPI_prepare(ClientData cdata, Tcl_Interp *interp, * Allocate the new querydesc structure ************************************************************/ qdesc = (pltcl_query_desc *) malloc(sizeof(pltcl_query_desc)); - sprintf(qdesc->qname, "%lx", (long) qdesc); + snprintf(qdesc->qname, sizeof(qdesc->qname), "%lx", (long) qdesc); qdesc->nargs = nargs; qdesc->argtypes = (Oid *) malloc(nargs * sizeof(Oid)); qdesc->arginfuncs = (FmgrInfo *) malloc(nargs * sizeof(FmgrInfo)); @@ -1815,7 +1827,7 @@ pltcl_SPI_prepare(ClientData cdata, Tcl_Interp *interp, break; default: - sprintf(buf, "unknown RC %d", SPI_result); + snprintf(buf, sizeof(buf), "unknown RC %d", SPI_result); reason = buf; break; @@ -1846,7 +1858,7 @@ pltcl_SPI_prepare(ClientData cdata, Tcl_Interp *interp, break; default: - sprintf(buf, "unknown RC %d", SPI_result); + snprintf(buf, sizeof(buf), "unknown RC %d", SPI_result); reason = buf; break; @@ -1897,6 +1909,7 @@ pltcl_SPI_execp(ClientData cdata, Tcl_Interp *interp, int ntuples; HeapTuple *volatile tuples = NULL; volatile TupleDesc tupdesc = NULL; + SPITupleTable *tuptable; sigjmp_buf save_restart; Tcl_HashTable *query_hash; @@ -2116,14 +2129,16 @@ pltcl_SPI_execp(ClientData cdata, Tcl_Interp *interp, { case SPI_OK_UTILITY: Tcl_SetResult(interp, "0", TCL_VOLATILE); + SPI_freetuptable(SPI_tuptable); return TCL_OK; case SPI_OK_SELINTO: case SPI_OK_INSERT: case SPI_OK_DELETE: case SPI_OK_UPDATE: - sprintf(buf, "%d", SPI_processed); + snprintf(buf, sizeof(buf), "%d", SPI_processed); Tcl_SetResult(interp, buf, TCL_VOLATILE); + SPI_freetuptable(SPI_tuptable); return TCL_OK; case SPI_OK_SELECT: @@ -2166,7 +2181,7 @@ pltcl_SPI_execp(ClientData cdata, Tcl_Interp *interp, return TCL_ERROR; default: - sprintf(buf, "%d", spi_rc); + snprintf(buf, sizeof(buf), "%d", spi_rc); Tcl_AppendResult(interp, "pltcl: SPI_exec() failed - ", "unknown RC ", buf, NULL); return TCL_ERROR; @@ -2208,11 +2223,14 @@ pltcl_SPI_execp(ClientData cdata, Tcl_Interp *interp, if (ntuples > 0) pltcl_set_tuple_values(interp, arrayname, 0, tuples[0], tupdesc); memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart)); - sprintf(buf, "%d", ntuples); + snprintf(buf, sizeof(buf), "%d", ntuples); Tcl_SetResult(interp, buf, TCL_VOLATILE); + SPI_freetuptable(SPI_tuptable); return TCL_OK; } + tuptable = SPI_tuptable; + /************************************************************ * There is a loop body - process all tuples and evaluate * the body on each @@ -2229,20 +2247,24 @@ pltcl_SPI_execp(ClientData cdata, Tcl_Interp *interp, continue; if (loop_rc == TCL_RETURN) { + SPI_freetuptable(tuptable); memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart)); return TCL_RETURN; } if (loop_rc == TCL_BREAK) break; + SPI_freetuptable(tuptable); memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart)); return TCL_ERROR; } + SPI_freetuptable(tuptable); + /************************************************************ * Finally return the number of tuples ************************************************************/ memcpy(&Warn_restart, &save_restart, sizeof(Warn_restart)); - sprintf(buf, "%d", ntuples); + snprintf(buf, sizeof(buf), "%d", ntuples); Tcl_SetResult(interp, buf, TCL_VOLATILE); return TCL_OK; } @@ -2258,7 +2280,7 @@ pltcl_SPI_lastoid(ClientData cdata, Tcl_Interp *interp, { char buf[64]; - sprintf(buf, "%u", SPI_lastoid); + snprintf(buf, sizeof(buf), "%u", SPI_lastoid); Tcl_SetResult(interp, buf, TCL_VOLATILE); return TCL_OK; } @@ -2300,7 +2322,7 @@ pltcl_set_tuple_values(Tcl_Interp *interp, char *arrayname, { arrptr = &arrayname; nameptr = &attname; - sprintf(buf, "%d", tupno); + snprintf(buf, sizeof(buf), "%d", tupno); Tcl_SetVar2(interp, arrayname, ".tupno", buf, 0); }