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

I wrote:
> My thought is that what we need to do is find a way for isTopLevel
> to be false if we're processing a multi-command string.

Nah, that's backwards, the problem is exactly that isTopLevel is
false if we're processing a multi-command string. That allows
DefineSavepoint to think that it's inside a function, and we don't
disallow savepoints inside functions. (Or at least, xact.c doesn't
enforce any such prohibition; it's up to spi.c and the individual PLs
to decide if they could support that.)

After contemplating my navel for awhile, I think that this case proves
that the quick hack embodied in commit 4f896dac1 is inadequate. Rather
than piling another quick hack on top and hoping that the result is OK,
I think it's time to bite the bullet and represent the behavior we want
explicitly in the transaction machinery. Accordingly, PFA a patch
that invents a notion of an "implicit" transaction block.

I also added a bunch of test cases exercising the behavior. Except
for the problem of FATAL exits for savepoint commands, all these
cases work exactly like they do in unpatched code. However, now that
we have an explicit representation, it'd be easy to tweak the behavior
if we want to. For instance, I'm not entirely sure whether we want
the behavior that COMMIT and ROLLBACK in this state print warnings.
Good luck changing that before; but now it'd be a straightforward
adjustment.

I'm inclined to complete the reversion of 4f896dac1 by also undoing
its error message text change in PreventTransactionChain,

- errmsg("%s cannot be executed from a function", stmtType)));
+ errmsg("%s cannot be executed from a function or multi-command string",
+ stmtType)));

but this patch doesn't include that change.

My feeling about this is that we don't need a back-patch. Throwing
FATAL rather than ERROR for a misplaced savepoint command is a bit
unpleasant, but it doesn't break other sessions, and the upshot is
really the same: don't do that.

regards, tom lane