Re: *_to_xml() should copy SPI_processed/SPI_tuptable

I wrote:
> Chapman Flack <chap@anastigmatix.net> writes:
>> Another alternative might be to have SPI_connect save them and
>> SPI_finish put them back, which leaves you just responsible for
>> reasoning about your own code. You'd still be expected to save them
>> across your own uses of other SPI calls, but no longer exposed to
>> spooky action at a distance from nested uses of SPI in stuff you call.

> Hmm.  I'd thought about that briefly and concluded that it didn't offer
> a full fix, but on reflection it's not clear why it'd be any less of
> a fix than the macroized-SPI_tuptable approach.  You end up with
> per-SPI-stack-level storage either way, and while that isn't perfect
> it does go a long way, as you say.  More, this has the huge advantage
> of being back-patchable, because there'd be no API/ABI change.

Here's a proposed patch along that line.  I included SPI_result (SPI's
equivalent of errno) in the set of saved-and-restored variables,
so that all the exposed SPI globals behave the same.

Also, in further service of providing insulation between SPI nesting
levels, I thought it'd be a good idea for SPI_connect() to reset the
globals to zero/NULL after saving them.  This ensures that a nesting
level can't accidentally be affected by the state of an outer level.
It's barely possible that there's somebody out there who's *intentionally*
accessing state from a calling SPI level, but that seems like it'd be
pretty dangerous programming practice.  Still, maybe there's an argument
for omitting that hunk in released branches.

Comments?

            regards, tom lane

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 5756365..11ca800 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -36,10 +36,16 @@
 #include "utils/typcache.h"
 
 
+/*
+ * These global variables are part of the API for various SPI functions
+ * (a horrible API choice, but it's too late now).  To reduce the risk of
+ * interference between different SPI callers, we save and restore them
+ * when entering/exiting a SPI nesting level.
+ */
 uint64        SPI_processed = 0;
 Oid            SPI_lastoid = InvalidOid;
 SPITupleTable *SPI_tuptable = NULL;
-int            SPI_result;
+int            SPI_result = 0;
 
 static _SPI_connection *_SPI_stack = NULL;
 static _SPI_connection *_SPI_current = NULL;
@@ -132,6 +138,10 @@ SPI_connect_ext(int options)
     _SPI_current->queryEnv = NULL;
     _SPI_current->atomic = (options & SPI_OPT_NONATOMIC ? false : true);
     _SPI_current->internal_xact = false;
+    _SPI_current->outer_processed = SPI_processed;
+    _SPI_current->outer_lastoid = SPI_lastoid;
+    _SPI_current->outer_tuptable = SPI_tuptable;
+    _SPI_current->outer_result = SPI_result;
 
     /*
      * Create memory contexts for this procedure
@@ -154,6 +164,15 @@ SPI_connect_ext(int options)
     /* ... and switch to procedure's context */
     _SPI_current->savedcxt = MemoryContextSwitchTo(_SPI_current->procCxt);
 
+    /*
+     * Reset API global variables so that current caller cannot accidentally
+     * depend on state of an outer caller.
+     */
+    SPI_processed = 0;
+    SPI_lastoid = InvalidOid;
+    SPI_tuptable = NULL;
+    SPI_result = 0;
+
     return SPI_OK_CONNECT;
 }
 
@@ -176,12 +195,13 @@ SPI_finish(void)
     _SPI_current->procCxt = NULL;
 
     /*
-     * Reset result variables, especially SPI_tuptable which is probably
+     * Restore outer API variables, especially SPI_tuptable which is probably
      * pointing at a just-deleted tuptable
      */
-    SPI_processed = 0;
-    SPI_lastoid = InvalidOid;
-    SPI_tuptable = NULL;
+    SPI_processed = _SPI_current->outer_processed;
+    SPI_lastoid = _SPI_current->outer_lastoid;
+    SPI_tuptable = _SPI_current->outer_tuptable;
+    SPI_result = _SPI_current->outer_result;
 
     /* Exit stack level */
     _SPI_connected--;
@@ -274,9 +294,11 @@ SPICleanup(void)
 {
     _SPI_current = NULL;
     _SPI_connected = -1;
+    /* Reset API global variables, too */
     SPI_processed = 0;
     SPI_lastoid = InvalidOid;
     SPI_tuptable = NULL;
+    SPI_result = 0;
 }
 
 /*
@@ -336,18 +358,20 @@ AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid)
         }
 
         /*
-         * Pop the stack entry and reset global variables.  Unlike
+         * Restore outer global variables and pop the stack entry.  Unlike
          * SPI_finish(), we don't risk switching to memory contexts that might
          * be already gone.
          */
+        SPI_processed = connection->outer_processed;
+        SPI_lastoid = connection->outer_lastoid;
+        SPI_tuptable = connection->outer_tuptable;
+        SPI_result = connection->outer_result;
+
         _SPI_connected--;
         if (_SPI_connected < 0)
             _SPI_current = NULL;
         else
             _SPI_current = &(_SPI_stack[_SPI_connected]);
-        SPI_processed = 0;
-        SPI_lastoid = InvalidOid;
-        SPI_tuptable = NULL;
     }
 
     if (found && isCommit)
diff --git a/src/include/executor/spi_priv.h b/src/include/executor/spi_priv.h
index 401fd99..0da3a41 100644
--- a/src/include/executor/spi_priv.h
+++ b/src/include/executor/spi_priv.h
@@ -42,6 +42,12 @@ typedef struct
                                  * transactions */
     bool        internal_xact;    /* SPI-managed transaction boundary, skip
                                  * cleanup */
+
+    /* saved values of API global variables for previous nesting level */
+    uint64        outer_processed;
+    Oid            outer_lastoid;
+    SPITupleTable *outer_tuptable;
+    int            outer_result;
 } _SPI_connection;
 
 /*