Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

On Sat, Nov  9, 2013 at 02:15:52PM -0500, Robert Haas wrote:
> On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > [ I'm so far behind ... ]
> >
> > Bruce Momjian <bruce@momjian.us> writes:
> >> Applied.  Thank you for all your suggestions.
> >
> > I thought the suggestion had been to issue a *warning*.  How did that
> > become an error?  This patch seems likely to break applications that
> > may have just been harmlessly sloppy about when they were issuing
> > SETs and/or what flavor of SET they use.  We don't for example throw
> > an error for START TRANSACTION with an open transaction or COMMIT or
> > ROLLBACK without one --- how can it possibly be argued that these
> > operations are more dangerous than those cases?
> >
> > I'd personally have voted for using NOTICE.
>
> Well, LOCK TABLE throws an error, so it's not without precedent.

OK, I dug into all cases where commands that are meaningless outside of
transactions currently throw an error;  they are:

    DECLARE
    LOCK
    ROLLBACK TO SAVEPOINT
    SET LOCAL*
    SET CONSTRAINTS*
    SET TRANSACTION*
    SAVEPOINT

The stared items are new in 9.4.  Here is the current behavior:

    test=> LOCK lkjasdf;
    ERROR:  LOCK TABLE can only be used in transaction blocks
    test=> SET LOCAL x = 3;
    ERROR:  SET LOCAL can only be used in transaction blocks
    test=> SAVEPOINT lkjsafd;
    ERROR:  SAVEPOINT can only be used in transaction blocks
    test=> ROLLBACK TO SAVEPOINT asdf;
    ERROR:  ROLLBACK TO SAVEPOINT can only be used in transaction blocks

Notice that they do _not_ check their arguments;  they just throw
errors.  With this patch they issue warnings and evaluate their
arguments:

    test=> LOCK lkjasdf;
    WARNING:  LOCK TABLE can only be used in transaction blocks
    ERROR:  relation "lkjasdf" does not exist
    test=> SET LOCAL x = 3;
    WARNING:  SET LOCAL can only be used in transaction blocks
    ERROR:  unrecognized configuration parameter "x"
    test=> SAVEPOINT lkjsafd;
    WARNING:  SAVEPOINT can only be used in transaction blocks
    SAVEPOINT
    test=> ROLLBACK TO SAVEPOINT asdf;
    WARNING:  ROLLBACK TO SAVEPOINT can only be used in transaction blocks
    ROLLBACK

However, SAVEPOINT/ROLLBACK throw weird errors when they are evaluated
outside a multi-statement transaction, so their arguments are not
evaluated.  This might be why they were originally marked as errors.

A patch to issue only warnings is attached.  In a way this change
improves the code by throwing errors only when the commands are invalid,
rather than just useless.  You could argue that ROLLBACK TO SAVEPOINT
should throw an error because no savepoint name is valid in that
context.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +