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

On Mon, Sep 4, 2017 at 7:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
> On further consideration, I think the control logic I added in
> exec_simple_query() is a shade bogus. I set it up to only force
> an implicit transaction block when there are at least two statements
> remaining to execute. However, that has the result of allowing, eg,
>
> begin\; select 1\; commit\; vacuum;
>
> Now in principle it's perfectly OK to allow that, since the vacuum
> is alone in its transaction. But it feels more like an implementation
> artifact than a good design. The existing code doesn't allow it,
> and we might have a hard time duplicating this behavior if we ever
> significantly rewrote the transaction infrastructure. Plus I'd hate
> to have to explain it to users. I think we'd be better off enforcing
> transaction block restrictions on every statement in a multi-command
> string, regardless of the location of any COMMIT/ROLLBACK within the
> string.
>
> Hence, attached a v2 that does it like that. I also fully reverted
> 4f896dac1 by undoing its changes to PreventTransactionChain; other
> than that, the changes in xact.c are the same as before.

Hmm. While this patch looks to me in a better shape than what Simon's
is proposing, thinking about
CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw(at)mail(dot)gmail(dot)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. And while looking around, it seems to
me that it is possible. Please find the attached to show my idea,
based on Tom's v2. The use of a new transaction state like
IMPLICIT_INPROGRESS is something that I got in mind upthread, but I
have not shaped that into a fully-blown patch.

All the following sequences are working as I would think they should
(a couple of inserts done within each savepoint allowed me to check
that the transactions happened correctly, though the set of
regressions presented in v2 looks enough):
BEGIN; SELECT 1; SAVEPOINT sp; RELEASE sp; SAVEPOINT sp; ROLLBACK TO
SAVEPOINT sp; COMMIT;
BEGIN; SELECT 1; SAVEPOINT sp; RELEASE sp; SAVEPOINT sp; ROLLBACK TO
SAVEPOINT sp; ROLLBACK;
SELECT 1; SAVEPOINT sp; RELEASE sp; SAVEPOINT sp; ROLLBACK TO SAVEPOINT sp;
So sequences of multiple commands are working with the patch attached
even if a BEGIN is not explicitly added. On HEAD or with v2, if BEGIN
is not specified, savepoint commands cause a failure.
--
Michael