Re: [BUGS] pg_logical_slot_peek_changes crashes postgres when called from inside pl/pgsql

Andres Freund <andres@anarazel.de> writes:
> I'm kinda surprised that this only causes problems with logical decoding
> and not elsewhere, this isn't a new issue.

The reason for that seems to be that ReorderBufferCommit and
ReorderBufferImmediateInvalidation are the only users of
BeginInternalSubTransaction other than the PLs.  So otherwise we
never see a case of subtransaction abort where we don't want
to clean up the current operation in the surrounding SPI context.

> So far your option of allowing to opt in into additional cleanup in the
> CATCH seems the least ugly

I thought of a better way, as attached.  The core problem is that we
need to know whether the SPI context's current executor operation is
"inside" or "outside" the subtransaction being abandoned.  That info
is not tracked at the moment, but we can track it, at a cost that's
entirely trivial compared to the other stuff that a SPI operation
will do.

This seems back-patchable without much angst, although we might want
to add the new _SPI_connection field at the end in the back branches.
I'm not sure if anything outside core is looking at spi_priv.h.

            regards, tom lane

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index afe231f..a2de087 100644
*** a/src/backend/executor/spi.c
--- b/src/backend/executor/spi.c
*************** static void _SPI_cursor_operation(Portal
*** 71,78 ****
  static SPIPlanPtr _SPI_make_plan_non_temp(SPIPlanPtr plan);
  static SPIPlanPtr _SPI_save_plan(SPIPlanPtr plan);

! static int    _SPI_begin_call(bool execmem);
! static int    _SPI_end_call(bool procmem);
  static MemoryContext _SPI_execmem(void);
  static MemoryContext _SPI_procmem(void);
  static bool _SPI_checktuples(void);
--- 71,78 ----
  static SPIPlanPtr _SPI_make_plan_non_temp(SPIPlanPtr plan);
  static SPIPlanPtr _SPI_save_plan(SPIPlanPtr plan);

! static int    _SPI_begin_call(bool use_exec);
! static int    _SPI_end_call(bool use_exec);
  static MemoryContext _SPI_execmem(void);
  static MemoryContext _SPI_procmem(void);
  static bool _SPI_checktuples(void);
*************** SPI_connect(void)
*** 118,123 ****
--- 118,124 ----
      _SPI_current->processed = 0;
      _SPI_current->lastoid = InvalidOid;
      _SPI_current->tuptable = NULL;
+     _SPI_current->execSubid = InvalidSubTransactionId;
      slist_init(&_SPI_current->tuptables);
      _SPI_current->procCxt = NULL;    /* in case we fail to create 'em */
      _SPI_current->execCxt = NULL;
*************** SPI_finish(void)
*** 149,155 ****
  {
      int            res;

!     res = _SPI_begin_call(false);    /* live in procedure memory */
      if (res < 0)
          return res;

--- 150,156 ----
  {
      int            res;

!     res = _SPI_begin_call(false);    /* just check we're connected */
      if (res < 0)
          return res;

*************** AtEOSubXact_SPI(bool isCommit, SubTransa
*** 268,275 ****
      {
          slist_mutable_iter siter;

!         /* free Executor memory the same as _SPI_end_call would do */
!         MemoryContextResetAndDeleteChildren(_SPI_current->execCxt);

          /* throw away any tuple tables created within current subxact */
          slist_foreach_modify(siter, &_SPI_current->tuptables)
--- 269,283 ----
      {
          slist_mutable_iter siter;

!         /*
!          * Throw away executor state if current executor operation was started
!          * within current subxact (essentially, force a _SPI_end_call).
!          */
!         if (_SPI_current->execSubid >= mySubid)
!         {
!             _SPI_current->execSubid = InvalidSubTransactionId;
!             MemoryContextResetAndDeleteChildren(_SPI_current->execCxt);
!         }

          /* throw away any tuple tables created within current subxact */
          slist_foreach_modify(siter, &_SPI_current->tuptables)
*************** AtEOSubXact_SPI(bool isCommit, SubTransa
*** 293,300 ****
                  MemoryContextDelete(tuptable->tuptabcxt);
              }
          }
-         /* in particular we should have gotten rid of any in-progress table */
-         Assert(_SPI_current->tuptable == NULL);
      }
  }

--- 301,306 ----
*************** _SPI_procmem(void)
*** 2446,2460 ****

  /*
   * _SPI_begin_call: begin a SPI operation within a connected procedure
   */
  static int
! _SPI_begin_call(bool execmem)
  {
      if (_SPI_current == NULL)
          return SPI_ERROR_UNCONNECTED;

!     if (execmem)                /* switch to the Executor memory context */
          _SPI_execmem();

      return 0;
  }
--- 2452,2475 ----

  /*
   * _SPI_begin_call: begin a SPI operation within a connected procedure
+  *
+  * use_exec is true if we intend to make use of the procedure's execCxt
+  * during this SPI operation.  We'll switch into that context, and arrange
+  * for it to be cleaned up at _SPI_end_call or if an error occurs.
   */
  static int
! _SPI_begin_call(bool use_exec)
  {
      if (_SPI_current == NULL)
          return SPI_ERROR_UNCONNECTED;

!     if (use_exec)
!     {
!         /* remember when the Executor operation started */
!         _SPI_current->execSubid = GetCurrentSubTransactionId();
!         /* switch to the Executor memory context */
          _SPI_execmem();
+     }

      return 0;
  }
*************** _SPI_begin_call(bool execmem)
*** 2462,2475 ****
  /*
   * _SPI_end_call: end a SPI operation within a connected procedure
   *
   * Note: this currently has no failure return cases, so callers don't check
   */
  static int
! _SPI_end_call(bool procmem)
  {
!     if (procmem)                /* switch to the procedure memory context */
      {
          _SPI_procmem();
          /* and free Executor memory */
          MemoryContextResetAndDeleteChildren(_SPI_current->execCxt);
      }
--- 2477,2495 ----
  /*
   * _SPI_end_call: end a SPI operation within a connected procedure
   *
+  * use_exec must be the same as in the previous _SPI_begin_call
+  *
   * Note: this currently has no failure return cases, so callers don't check
   */
  static int
! _SPI_end_call(bool use_exec)
  {
!     if (use_exec)
      {
+         /* switch to the procedure memory context */
          _SPI_procmem();
+         /* mark Executor context no longer in use */
+         _SPI_current->execSubid = InvalidSubTransactionId;
          /* and free Executor memory */
          MemoryContextResetAndDeleteChildren(_SPI_current->execCxt);
      }
diff --git a/src/include/executor/spi_priv.h b/src/include/executor/spi_priv.h
index ba7fb98..8fae755 100644
*** a/src/include/executor/spi_priv.h
--- b/src/include/executor/spi_priv.h
*************** typedef struct
*** 26,31 ****
--- 26,34 ----
      Oid            lastoid;
      SPITupleTable *tuptable;    /* tuptable currently being built */

+     /* subtransaction in which current Executor call was started */
+     SubTransactionId execSubid;
+
      /* resources of this execution context */
      slist_head    tuptables;        /* list of all live SPITupleTables */
      MemoryContext procCxt;        /* procedure context */

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs