_SPI_execute_plan failed to return result tuple table to caller in

the ProcessUtility case, resulting in an intratransaction memory leak
if a utility command actually did return any tuples, as reported by
Dmitry Karasik.  Fix this and also make the behavior more consistent
for cases involving nested SPI operations and multiple query trees,
by ensuring that we store the state locally until it is ready to be
returned to the caller.

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index f00a2c3c4116cf3e6c14b74dc711f9d98130d596..ff1b8932ea1e93902c3cf1c837c84fc432b2d4df 100644 (file)
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.141 2005/06/09 21:25:22 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.142 2005/10/01 18:43:19 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -104,6 +104,7 @@ SPI_connect(void)
 
        _SPI_current = &(_SPI_stack[_SPI_connected]);
        _SPI_current->processed = 0;
+       _SPI_current->lastoid = InvalidOid;
        _SPI_current->tuptable = NULL;
        _SPI_current->procCxt = NULL; /* in case we fail to create 'em */
        _SPI_current->execCxt = NULL;
@@ -859,7 +860,7 @@ SPI_cursor_open(const char *name, void *plan,
                        break;
        }
 
-       /* Reset SPI result */
+       /* Reset SPI result (note we deliberately don't touch lastoid) */
        SPI_processed = 0;
        SPI_tuptable = NULL;
        _SPI_current->processed = 0;
@@ -1313,6 +1314,9 @@ _SPI_execute_plan(_SPI_plan *plan, Datum *Values, const char *Nulls,
                                  bool read_only, long tcount)
 {
        volatile int res = 0;
+       volatile uint32 my_processed = 0;
+       volatile Oid my_lastoid = InvalidOid;
+       SPITupleTable * volatile my_tuptable = NULL;
        Snapshot        saveActiveSnapshot;
 
        /* Be sure to restore ActiveSnapshot on error exit */
@@ -1347,12 +1351,6 @@ _SPI_execute_plan(_SPI_plan *plan, Datum *Values, const char *Nulls,
                else
                        paramLI = NULL;
 
-               /* Reset state (only needed in case string is empty) */
-               SPI_processed = 0;
-               SPI_lastoid = InvalidOid;
-               SPI_tuptable = NULL;
-               _SPI_current->tuptable = NULL;
-
                /*
                 * Setup error traceback support for ereport()
                 */
@@ -1366,13 +1364,6 @@ _SPI_execute_plan(_SPI_plan *plan, Datum *Values, const char *Nulls,
                        List       *query_list = lfirst(query_list_list_item);
                        ListCell   *query_list_item;
 
-                       /* Reset state for each original parsetree */
-                       /* (at most one of its querytrees will be marked canSetTag) */
-                       SPI_processed = 0;
-                       SPI_lastoid = InvalidOid;
-                       SPI_tuptable = NULL;
-                       _SPI_current->tuptable = NULL;
-
                        foreach(query_list_item, query_list)
                        {
                                Query      *queryTree = (Query *) lfirst(query_list_item);
@@ -1383,6 +1374,10 @@ _SPI_execute_plan(_SPI_plan *plan, Datum *Values, const char *Nulls,
                                planTree = lfirst(plan_list_item);
                                plan_list_item = lnext(plan_list_item);
 
+                               _SPI_current->processed = 0;
+                               _SPI_current->lastoid = InvalidOid;
+                               _SPI_current->tuptable = NULL;
+
                                if (queryTree->commandType == CMD_UTILITY)
                                {
                                        if (IsA(queryTree->utilityStmt, CopyStmt))
@@ -1467,6 +1462,23 @@ _SPI_execute_plan(_SPI_plan *plan, Datum *Values, const char *Nulls,
                                }
                                FreeSnapshot(ActiveSnapshot);
                                ActiveSnapshot = NULL;
+                               /*
+                                * The last canSetTag query sets the auxiliary values returned
+                                * to the caller.  Be careful to free any tuptables not
+                                * returned, to avoid intratransaction memory leak.
+                                */
+                               if (queryTree->canSetTag)
+                               {
+                                       my_processed = _SPI_current->processed;
+                                       my_lastoid = _SPI_current->lastoid;
+                                       SPI_freetuptable(my_tuptable);
+                                       my_tuptable = _SPI_current->tuptable;
+                               }
+                               else
+                               {
+                                       SPI_freetuptable(_SPI_current->tuptable);
+                                       _SPI_current->tuptable = NULL;
+                               }
                                /* we know that the receiver doesn't need a destroy call */
                                if (res < 0)
                                        goto fail;
@@ -1490,6 +1502,11 @@ fail:
 
        ActiveSnapshot = saveActiveSnapshot;
 
+       /* Save results for caller */
+       SPI_processed = my_processed;
+       SPI_lastoid = my_lastoid;
+       SPI_tuptable = my_tuptable;
+
        return res;
 }
 
@@ -1497,9 +1514,7 @@ static int
 _SPI_pquery(QueryDesc *queryDesc, long tcount)
 {
        int                     operation = queryDesc->operation;
-       CommandDest     origDest = queryDesc->dest->mydest;
        int                     res;
-       Oid                     save_lastoid;
 
        switch (operation)
        {
@@ -1510,6 +1525,11 @@ _SPI_pquery(QueryDesc *queryDesc, long tcount)
                                res = SPI_OK_SELINTO;
                                queryDesc->dest = None_Receiver;        /* don't output results */
                        }
+                       else if (queryDesc->dest->mydest != SPI)
+                       {
+                               /* Don't return SPI_OK_SELECT if we're discarding result */
+                               res = SPI_OK_UTILITY;
+                       }
                        break;
                case CMD_INSERT:
                        res = SPI_OK_INSERT;
@@ -1536,7 +1556,7 @@ _SPI_pquery(QueryDesc *queryDesc, long tcount)
        ExecutorRun(queryDesc, ForwardScanDirection, tcount);
 
        _SPI_current->processed = queryDesc->estate->es_processed;
-       save_lastoid = queryDesc->estate->es_lastoid;
+       _SPI_current->lastoid = queryDesc->estate->es_lastoid;
 
        if (operation == CMD_SELECT && queryDesc->dest->mydest == SPI)
        {
@@ -1549,19 +1569,6 @@ _SPI_pquery(QueryDesc *queryDesc, long tcount)
 
        ExecutorEnd(queryDesc);
 
-       /* Test origDest here so that SPI_processed gets set in SELINTO case */
-       if (origDest == SPI)
-       {
-               SPI_processed = _SPI_current->processed;
-               SPI_lastoid = save_lastoid;
-               SPI_tuptable = _SPI_current->tuptable;
-       }
-       else if (res == SPI_OK_SELECT)
-       {
-               /* Don't return SPI_OK_SELECT if we discarded the result */
-               res = SPI_OK_UTILITY;
-       }
-
 #ifdef SPI_EXECUTOR_STATS
        if (ShowExecutorStats)
                ShowUsage("SPI EXECUTOR STATS");
@@ -1615,7 +1622,7 @@ _SPI_cursor_operation(Portal portal, bool forward, long count,
        if (_SPI_begin_call(true) < 0)
                elog(ERROR, "SPI cursor operation called while not connected");
 
-       /* Reset the SPI result */
+       /* Reset the SPI result (note we deliberately don't touch lastoid) */
        SPI_processed = 0;
        SPI_tuptable = NULL;
        _SPI_current->processed = 0;

diff --git a/src/include/executor/spi_priv.h b/src/include/executor/spi_priv.h
index 1ba7d765e59fba1acc308b16d51b7fb0d107945e..a21bbc5e7c067b4bd1ed35160913695dcdeacb2b 100644 (file)
--- a/src/include/executor/spi_priv.h
+++ b/src/include/executor/spi_priv.h
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/executor/spi_priv.h,v 1.22 2004/12/31 22:03:29 pgsql Exp $
+ * $PostgreSQL: pgsql/src/include/executor/spi_priv.h,v 1.23 2005/10/01 18:43:19 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -18,8 +18,11 @@
 
 typedef struct
 {
+       /* current results */
        uint32          processed;              /* by Executor */
+       Oid                     lastoid;
        SPITupleTable *tuptable;
+
        MemoryContext procCxt;          /* procedure context */
        MemoryContext execCxt;          /* executor context */
        MemoryContext savedcxt;