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 -0000      1.157
--- src/backend/access/transam/xact.c   1 Dec 2003 03:50:40 -0000
***************
*** 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 -0000      1.108
--- src/backend/executor/spi.c  1 Dec 2003 03:50:40 -0000
***************
*** 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 -0000      1.40
--- src/include/executor/spi.h  1 Dec 2003 03:50:43 -0000
***************
*** 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 SPI_cursor_close(Portal portal);
  
! extern void AtEOXact_SPI(bool isCommit);
  
  #endif   /* SPI_H */
---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]

Reply via email to