From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: thomas.d.mckay@gmail.com
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 15:01:15 -0400
Message-ID: <3092360.1664132475@sss.pgh.pa.us> (raw)
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>
I wrote:
> 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.
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 SubTransaction
ROLLBACK
ROLLBACK
So what we actually need here is more like the attached.
regards, tom lane
Attachments:fix-trigger-tupdesc-refcount-issue-2.patch (text/x-diff, 1.3K) downloaddiff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.cindex 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 = MemoryContextSwitchTo(CurTransactionContext);
+ tupdesc = CreateTupleDescCopy(tupdesc);
table->storeslot = 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 = table->storeslot;++ table->storeslot = NULL;+ ExecDropSingleTupleTableSlot(slot);+ }
}
/*
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, alvherre@alvh.no-ip.org, 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: <3092360.1664132475@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