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

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

I wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> I've encountered a bug while doing some SQL migrations on a PostgreSQL =
13.8
>> server. My initial use-case is a Django app running migrations, a test =
setup
>> and a test, but that code was more than 10K lines of DDL, so I've reduc=
ed
>> the code to a minimal reproducible example (as best I could).

> Thanks for the minimal reproducer!  I confirm that this problem is visib=
le
> in the v12 and v13 branches, but not before or after.

I got a chance to look at this today, and what I find is that we're making
a tuple slot that will be freed at end of subtransaction, but it has a
refcount on a refcounted tuple descriptor that belongs to the Portal's
resowner.  While trying to free the slot, we get a complaint because the
subtransaction's resowner isn't holding the appropriate tupdesc reference.

It seems to me there are probably other hazards here, because the tupdesc
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.

Not sure about whether to bother with a test case.  The submitted
reproducer doesn't work in >=3D v14, and even in v12/v13 it seems awfully
corner-case-ish.  (I find for instance that you need the ALTER TABLE
step rather than just making the table in one step, and I'm a tad
baffled as to why.)  In any case, I've got zero faith that there are
not other ways to trigger the same problem, so I think we need to apply
this up to HEAD.

			regards, tom lane


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

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger=
.c
index 59b38d00ed..b9a012512d 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);
 	}

------- =_aaaaaaaaaa0--