Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

Michael Paquier <michael.paquier@gmail.com> writes:
> Hmm. While this patch looks to me in a better shape than what Simon's
> is proposing, thinking about
> CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw@mail.gmail.com
> which involved a migration Oracle->Postgres, I have been wondering if
> it is possible to still allow savepoints in those cases to ease the
> pain and surprise of some users.

I don't want to go there, and was thinking we should expand the new
comment in DefineSavepoint to explain why not.  It's certainly not that
much additional work to allow a savepoint so far as xact.c is concerned,
as your patch shows.  The problem is that intra-string savepoints seem
inconsistent with exec_simple_query's behavior of abandoning the whole
query string upon error.  If you do

insert ...\; savepoint\; insert ...\; release savepoint\; insert ...;

wouldn't you sort of expect that the savepoint commands mean to keep going
if the second insert fails?  If they don't mean that, what do they mean?

Also, the main thing that we need xact.c's involvement for in the first
place is the fact that implicit transaction blocks, unlike regular ones,
auto-cancel on an error, leaving you outside a block not inside a failed
one.  So I don't exactly see how savepoints would fit into that.

Now I do not think we can change exec_simple_query's behavior without big
compatibility problems --- to the extent that there's a justifiable
use-case for multi-query strings at all, a big part of it is the implied
"do B only if A succeeds" semantics.  But if that's what happens, then
having savepoint commands in the string is just a can of worms from both
definitional and practical points of view.  If an error happens, did it
happen before or after the savepoint, and what state is the session left
in?  You can't easily tell because of the lack of reporting about
savepoint state.  Right now, the only real issue after a failure is "are
we in a transaction block or not", which the server does return enough
info to distinguish.

Now admittedly, the same set of issues pops up if one uses an
explicit transaction block in a multi-query string:

begin\; insert ...\; savepoint\; insert ...\; release savepoint\; insert ...\; commit;

If one of the inserts fails, you don't really know which one unless you
were counting command-complete replies (which PQexec doesn't let you do).
But that behavior was there already, we aren't proposing to make it worse.
(I think this approach is also the correct workaround to give those
Oracle-conversion folk: their real problem is failure to convert from
Oracle's implicit-BEGIN behavior to our explicit-BEGIN.)

In short, -1 for relaxing the prohibition on SAVEPOINT.
        regards, tom lane