BUG #18059: Unexpected error 25001 in stored procedure

[ redirected to -hackers ]

PG Bug reporting form <noreply@postgresql.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.

Thoughts?

regards, tom lane

On Sat, Aug 19, 2023 at 1:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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:

That sounds like the right thing. It is perhaps unfortunate that we
don't have a proper parse analysis/execution distinction for other
types of statements, but if that ever changes then this can be
revisited.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Aug 19, 2023 at 1:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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 sounds like the right thing. It is perhaps unfortunate that we
don't have a proper parse analysis/execution distinction for other
types of statements, but if that ever changes then this can be
revisited.

I started to code this, and immediately noticed that transformStmt()
already has a companion function analyze_requires_snapshot() that
returns "true" in the cases of interest ... except that it does
not return true for T_CallStmt. Perhaps that was intentional to
begin with, but it is very hard to believe that it isn't a bug now,
since transformCallStmt can invoke nearly arbitrary processing via
transformExpr(). What semantic anomalies, if any, do we risk if CALL
processing forces a transaction start? (I rather imagine it does
already, somewhere later on...)

Anyway, I'm now of two minds whether to use analyze_requires_snapshot()
as-is for plancache.c's invalidation test, or duplicate it under a
different name, or have two names but one is just an alias for the
other. It still seems like "analyze requires snapshot" isn't
necessarily the exact inverse condition of "analyze is a no-op", but
it is today (assuming we agree that CALL needs a snapshot), and maybe
maintaining two duplicate functions is silly. Thoughts?

regards, tom lane

I wrote:

I started to code this, and immediately noticed that transformStmt()
already has a companion function analyze_requires_snapshot() that
returns "true" in the cases of interest ... except that it does
not return true for T_CallStmt. Perhaps that was intentional to
begin with, but it is very hard to believe that it isn't a bug now,
since transformCallStmt can invoke nearly arbitrary processing via
transformExpr(). What semantic anomalies, if any, do we risk if CALL
processing forces a transaction start? (I rather imagine it does
already, somewhere later on...)

I poked around some more, and determined that there should not be any
new semantic anomalies if analyze_requires_snapshot starts returning
true for CallStmts, because ExecuteCallStmt already acquires and
releases a snapshot before invoking the procedure (at least in the
non-atomic case, which is the one of interest here). I spent some
time trying to devise a test case showing it's broken, and did not
succeed: the fact that we disallow sub-SELECTs in CALL arguments makes
it a lot harder than I'd expected to reach anyplace that would require
having a transaction snapshot set. Nonetheless, I have very little
faith that such a scenario doesn't exist today, and even less that
we won't add one in future. The only real reason I can see for not
setting a snapshot here is as a micro-optimization. While that's
not without value, it seems hard to argue that CALL deserves an
optimization that SELECT doesn't get.

I also realized that ReturnStmts are likewise missing from
analyze_requires_snapshot(). This is probably unreachable, because
ReturnStmt can only appear in a SQL-language function and I can't
think of a scenario where we'd be parsing one outside a transaction.
Nonetheless it seems hard to argue that this is an optimization
we need to keep.

Hence I propose the attached patch, which invents
stmt_requires_parse_analysis() and makes analyze_requires_snapshot()
into an alias for it, so that all these statement types are treated
alike. I made the adjacent comments a lot more opinionated, too,
in hopes that future additions won't overlook these concerns.

The actual bug fix is in plancache.c. I decided to invert the tests
in plancache.c, because the macro really needed renaming anyway and
it seemed to read better this way. I also noticed that
ResetPlanCache() already tries to optimize away invalidation of
utility statements, but that logic seems no longer necessary ---
what's more, it's outright wrong for CALL, because that does need
invalidation and won't get it. (I have not tried to build a test
case proving that that's broken, but surely it is.)

Barring objections, this needs to be back-patched as far as v11.

regards, tom lane