Re: FATAL: ReleaseSavepoint: unexpected state STARTED

On Thu, Aug 18, 2011 at 3:57 AM, Marcin Mańk <marcin(dot)mank(at)gmail(dot)com> wrote:
> On Wed, Aug 17, 2011 at 11:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> =?UTF-8?B?TWFyY2luIE1hxYRr?= <marcin(dot)mank(at)gmail(dot)com> writes:
>>>  psql -c 'release q; prepare q(int) as select 1'
>>> FATAL:  ReleaseSavepoint: unexpected state STARTED
>>
>> Can't get terribly excited about that, seeing that the statement is
>> surely going to draw an error and abort processing the rest of the
>> command string in any case ...
>
> Oh, I thought FATAL was disconnectiong other sessions. Yeah, that was
> not bad enough.
>
> Here is a better one:
>
> psql postgres -c 'savepoint q; select 1'
> FATAL:  DefineSavepoint: unexpected state STARTED
> server closed the connection unexpectedly
>        This probably means the server terminated abnormally
>        before or while processing the request.

I spent some time looking at this afternoon and it appears that the
root of this problem is that we're a bit schizophrenic about whether a
multi-query command string constitutes a transaction or not. For
example, DECLARE CURSOR works fine in that context, and you can fetch
from the cursor. Since that command normally only works from within a
transaction, you might reasonably conclude "hey, we're in a
transaction". Furthermore, commands that can't be run from with a
transaction context are fenced out here, too - e.g. VACUUM. The error
message that you get there shows that someone thought about this
specific case:

[rhaas pgsql]$ psql -c 'SELECT 1;VACUUM'
ERROR: VACUUM cannot be executed from a function or multi-command string

The transaction control commands are not on the same page. BEGIN
thinks we're not in a transaction, so you can use BEGIN to start one.
If you do that then things are pretty normal. Phew. But let's say
you don't use BEGIN. ROLLBACK claims that you aren't in a transaction
and therefore you can't roll back:

[rhaas pgsql]$ psql -c 'create table xyz(); rollback'
NOTICE: there is no transaction in progress
ROLLBACK

But if there really were no transaction in progress at the point where
ROLLBACK was issued, then the rollback wouldn't do anything. In fact,
it does do something: it prevents xyz from getting created. Crazily
enough, it does this without aborting the execution of the remaining
commands:

[rhaas pgsql]$ psql -c 'create table xyz(); rollback; select 1'
NOTICE: there is no transaction in progress
?column?
----------
1
(1 row)

So, from ROLLBACK's perspective, you are sort of in a transaction, and
sort of not in a transaction.

SAVEPOINT and ROLLBACK TO SAVEPOINT are normally called after
RequireTransactionChain(), so that they can only be called from within
a transaction block. But exec_simple_query is passing down isTopLevel
= true, so RequireTransactionChain() eventually gets that value in the
mail and says "oh, good. we're in a transaction. these commands can
be allowed to work!" But then when the functions actually
implementing those commands are invoked, they view TBLOCK_STARTED as
unacceptable, and a FATAL error results.

If backward compatibility were no object, I'd be tempted to deal with
this by turning a multi-query command string into a full-fledged
transaction. But that would break things like psql -c 'BEGIN; ...do
stuff...; COMMIT; BEGIN; ...do other stuff...; COMMIT', which is
probably common enough that we shouldn't indiscriminately break it.
So I'm inclined to think that the problem - at least as far as
SAVEPOINT and ROLLBACK TO SAVEPOINT are concerned - is that
RequireTransactionChain() really can't just be the mirror image of
PreventTransactionChain(). When you're in a transaction,
PreventTransactionChain() should be unhappy; when you're outside one,
RequireTransactionChain() should be unhappy; and when you're in this
intermediate state inside of a multi-command string, BOTH of them
should be unhappy. I'm not sure whether we need to go through and
replace isTopLevel with a three-valued flag, or whether we can just
delete the isTopLevel test from RequireTransactionChain() altogether.
The latter seems like it might do the trick, but I haven't puzzled
through all the logic yet.

As for ROLLBACK, I think it should chuck an error instead of doing
this funny emit-a-warning-and-silently-arrange-for-the-transaction-to-be-aborted-later
thing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company