Re: SPI nesting in plperl - Mailing list pgsql-hackers

I wrote:
> I tried fixing this
> http://archives.postgresql.org/pgsql-general/2009-01/msg00030.php
> by inserting SPI_push/SPI_pop calls around plperl's use of
> InputFunctionCall and OutputFunctionCall ...

> I also thought about attacking the problem by having InputFunctionCall
> and OutputFunctionCall automatically do SPI_push/SPI_pop if they are
> called within an active SPI context.  I don't like this approach too
> much because it seems likely to mask bugs as often as fix them.  (In
> particular I'd be afraid to back-patch such a change.)  It might be the
> cleanest solution overall, though, particularly when you consider that
> we've probably got similar issues in pltcl, plpython, and add-on PLs.

I've done a trial patch along the second line, and on the whole I think
it's probably far safer than sprinkling the system with SPI_push/SPI_pop
calls.  Comments?

            regards, tom lane

Index: src/backend/executor/spi.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/spi.c,v
retrieving revision 1.204
diff -c -r1.204 spi.c
*** src/backend/executor/spi.c    2 Jan 2009 20:42:00 -0000    1.204
--- src/backend/executor/spi.c    6 Jan 2009 18:40:54 -0000
***************
*** 296,301 ****
--- 296,326 ----
      _SPI_curid--;
  }

+ /* Conditional push: push only if we're inside a SPI procedure */
+ bool
+ SPI_push_conditional(void)
+ {
+     bool    pushed = (_SPI_curid != _SPI_connected);
+
+     if (pushed)
+     {
+         _SPI_curid++;
+         /* We should now be in a state where SPI_connect would succeed */
+         Assert(_SPI_curid == _SPI_connected);
+     }
+     return pushed;
+ }
+
+ /* Conditional pop: pop only if SPI_push_conditional pushed */
+ void
+ SPI_pop_conditional(bool pushed)
+ {
+     /* We should be in a state where SPI_connect would succeed */
+     Assert(_SPI_curid == _SPI_connected);
+     if (pushed)
+         _SPI_curid--;
+ }
+
  /* Restore state of SPI stack after aborting a subtransaction */
  void
  SPI_restore_connection(void)
Index: src/backend/utils/fmgr/fmgr.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/fmgr/fmgr.c,v
retrieving revision 1.124
diff -c -r1.124 fmgr.c
*** src/backend/utils/fmgr/fmgr.c    1 Jan 2009 17:23:51 -0000    1.124
--- src/backend/utils/fmgr/fmgr.c    6 Jan 2009 18:40:55 -0000
***************
*** 1846,1861 ****
--- 1851,1875 ----
   * the caller should assume the result is NULL, but we'll call the input
   * function anyway if it's not strict.  So this is almost but not quite
   * the same as FunctionCall3.
+  *
+  * One important difference from the bare function call is that we will
+  * push any active SPI context, allowing SPI-using I/O functions to be
+  * called from other SPI functions without extra notation.  This is a hack,
+  * but the alternative of expecting all SPI functions to do SPI_push/SPI_pop
+  * around I/O calls seems worse.
   */
  Datum
  InputFunctionCall(FmgrInfo *flinfo, char *str, Oid typioparam, int32 typmod)
  {
      FunctionCallInfoData fcinfo;
      Datum        result;
+     bool        pushed;

      if (str == NULL && flinfo->fn_strict)
          return (Datum) 0;        /* just return null result */

+     pushed = SPI_push_conditional();
+
      InitFunctionCallInfoData(fcinfo, flinfo, 3, NULL, NULL);

      fcinfo.arg[0] = CStringGetDatum(str);
***************
*** 1881,1886 ****
--- 1895,1902 ----
                   fcinfo.flinfo->fn_oid);
      }

+     SPI_pop_conditional(pushed);
+
      return result;
  }

***************
*** 1889,1901 ****
   *
   * Do not call this on NULL datums.
   *
!  * This is mere window dressing for FunctionCall1, but its use is recommended
!  * anyway so that code invoking output functions can be identified easily.
   */
  char *
  OutputFunctionCall(FmgrInfo *flinfo, Datum val)
  {
!     return DatumGetCString(FunctionCall1(flinfo, val));
  }

  /*
--- 1905,1926 ----
   *
   * Do not call this on NULL datums.
   *
!  * This is almost just window dressing for FunctionCall1, but it includes
!  * SPI context pushing for the same reasons as InputFunctionCall.
   */
  char *
  OutputFunctionCall(FmgrInfo *flinfo, Datum val)
  {
!     char       *result;
!     bool        pushed;
!
!     pushed = SPI_push_conditional();
!
!     result = DatumGetCString(FunctionCall1(flinfo, val));
!
!     SPI_pop_conditional(pushed);
!
!     return result;
  }

  /*
***************
*** 1904,1910 ****
   * "buf" may be NULL to indicate we are reading a NULL.  In this case
   * the caller should assume the result is NULL, but we'll call the receive
   * function anyway if it's not strict.  So this is almost but not quite
!  * the same as FunctionCall3.
   */
  Datum
  ReceiveFunctionCall(FmgrInfo *flinfo, StringInfo buf,
--- 1929,1936 ----
   * "buf" may be NULL to indicate we are reading a NULL.  In this case
   * the caller should assume the result is NULL, but we'll call the receive
   * function anyway if it's not strict.  So this is almost but not quite
!  * the same as FunctionCall3.  Also, this includes SPI context pushing for
!  * the same reasons as InputFunctionCall.
   */
  Datum
  ReceiveFunctionCall(FmgrInfo *flinfo, StringInfo buf,
***************
*** 1912,1921 ****
--- 1938,1950 ----
  {
      FunctionCallInfoData fcinfo;
      Datum        result;
+     bool        pushed;

      if (buf == NULL && flinfo->fn_strict)
          return (Datum) 0;        /* just return null result */

+     pushed = SPI_push_conditional();
+
      InitFunctionCallInfoData(fcinfo, flinfo, 3, NULL, NULL);

      fcinfo.arg[0] = PointerGetDatum(buf);
***************
*** 1941,1946 ****
--- 1970,1977 ----
                   fcinfo.flinfo->fn_oid);
      }

+     SPI_pop_conditional(pushed);
+
      return result;
  }

***************
*** 1949,1962 ****
   *
   * Do not call this on NULL datums.
   *
!  * This is little more than window dressing for FunctionCall1, but its use is
!  * recommended anyway so that code invoking output functions can be identified
!  * easily.    Note however that it does guarantee a non-toasted result.
   */
  bytea *
  SendFunctionCall(FmgrInfo *flinfo, Datum val)
  {
!     return DatumGetByteaP(FunctionCall1(flinfo, val));
  }

  /*
--- 1980,2003 ----
   *
   * Do not call this on NULL datums.
   *
!  * This is little more than window dressing for FunctionCall1, but it does
!  * guarantee a non-toasted result, which strictly speaking the underlying
!  * function doesn't.  Also, this includes SPI context pushing for the same
!  * reasons as InputFunctionCall.
   */
  bytea *
  SendFunctionCall(FmgrInfo *flinfo, Datum val)
  {
!     bytea       *result;
!     bool        pushed;
!
!     pushed = SPI_push_conditional();
!
!     result = DatumGetByteaP(FunctionCall1(flinfo, val));
!
!     SPI_pop_conditional(pushed);
!
!     return result;
  }

  /*
Index: src/include/executor/spi.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/executor/spi.h,v
retrieving revision 1.68
diff -c -r1.68 spi.h
*** src/include/executor/spi.h    1 Jan 2009 17:23:59 -0000    1.68
--- src/include/executor/spi.h    6 Jan 2009 18:40:55 -0000
***************
*** 93,98 ****
--- 93,100 ----
  extern int    SPI_finish(void);
  extern void SPI_push(void);
  extern void SPI_pop(void);
+ extern bool SPI_push_conditional(void);
+ extern void SPI_pop_conditional(bool pushed);
  extern void SPI_restore_connection(void);
  extern int    SPI_execute(const char *src, bool read_only, long tcount);
  extern int SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.226
diff -c -r1.226 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c    1 Jan 2009 17:24:03 -0000    1.226
--- src/pl/plpgsql/src/pl_exec.c    6 Jan 2009 18:40:55 -0000
***************
*** 4689,4715 ****
  static char *
  convert_value_to_string(Datum value, Oid valtype)
  {
-     char       *str;
      Oid            typoutput;
      bool        typIsVarlena;

      getTypeOutputInfo(valtype, &typoutput, &typIsVarlena);
!
!     /*
!      * We do SPI_push to allow the datatype output function to use SPI.
!      * However we do not mess around with CommandCounterIncrement or advancing
!      * the snapshot, which means that a stable output function would not see
!      * updates made so far by our own function.  The use-case for such
!      * scenarios seems too narrow to justify the cycles that would be
!      * expended.
!      */
!     SPI_push();
!
!     str = OidOutputFunctionCall(typoutput, value);
!
!     SPI_pop();
!
!     return str;
  }

  /* ----------
--- 4689,4699 ----
  static char *
  convert_value_to_string(Datum value, Oid valtype)
  {
      Oid            typoutput;
      bool        typIsVarlena;

      getTypeOutputInfo(valtype, &typoutput, &typIsVarlena);
!     return OidOutputFunctionCall(typoutput, value);
  }

  /* ----------
***************
*** 4735,4759 ****
              char       *extval;

              extval = convert_value_to_string(value, valtype);
-
-             /* Allow input function to use SPI ... see notes above */
-             SPI_push();
-
              value = InputFunctionCall(reqinput, extval,
                                        reqtypioparam, reqtypmod);
-
-             SPI_pop();
-
              pfree(extval);
          }
          else
          {
-             SPI_push();
-
              value = InputFunctionCall(reqinput, NULL,
                                        reqtypioparam, reqtypmod);
-
-             SPI_pop();
          }
      }

--- 4719,4732 ----