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

[ 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.

Thoughts?

regards, tom lane