From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: thomas.d.mckay@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17607: Server process crashes when PLpgSQL function raises
 error in subtransaction
In-reply-to: <3065565.1664130247@sss.pgh.pa.us>
References: <17607-bd8ccc81226f7f80@postgresql.org>
 <2452250.1662398179@sss.pgh.pa.us> <3065565.1664130247@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sun, 25 Sep 2022 14:24:07 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3092251.1664132457.0@sss.pgh.pa.us>
Date: Sun, 25 Sep 2022 15:01:15 -0400
Message-ID: <3092360.1664132475@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3092360.1664132475%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3092251.1664132457.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

I wrote:
> It seems to me there are probably other hazards here, because the tupdes=
c
> could possibly also go away before the slot does.  I think what we ought
> to do is copy the tupdesc, so that we have a non-refcounted descriptor
> that we know has exactly the right lifespan.  As attached.

Actually, after looking around some more, I realize that there is a
second mistake in 25936fd46: it ignored the comment on
AfterTriggerFreeQuery that

 *	Note: it's important for this to be safe if interrupted by an error
 *	and then called again for the same query level.

This is the reason why we end up in a recursive error and PANIC:
we keep trying to free the tupdesc again after the previous error.
If I fix that but omit the CreateTupleDescCopy step, then the
reproducer behaves much more sanely:

psql:bug17607.sql:29: WARNING:  AbortSubTransaction while in ABORT state
psql:bug17607.sql:29: ERROR:  BOOM !
CONTEXT:  PL/pgSQL function on_tbl_parent_id_change_fn() line 3 at RAISE
ERROR:  tupdesc reference 0x7fdef236a1b8 is not owned by resource owner Su=
bTransaction
ROLLBACK
ROLLBACK

So what we actually need here is more like the attached.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-trigger-tupdesc-refcount-issue-2.patch";
	charset="us-ascii"
Content-ID: <3092251.1664132457.2@sss.pgh.pa.us>
Content-Description: fix-trigger-tupdesc-refcount-issue-2.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger=
.c
index 59b38d00ed..1b1193997b 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -4340,11 +4340,13 @@ GetAfterTriggersStoreSlot(AfterTriggersTableData *=
table,
 		MemoryContext	oldcxt;
 =

 		/*
-		 * We only need this slot only until AfterTriggerEndQuery, but making
-		 * it last till end-of-subxact is good enough.  It'll be freed by
-		 * AfterTriggerFreeQuery().
+		 * We need this slot only until AfterTriggerEndQuery, but making it
+		 * last till end-of-subxact is good enough.  It'll be freed by
+		 * AfterTriggerFreeQuery().  However, the passed-in tupdesc might have
+		 * a different lifespan, so we'd better make a copy of that.
 		 */
 		oldcxt =3D MemoryContextSwitchTo(CurTransactionContext);
+		tupdesc =3D CreateTupleDescCopy(tupdesc);
 		table->storeslot =3D MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual);
 		MemoryContextSwitchTo(oldcxt);
 	}
@@ -4640,7 +4642,12 @@ AfterTriggerFreeQuery(AfterTriggersQueryData *qs)
 		if (ts)
 			tuplestore_end(ts);
 		if (table->storeslot)
-			ExecDropSingleTupleTableSlot(table->storeslot);
+		{
+			TupleTableSlot *slot =3D table->storeslot;
+
+			table->storeslot =3D NULL;
+			ExecDropSingleTupleTableSlot(slot);
+		}
 	}
 =

 	/*

------- =_aaaaaaaaaa0--