Re: BUG #18059: Unexpected error 25001 in stored procedure

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: paul(dot)kulakov(at)systematica(dot)ru
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: BUG #18059: Unexpected error 25001 in stored procedure
Date: 2023-08-19 17:19:22
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-bugs pgsql-hackers

[ redirected to -hackers ]

PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
> Steps to reproduce:
> 1. Create stored procedure

> create or replace procedure test_proc()
> language plpgsql as $procedure$
> begin
> commit;
> set transaction isolation level repeatable read;
> -- here follows some useful code which is omitted for brevity
> end
> $procedure$;

> 2. Open new connection

> 3. Execute the following 3 queries one by one:
> a) call test_proc();
> b) create temporary table "#tmp"(c int) on commit drop;
> c) call test_proc();
> On step c) you'll get an error
> [25001]: ERROR: SET TRANSACTION ISOLATION LEVEL must be called before any
> query

Thanks for the report!

I looked into this. The issue is that the plancache decides it needs
to revalidate the plan for the SET command, and that causes a
transaction start (or at least acquisition of a snapshot), which then
causes check_transaction_isolation to complain. The weird sequence
that you have to go through to trigger the failure is conditioned by
the need to get the plancache entry into the needs-revalidation state
at the right time. This wasn't really a problem when the plancache
code was written, but now that we have procedures it's not good.

We could imagine trying to terminate the new transaction once we've
finished revalidating the plan, but that direction seems silly to me.
A SET command has no plan to rebuild, while for commands that do need
that, terminating and restarting the transaction adds useless overhead.
So the right fix seems to be to just do nothing. plancache.c already
knows revalidation should do nothing for TransactionStmts, but that
amount of knowledge is insufficient, as shown by this report.

One reasonable precedent is found in PlannedStmtRequiresSnapshot:
we could change plancache.c to exclude exactly the same utility
commands that does, viz

if (IsA(utilityStmt, TransactionStmt) ||
IsA(utilityStmt, LockStmt) ||
IsA(utilityStmt, VariableSetStmt) ||
IsA(utilityStmt, VariableShowStmt) ||
IsA(utilityStmt, ConstraintsSetStmt) ||
/* efficiency hacks from here down */
IsA(utilityStmt, FetchStmt) ||
IsA(utilityStmt, ListenStmt) ||
IsA(utilityStmt, NotifyStmt) ||
IsA(utilityStmt, UnlistenStmt) ||
IsA(utilityStmt, CheckPointStmt))
return false;

However, this feels unsatisfying. "Does it require a snapshot?" is not
the same question as "does it have a plan that could need rebuilding?".
The vast majority of utility statements do not have any such plan:
they are left untouched by parse analysis, rewriting, and planning.

What I'm inclined to propose, therefore, is that we make revalidation
be a no-op for every statement type for which transformStmt() reaches
its default: case. (When it does so, the resulting CMD_UTILITY Query
will not get any processing from the rewriter or planner either.)
That gives us this list of statements requiring revalidation:

case T_InsertStmt:
case T_DeleteStmt:
case T_UpdateStmt:
case T_MergeStmt:
case T_SelectStmt:
case T_ReturnStmt:
case T_PLAssignStmt:
case T_DeclareCursorStmt:
case T_ExplainStmt:
case T_CreateTableAsStmt:
case T_CallStmt:

For maintainability's sake I'd suggest writing a new function along
the line of RawStmtRequiresParseAnalysis() and putting it beside
transformStmt(), rather than allowing plancache.c to know directly
which statement types require analysis.


regards, tom lane

In response to


Browse pgsql-bugs by date

  From Date Subject
Next Message Vik Fearing 2023-08-19 19:39:12 Re: BUG #18034: Accept the spelling "+infinity" in datetime input is not accurate
Previous Message Tom Lane 2023-08-19 16:30:04 Re: BUG #18057: unaccent removes intentional spaces

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2023-08-19 18:47:33 Re: ci: Improve macos startup using a cached macports installation
Previous Message Bruce Momjian 2023-08-19 16:59:47 Re: PG 16 draft release notes ready