I wrote:
> Chapman Flack <c...@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;
/*
