Tom Lane wrote:
1. AFAIR, all the other at-end-of-xact routines that take a flag telling
them if it's commit or abort define the flag as isCommit. Having the
reverse convention for this one routine is confusing and a recipe for
errors, so please make it be isCommit too.
Done.
2. The initial comment for AtEOXact_SPI:
* Clean up SPI state at transaction commit or abort (we don't care which).
is now incorrect and needs to be updated (at least get rid of the
parenthetical note).
Done.
! if (_SPI_stack != NULL)
! {
free(_SPI_stack);
+ if (!isAbort)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING),
+ errmsg(freeing non-empty SPI stack),
+ errhint(Check for missing \SPI_finish\ calls)));
+ }
+
While this isn't necessarily wrong, what would probably be a stronger
test is to complain if either _SPI_connected or _SPI_curid is not -1.
For one thing that would catch missing SPI_pop(). (Jan, any comment
about that?)
Based on some simple testing, a warning for (_SPI_connected != -1) seems
redundant with (_SPI_stack != NULL) -- i.e. I just get both warnings
when SPI_finish() isn't called. And commenting out SPI_pop() causes a
SPI_finish failed error, at least in the one place it's used
(pl_exec.c), so we never get to AtEOXact_SPI(). So for the moment at
least I left that last section the same.
OK to commit?
Joe
Index: src/backend/access/transam/xact.c
===
RCS file: /cvsroot/pgsql-server/src/backend/access/transam/xact.c,v
retrieving revision 1.157
diff -c -r1.157 xact.c
*** src/backend/access/transam/xact.c 29 Nov 2003 19:51:40 - 1.157
--- src/backend/access/transam/xact.c 1 Dec 2003 03:50:40 -
***
*** 977,983
CallEOXactCallbacks(true);
AtEOXact_GUC(true);
! AtEOXact_SPI();
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
--- 977,983
CallEOXactCallbacks(true);
AtEOXact_GUC(true);
! AtEOXact_SPI(true);
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
***
*** 1087,1093
CallEOXactCallbacks(false);
AtEOXact_GUC(false);
! AtEOXact_SPI();
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
--- 1087,1093
CallEOXactCallbacks(false);
AtEOXact_GUC(false);
! AtEOXact_SPI(false);
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
Index: src/backend/executor/spi.c
===
RCS file: /cvsroot/pgsql-server/src/backend/executor/spi.c,v
retrieving revision 1.108
diff -c -r1.108 spi.c
*** src/backend/executor/spi.c 29 Nov 2003 19:51:48 - 1.108
--- src/backend/executor/spi.c 1 Dec 2003 03:50:40 -
***
*** 174,191
}
/*
! * Clean up SPI state at transaction commit or abort (we don't care which).
*/
void
! AtEOXact_SPI(void)
{
/*
* Note that memory contexts belonging to SPI stack entries will be
* freed automatically, so we can ignore them here. We just need to
* restore our static variables to initial state.
*/
! if (_SPI_stack != NULL) /* there was abort */
free(_SPI_stack);
_SPI_current = _SPI_stack = NULL;
_SPI_connected = _SPI_curid = -1;
SPI_processed = 0;
--- 174,199
}
/*
! * Clean up SPI state at transaction commit or abort.
*/
void
! AtEOXact_SPI(bool isCommit)
{
/*
* Note that memory contexts belonging to SPI stack entries will be
* freed automatically, so we can ignore them here. We just need to
* restore our static variables to initial state.
*/
! if (_SPI_stack != NULL)
! {
free(_SPI_stack);
+ if (isCommit)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING),
+errmsg(freeing non-empty SPI stack),
+errhint(Check for missing \SPI_finish\
calls)));
+ }
+
_SPI_current = _SPI_stack = NULL;
_SPI_connected = _SPI_curid = -1;
SPI_processed = 0;
Index: src/include/executor/spi.h
===
RCS file: /cvsroot/pgsql-server/src/include/executor/spi.h,v
retrieving revision 1.40
diff -c -r1.40 spi.h
*** src/include/executor/spi.h 29 Nov 2003 22:41:01 - 1.40
--- src/include/executor/spi.h 1 Dec 2003 03:50:43 -
***
*** 116,121
extern void SPI_cursor_move(Portal portal, bool forward, int count);
extern void SPI_cursor_close(Portal portal);
! extern void AtEOXact_SPI(void);
#endif /* SPI_H */
--- 116,121
extern void SPI_cursor_move(Portal portal, bool forward, int count);
extern void
