Re: Problem with dblink - Mailing list pgsql-patches

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 */