From: Tom Lane <tgl@sss.pgh.pa.us>
To: thomas.d.mckay@gmail.com
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17607: Server process crashes when PLpgSQL function raises error in subtransaction
Date: Sun, 25 Sep 2022 14:24:07 -0400
Message-ID: <3065565.1664130247@sss.pgh.pa.us> (raw)
In-Reply-To: <2452250.1662398179@sss.pgh.pa.us>
References: <17607-bd8ccc81226f7f80@postgresql.org>
<2452250.1662398179@sss.pgh.pa.us>
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 reduced
>> the code to a minimal reproducible example (as best I could).
> Thanks for the minimal reproducer! I confirm that this problem is visible
> 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 >= 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
Attachments:fix-trigger-tupdesc-refcount-issue.patch (text/x-diff, 967B) downloaddiff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.cindex 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 = MemoryContextSwitchTo(CurTransactionContext);
+ tupdesc = CreateTupleDescCopy(tupdesc);
table->storeslot = MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual);
MemoryContextSwitchTo(oldcxt);
}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, thomas.d.mckay@gmail.com, 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: <3065565.1664130247@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox