From: Tom Lane <tgl@sss.pgh.pa.us>
To: Japin Li <japinli@hotmail.com>
cc: 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: 
 <MEYP282MB16691E0295CE3501C8EABD89B65CA@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
References: <17983-da4569fcb878672e@postgresql.org>
 <MEYP282MB16691E0295CE3501C8EABD89B65CA@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
Comments: In-reply-to Japin Li <japinli@hotmail.com>
	message dated "Tue, 20 Jun 2023 16:17:33 +0800"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3505697.1687278682.0@sss.pgh.pa.us>
Date: Tue, 20 Jun 2023 12:32:27 -0400
Message-ID: <3505791.1687278747@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3505791.1687278747%40sss.pgh.pa.us>
Precedence: bulk

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

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="dont-fail-on-preparing-empty-statement.patch";
	charset="us-ascii"
Content-ID: <3505697.1687278682.2@sss.pgh.pa.us>
Content-Description: dont-fail-on-preparing-empty-statement.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache=
/plancache.c
index 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 t=
he
  * 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 =3D=3D NULL || \
 	 IsA((plansource)->raw_parse_tree->stmt, TransactionStmt))
 =

 /*

------- =_aaaaaaaaaa0--