From: Tom Lane <tgl@sss.pgh.pa.us>
To: Japin Li <japinli@hotmail.com>
Cc: exclusion@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17983: Assert IsTransactionState() failed when empty string statement prepared in aborted transaction
Date: Tue, 20 Jun 2023 12:32:27 -0400
Message-ID: <3505791.1687278747@sss.pgh.pa.us> (raw)
In-Reply-To: <MEYP282MB16691E0295CE3501C8EABD89B65CA@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
References: <17983-da4569fcb878672e@postgresql.org>
<MEYP282MB16691E0295CE3501C8EABD89B65CA@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
Japin Li <japinli@hotmail.com> writes:
> On Tue, 20 Jun 2023 at 03:00, PG Bug reporting form <noreply@postgresql.org> wrote:
>> The following psql script:
>> BEGIN;
>> ERROR;
>> ;
>> \gdesc
>> triggers an assertion failure with the following stack trace:
>> so maybe an empty input string is not legal for creating a cached plan
>> when the transaction is aborted?
> Yeah, SearchSysCache1() need in an transaction block, here is a patch
> fixed it.
I'm not sure if anyone out there is expecting that this case should
work, but it probably did work at one time. Rather than throwing
an error, it'd be better to fix plancache.c so it doesn't fail.
I looked at the code and found that that's pretty much a one-line
fix, because there are already code paths that avoid doing anything
extra for transaction control commands (e.g ROLLBACK, which'd
otherwise have this same issue). We just need to make a NULL
parsetree use those paths.
> Another question, why should we need to create a plan cache entry for
> empty input?
Well, we have to have something to support the wire protocol behavior
for this case. No doubt we could hack up postgres.c to handle it
without a plan cache entry, but it'd be far more invasive to do it
there.
regards, tom lane
Attachments:
[text/x-diff] dont-fail-on-preparing-empty-statement.patch (645B, ../3505791.1687278747@sss.pgh.pa.us/2-dont-fail-on-preparing-empty-statement.patch)
download | inline diff:diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.cindex 87210fcf62..d4bfc55001 100644--- a/src/backend/utils/cache/plancache.c+++ b/src/backend/utils/cache/plancache.c@@ -78,9 +78,10 @@
/*
* We must skip "overhead" operations that involve database access when the
* cached plan's subject statement is a transaction control command.
+ * For convenience, treat empty statements as control commands too.
*/
#define IsTransactionStmtPlan(plansource) \
- ((plansource)->raw_parse_tree && \+ ((plansource)->raw_parse_tree == NULL || \
IsA((plansource)->raw_parse_tree->stmt, TransactionStmt))
/*
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, japinli@hotmail.com, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17983: Assert IsTransactionState() failed when empty string statement prepared in aborted transaction
In-Reply-To: <3505791.1687278747@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