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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, paul(dot)kulakov(at)systematica(dot)ru, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: BUG #18059: Unexpected error 25001 in stored procedure
Date: 2023-08-23 20:53:12
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-bugs pgsql-hackers

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

Attachment Content-Type Size
v1-fix-bug-18059.patch text/x-diff 11.1 KB

In response to

Browse pgsql-bugs by date

  From Date Subject
Next Message Kyotaro Horiguchi 2023-08-24 00:45:38 Re: pg_rewind WAL segments deletion pitfall
Previous Message Euler Taveira 2023-08-23 19:26:19 Re: BUG #18067: Droping function that was used to generate column drops the column, even after `DROP EXPRESSION`

Browse pgsql-hackers by date

  From Date Subject
Next Message Nazir Bilal Yavuz 2023-08-23 20:55:15 Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc?
Previous Message Imseih (AWS), Sami 2023-08-23 20:49:29 Re: False "pg_serial": apparent wraparound” in logs