Обсуждение: BUG #17983: Assert IsTransactionState() failed when empty string statement prepared in aborted transaction
BUG #17983: Assert IsTransactionState() failed when empty string statement prepared in aborted transaction
От
PG Bug reporting form
Дата:
The following bug has been logged on the website:
Bug reference: 17983
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 16beta1
Operating system: Ubuntu 22.04
Description:
The following psql script:
BEGIN;
ERROR;
;
\gdesc
triggers an assertion failure with the following stack trace:
Core was generated by `postgres: law regression [local] PARSE
'.
Program terminated with signal SIGABRT, Aborted.
warning: Section `.reg-xstate/2900464' in core file too small.
#0 __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140715061659456) at ./nptl/pthread_kill.c:44
44 ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0 __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140715061659456) at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=140715061659456) at
./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=140715061659456, signo=signo@entry=6) at
./nptl/pthread_kill.c:89
#3 0x00007ffac6a09476 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4 0x00007ffac69ef7f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x000055917976c830 in ExceptionalCondition (conditionName=0x5591799946f4
"IsTransactionState()",
fileName=0x55917999443e "catcache.c", lineNumber=1208) at assert.c:66
#6 0x0000559179747288 in SearchCatCacheInternal (cache=0x55917acc5380,
nkeys=1, v1=10, v2=0, v3=0, v4=0)
at catcache.c:1208
#7 0x0000559179747159 in SearchCatCache1 (cache=0x55917acc5380, v1=10) at
catcache.c:1162
#8 0x0000559179764caf in SearchSysCache1 (cacheId=11, key1=10) at
syscache.c:825
#9 0x000055917912ed4d in recomputeNamespacePath () at namespace.c:3834
#10 0x000055917912e180 in GetOverrideSearchPath (context=0x55917ad3a5a0) at
namespace.c:3409
#11 0x00005591797521e4 in CompleteCachedPlan (plansource=0x55917ac92ae8,
querytree_list=0x0,
querytree_context=0x55917ad3a5a0, param_types=0x0, num_params=0,
parserSetup=0x0, parserSetupArg=0x0,
cursor_options=2048, fixed_result=true) at plancache.c:408
#12 0x000055917958392c in exec_parse_message (query_string=0x55917ac68679
";", stmt_name=0x55917ac68678 "",
paramTypes=0x0, numParams=0) at postgres.c:1550
#13 0x00005591795884b3 in PostgresMain (dbname=0x55917aca0af8 "regression",
username=0x55917ac646e8 "law")
at postgres.c:4663
#14 0x00005591794a94ff in BackendRun (port=0x55917ac91090) at
postmaster.c:4461
#15 0x00005591794a8d8b in BackendStartup (port=0x55917ac91090) at
postmaster.c:4189
#16 0x00005591794a50d0 in ServerLoop () at postmaster.c:1779
#17 0x00005591794a497a in PostmasterMain (argc=3, argv=0x55917ac62620) at
postmaster.c:1463
#18 0x00005591793591f2 in main (argc=3, argv=0x55917ac62620) at main.c:198
With "SELECT 1" instead of ";" I get:
ERROR: current transaction is aborted, commands ignored until end of
transaction block
as expected.
postgres.c contains:
if (parsetree_list != NIL)
{
...
/*
* If we are in an aborted transaction, reject all commands except
* COMMIT/ROLLBACK. It is important that this test occur before we
* try to do parse analysis, rewrite, or planning, since all those
* phases try to do database accesses, which may fail in abort state.
* (It might be safe to allow some additional utility commands in this
* state, but not many...)
*/
if (IsAbortedTransactionBlockState() &&
!IsTransactionExitStmt(raw_parse_tree->stmt))
ereport(ERROR,
(errcode(ERRCODE_IN_FAILED_SQL_TRANSACTION),
errmsg("current transaction is aborted, "
"commands ignored until end of transaction block"),
errdetail_abort()));
...
}
else
{
/* Empty input string. This is legal. */
raw_parse_tree = NULL;
psrc = CreateCachedPlan(raw_parse_tree, query_string,
CMDTAG_UNKNOWN);
querytree_list = NIL;
}
...
so maybe an empty input string is not legal for creating a cached plan
when the transaction is aborted?
Reproduced on REL_11_STABLE .. HEAD.
On Tue, 20 Jun 2023 at 03:00, PG Bug reporting form <noreply@postgresql.org> wrote:
> The following bug has been logged on the website:
>
> Bug reference: 17983
> Logged by: Alexander Lakhin
> Email address: exclusion@gmail.com
> PostgreSQL version: 16beta1
> Operating system: Ubuntu 22.04
> Description:
>
> The following psql script:
> BEGIN;
> ERROR;
> ;
> \gdesc
>
> triggers an assertion failure with the following stack trace:
> Core was generated by `postgres: law regression [local] PARSE
> '.
> Program terminated with signal SIGABRT, Aborted.
>
> warning: Section `.reg-xstate/2900464' in core file too small.
> #0 __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=140715061659456) at ./nptl/pthread_kill.c:44
> 44 ./nptl/pthread_kill.c: No such file or directory.
> (gdb) bt
> #0 __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=140715061659456) at ./nptl/pthread_kill.c:44
> #1 __pthread_kill_internal (signo=6, threadid=140715061659456) at
> ./nptl/pthread_kill.c:78
> #2 __GI___pthread_kill (threadid=140715061659456, signo=signo@entry=6) at
> ./nptl/pthread_kill.c:89
> #3 0x00007ffac6a09476 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4 0x00007ffac69ef7f3 in __GI_abort () at ./stdlib/abort.c:79
> #5 0x000055917976c830 in ExceptionalCondition (conditionName=0x5591799946f4
> "IsTransactionState()",
> fileName=0x55917999443e "catcache.c", lineNumber=1208) at assert.c:66
> #6 0x0000559179747288 in SearchCatCacheInternal (cache=0x55917acc5380,
> nkeys=1, v1=10, v2=0, v3=0, v4=0)
> at catcache.c:1208
> #7 0x0000559179747159 in SearchCatCache1 (cache=0x55917acc5380, v1=10) at
> catcache.c:1162
> #8 0x0000559179764caf in SearchSysCache1 (cacheId=11, key1=10) at
> syscache.c:825
> #9 0x000055917912ed4d in recomputeNamespacePath () at namespace.c:3834
> #10 0x000055917912e180 in GetOverrideSearchPath (context=0x55917ad3a5a0) at
> namespace.c:3409
> #11 0x00005591797521e4 in CompleteCachedPlan (plansource=0x55917ac92ae8,
> querytree_list=0x0,
> querytree_context=0x55917ad3a5a0, param_types=0x0, num_params=0,
> parserSetup=0x0, parserSetupArg=0x0,
> cursor_options=2048, fixed_result=true) at plancache.c:408
> #12 0x000055917958392c in exec_parse_message (query_string=0x55917ac68679
> ";", stmt_name=0x55917ac68678 "",
> paramTypes=0x0, numParams=0) at postgres.c:1550
> #13 0x00005591795884b3 in PostgresMain (dbname=0x55917aca0af8 "regression",
> username=0x55917ac646e8 "law")
> at postgres.c:4663
> #14 0x00005591794a94ff in BackendRun (port=0x55917ac91090) at
> postmaster.c:4461
> #15 0x00005591794a8d8b in BackendStartup (port=0x55917ac91090) at
> postmaster.c:4189
> #16 0x00005591794a50d0 in ServerLoop () at postmaster.c:1779
> #17 0x00005591794a497a in PostmasterMain (argc=3, argv=0x55917ac62620) at
> postmaster.c:1463
> #18 0x00005591793591f2 in main (argc=3, argv=0x55917ac62620) at main.c:198
>
> With "SELECT 1" instead of ";" I get:
> ERROR: current transaction is aborted, commands ignored until end of
> transaction block
> as expected.
>
> postgres.c contains:
> if (parsetree_list != NIL)
> {
> ...
> /*
> * If we are in an aborted transaction, reject all commands except
> * COMMIT/ROLLBACK. It is important that this test occur before we
> * try to do parse analysis, rewrite, or planning, since all those
> * phases try to do database accesses, which may fail in abort state.
> * (It might be safe to allow some additional utility commands in this
> * state, but not many...)
> */
> if (IsAbortedTransactionBlockState() &&
> !IsTransactionExitStmt(raw_parse_tree->stmt))
> ereport(ERROR,
> (errcode(ERRCODE_IN_FAILED_SQL_TRANSACTION),
> errmsg("current transaction is aborted, "
> "commands ignored until end of transaction block"),
> errdetail_abort()));
> ...
> }
> else
> {
> /* Empty input string. This is legal. */
> raw_parse_tree = NULL;
> psrc = CreateCachedPlan(raw_parse_tree, query_string,
> CMDTAG_UNKNOWN);
> querytree_list = NIL;
> }
> ...
> so maybe an empty input string is not legal for creating a cached plan
> when the transaction is aborted?
>
> Reproduced on REL_11_STABLE .. HEAD.
Yeah, SearchSysCache1() need in an transaction block, here is a patch
fixed it.
Another question, why should we need to create a plan cache entry for
empty input?
--
Regrads,
Japin Li.
Вложения
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
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 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))
/*
On Wed, 21 Jun 2023 at 00:32, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Japin Li <japinli@hotmail.com> writes: >> 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). Agreed, it's much better than throwing an error. LGTM. >> 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. > Thanks for your explanation! Got it. -- Regrads, Japin Li.