_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 7999db6f39b7169a9fa780807b66c60ee03fb23b..d3cb68396e6802696ded5136daaa3e23701e3d49 100644 (file)
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -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 c07fb365ca8eb3cb76b796b6a5f0cbabbd560743..f86f790653fcf722413007fe39f94505603650e5 100644 (file)
--- a/src/include/executor/spi_priv.h
+++ b/src/include/executor/spi_priv.h
@@ -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;