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

2013-11-28 Thread Robert Haas
On Wed, Nov 27, 2013 at 4:44 PM, Bruce Momjian br...@momjian.us wrote:
 Seems broadly reasonable, but I'd use no other effect throughout.

 That sounds awkward, e.g.:

  Issuing commandROLLBACK/ outside of a transaction
  block emits a warning but has no other effect.

 I could live with this:

  Issuing commandROLLBACK/ outside of a transaction
  block has no effect except emitting a warning.

I prefer the first version, but that's obviously a judgement call.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-28 Thread Bruce Momjian
On Thu, Nov 28, 2013 at 04:51:14PM -0500, Robert Haas wrote:
 On Wed, Nov 27, 2013 at 4:44 PM, Bruce Momjian br...@momjian.us wrote:
  Seems broadly reasonable, but I'd use no other effect throughout.
 
  That sounds awkward, e.g.:
 
   Issuing commandROLLBACK/ outside of a transaction
   block emits a warning but has no other effect.
 
  I could live with this:
 
   Issuing commandROLLBACK/ outside of a transaction
   block has no effect except emitting a warning.
 
 I prefer the first version, but that's obviously a judgement call.

How about:

 Issuing commandROLLBACK/ outside of a transaction
 block has the sole effect of emitting a warning.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-28 Thread Robert Haas
On Nov 28, 2013, at 5:18 PM, Bruce Momjian br...@momjian.us wrote:
 On Thu, Nov 28, 2013 at 04:51:14PM -0500, Robert Haas wrote:
 On Wed, Nov 27, 2013 at 4:44 PM, Bruce Momjian br...@momjian.us wrote:
 Seems broadly reasonable, but I'd use no other effect throughout.
 
 That sounds awkward, e.g.:
 
 Issuing commandROLLBACK/ outside of a transaction
 block emits a warning but has no other effect.
 
 I could live with this:
 
 Issuing commandROLLBACK/ outside of a transaction
 block has no effect except emitting a warning.
 
 I prefer the first version, but that's obviously a judgement call.
 
 How about:
 
 Issuing commandROLLBACK/ outside of a transaction
 block has the sole effect of emitting a warning.

Sure, that sounds OK.

...Robert


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-27 Thread Robert Haas
On Tue, Nov 26, 2013 at 5:02 PM, Bruce Momjian br...@momjian.us wrote:
 On Tue, Nov 26, 2013 at 01:58:04PM -0300, Alvaro Herrera wrote:
 Bruce Momjian escribió:
  On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:

Uh, I ended up mentioning no effect to highlight it does nothing,
rather than mention a warning.  Would people prefer I say warning?  
Or
should I say issues a warning because it has no effect or something?
It is easy to change.
  
   I'd revert the change Robert highlights above.  ISTM you've changed the
   code to match the documentation; why would you then change the docs?
 
  Well, I did it to make it consistent.  The question is what to write for
  _all_ of the new warnings, including SET.  Do we say warning, do we
  say it has no effect, or do we say both?  The ABORT is a just one case
  of that.

 Maybe it emits a warning and otherwise has no effect?  Emitting a
 warning is certainly not doing nothing; as has been pointed out in the
 SSL renegotiation thread, it might cause the log to fill disk.

 OK, doc patch attached.

Seems broadly reasonable, but I'd use no other effect throughout.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-27 Thread Bruce Momjian
On Wed, Nov 27, 2013 at 03:59:31PM -0500, Robert Haas wrote:
 On Tue, Nov 26, 2013 at 5:02 PM, Bruce Momjian br...@momjian.us wrote:
  On Tue, Nov 26, 2013 at 01:58:04PM -0300, Alvaro Herrera wrote:
  Bruce Momjian escribió:
   On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:
 
 Uh, I ended up mentioning no effect to highlight it does nothing,
 rather than mention a warning.  Would people prefer I say warning? 
  Or
 should I say issues a warning because it has no effect or 
 something?
 It is easy to change.
   
I'd revert the change Robert highlights above.  ISTM you've changed the
code to match the documentation; why would you then change the docs?
  
   Well, I did it to make it consistent.  The question is what to write for
   _all_ of the new warnings, including SET.  Do we say warning, do we
   say it has no effect, or do we say both?  The ABORT is a just one case
   of that.
 
  Maybe it emits a warning and otherwise has no effect?  Emitting a
  warning is certainly not doing nothing; as has been pointed out in the
  SSL renegotiation thread, it might cause the log to fill disk.
 
  OK, doc patch attached.
 
 Seems broadly reasonable, but I'd use no other effect throughout.

That sounds awkward, e.g.:

 Issuing commandROLLBACK/ outside of a transaction
 block emits a warning but has no other effect.

I could live with this:

 Issuing commandROLLBACK/ outside of a transaction
 block has no effect except emitting a warning.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-27 Thread Bruce Momjian
On Wed, Nov 27, 2013 at 04:44:02PM -0500, Bruce Momjian wrote:
 I could live with this:
 
  Issuing commandROLLBACK/ outside of a transaction
  block has no effect except emitting a warning.

Proposed doc patch attached.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/abort.sgml b/doc/src/sgml/ref/abort.sgml
new file mode 100644
index f3a2fa8..ce70e7f
*** a/doc/src/sgml/ref/abort.sgml
--- b/doc/src/sgml/ref/abort.sgml
*** ABORT [ WORK | TRANSACTION ]
*** 63,69 
/para
  
para
!Issuing commandABORT/ outside of a transaction block has no effect.
/para
   /refsect1
  
--- 63,70 
/para
  
para
!Issuing commandABORT/ outside of a transaction block
!has no effect except emitting a warning.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/rollback.sgml b/doc/src/sgml/ref/rollback.sgml
new file mode 100644
index 4f79621..3465d51
*** a/doc/src/sgml/ref/rollback.sgml
--- b/doc/src/sgml/ref/rollback.sgml
*** ROLLBACK [ WORK | TRANSACTION ]
*** 60,66 
  
para
 Issuing commandROLLBACK/ outside of a transaction
!block has no effect.
/para
   /refsect1
  
--- 60,66 
  
para
 Issuing commandROLLBACK/ outside of a transaction
!block has no effect except emitting a warning.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 5a84f69..6ef06e6
*** a/doc/src/sgml/ref/set.sgml
--- b/doc/src/sgml/ref/set.sgml
*** SET [ SESSION | LOCAL ] TIME ZONE { rep
*** 110,117 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.  This has no effect
!   outside of a transaction block.
   /para
  /listitem
 /varlistentry
--- 110,117 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.  Issuing this
!   outside of a transaction block has no effect except emitting a warning.
   /para
  /listitem
 /varlistentry
diff --git a/doc/src/sgml/ref/set_constraints.sgml b/doc/src/sgml/ref/set_constraints.sgml
new file mode 100644
index a33190c..541a50b
*** a/doc/src/sgml/ref/set_constraints.sgml
--- b/doc/src/sgml/ref/set_constraints.sgml
*** SET CONSTRAINTS { ALL | replaceable cla
*** 99,105 
  
para
 This command only alters the behavior of constraints within the
!current transaction.  This has no effect outside of a transaction block.
/para
   /refsect1
  
--- 99,106 
  
para
 This command only alters the behavior of constraints within the
!current transaction.  Issuing this outside of a transaction block
!has no effect except emitting a warning.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/set_transaction.sgml b/doc/src/sgml/ref/set_transaction.sgml
new file mode 100644
index e90ff4a..1597657
*** a/doc/src/sgml/ref/set_transaction.sgml
--- b/doc/src/sgml/ref/set_transaction.sgml
*** SET SESSION CHARACTERISTICS AS TRANSACTI
*** 185,191 
para
 If commandSET TRANSACTION/command is executed without a prior
 commandSTART TRANSACTION/command or commandBEGIN/command,
!it will have no effect.
/para
  
para
--- 185,191 
para
 If commandSET TRANSACTION/command is executed without a prior
 commandSTART TRANSACTION/command or commandBEGIN/command,
!ithas no effect except emitting a warning.
/para
  
para

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-26 Thread Bruce Momjian
On Mon, Nov 25, 2013 at 10:12:43PM -0500, Bruce Momjian wrote:
  Those things are not the same.
 
 Uh, I ended up mentioning no effect to highlight it does nothing,
 rather than mention a warning.  Would people prefer I say warning?  Or
 should I say issues a warning because it has no effect or something? 
 It is easy to change.

I should also point out that in 9.3, ABORT outside of a transaction was
documented as issuing a warning, but issued a notice.  git head now
issues a warning.  That might be part of the confusion.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-26 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 On Mon, Nov 25, 2013 at 10:04:19PM -0500, Robert Haas wrote:
 But the documentation says:
 
 -   Issuing commandABORT/ when not inside a transaction does
 -   no harm, but it will provoke a warning message.
 +   Issuing commandABORT/ outside of a transaction block has no effect.
 
 Those things are not the same.

 Uh, I ended up mentioning no effect to highlight it does nothing,
 rather than mention a warning.  Would people prefer I say warning?  Or
 should I say issues a warning because it has no effect or something? 
 It is easy to change.

I'd revert the change Robert highlights above.  ISTM you've changed the
code to match the documentation; why would you then change the docs?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-26 Thread Bruce Momjian
On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  On Mon, Nov 25, 2013 at 10:04:19PM -0500, Robert Haas wrote:
  But the documentation says:
  
  -   Issuing commandABORT/ when not inside a transaction does
  -   no harm, but it will provoke a warning message.
  +   Issuing commandABORT/ outside of a transaction block has no effect.
  
  Those things are not the same.
 
  Uh, I ended up mentioning no effect to highlight it does nothing,
  rather than mention a warning.  Would people prefer I say warning?  Or
  should I say issues a warning because it has no effect or something? 
  It is easy to change.
 
 I'd revert the change Robert highlights above.  ISTM you've changed the
 code to match the documentation; why would you then change the docs?

Well, I did it to make it consistent.  The question is what to write for
_all_ of the new warnings, including SET.  Do we say warning, do we
say it has no effect, or do we say both?  The ABORT is a just one case
of that.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-26 Thread Alvaro Herrera
Bruce Momjian escribió:
 On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:

   Uh, I ended up mentioning no effect to highlight it does nothing,
   rather than mention a warning.  Would people prefer I say warning?  Or
   should I say issues a warning because it has no effect or something? 
   It is easy to change.
  
  I'd revert the change Robert highlights above.  ISTM you've changed the
  code to match the documentation; why would you then change the docs?
 
 Well, I did it to make it consistent.  The question is what to write for
 _all_ of the new warnings, including SET.  Do we say warning, do we
 say it has no effect, or do we say both?  The ABORT is a just one case
 of that.

Maybe it emits a warning and otherwise has no effect?  Emitting a
warning is certainly not doing nothing; as has been pointed out in the
SSL renegotiation thread, it might cause the log to fill disk.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-26 Thread Bruce Momjian
On Tue, Nov 26, 2013 at 01:58:04PM -0300, Alvaro Herrera wrote:
 Bruce Momjian escribió:
  On Tue, Nov 26, 2013 at 11:22:39AM -0500, Tom Lane wrote:
 
Uh, I ended up mentioning no effect to highlight it does nothing,
rather than mention a warning.  Would people prefer I say warning?  Or
should I say issues a warning because it has no effect or something? 
It is easy to change.
   
   I'd revert the change Robert highlights above.  ISTM you've changed the
   code to match the documentation; why would you then change the docs?
  
  Well, I did it to make it consistent.  The question is what to write for
  _all_ of the new warnings, including SET.  Do we say warning, do we
  say it has no effect, or do we say both?  The ABORT is a just one case
  of that.
 
 Maybe it emits a warning and otherwise has no effect?  Emitting a
 warning is certainly not doing nothing; as has been pointed out in the
 SSL renegotiation thread, it might cause the log to fill disk.

OK, doc patch attached.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/abort.sgml b/doc/src/sgml/ref/abort.sgml
new file mode 100644
index f3a2fa8..7c503c6
*** a/doc/src/sgml/ref/abort.sgml
--- b/doc/src/sgml/ref/abort.sgml
*** ABORT [ WORK | TRANSACTION ]
*** 63,69 
/para
  
para
!Issuing commandABORT/ outside of a transaction block has no effect.
/para
   /refsect1
  
--- 63,70 
/para
  
para
!Issuing commandABORT/ outside of a transaction block emits a
!warning and has no effect.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/rollback.sgml b/doc/src/sgml/ref/rollback.sgml
new file mode 100644
index 4f79621..31b8762
*** a/doc/src/sgml/ref/rollback.sgml
--- b/doc/src/sgml/ref/rollback.sgml
*** ROLLBACK [ WORK | TRANSACTION ]
*** 60,66 
  
para
 Issuing commandROLLBACK/ outside of a transaction
!block has no effect.
/para
   /refsect1
  
--- 60,66 
  
para
 Issuing commandROLLBACK/ outside of a transaction
!block emits a warning and has no effect.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 5a84f69..58089e6
*** a/doc/src/sgml/ref/set.sgml
--- b/doc/src/sgml/ref/set.sgml
*** SET [ SESSION | LOCAL ] TIME ZONE { rep
*** 110,117 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.  This has no effect
!   outside of a transaction block.
   /para
  /listitem
 /varlistentry
--- 110,117 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.  Issuing this
!   outside of a transaction block emits a warning and has no effect.
   /para
  /listitem
 /varlistentry
diff --git a/doc/src/sgml/ref/set_constraints.sgml b/doc/src/sgml/ref/set_constraints.sgml
new file mode 100644
index a33190c..3a080ad
*** a/doc/src/sgml/ref/set_constraints.sgml
--- b/doc/src/sgml/ref/set_constraints.sgml
*** SET CONSTRAINTS { ALL | replaceable cla
*** 99,105 
  
para
 This command only alters the behavior of constraints within the
!current transaction.  This has no effect outside of a transaction block.
/para
   /refsect1
  
--- 99,106 
  
para
 This command only alters the behavior of constraints within the
!current transaction.  Issuing this outside of a transaction block
!emits a warning and has no effect.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/set_transaction.sgml b/doc/src/sgml/ref/set_transaction.sgml
new file mode 100644
index e90ff4a..2c5bf33
*** a/doc/src/sgml/ref/set_transaction.sgml
--- b/doc/src/sgml/ref/set_transaction.sgml
*** SET SESSION CHARACTERISTICS AS TRANSACTI
*** 185,191 
para
 If commandSET TRANSACTION/command is executed without a prior
 commandSTART TRANSACTION/command or commandBEGIN/command,
!it will have no effect.
/para
  
para
--- 185,191 
para
 If commandSET TRANSACTION/command is executed without a prior
 commandSTART TRANSACTION/command or commandBEGIN/command,
!it will emit a warning and have no effect.
/para
  
para

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-25 Thread Bruce Momjian
On Fri, Nov 22, 2013 at 01:19:55PM -0500, Bruce Momjian wrote:
 On Fri, Nov 22, 2013 at 12:17:41PM -0500, Bruce Momjian wrote:
  Good points.  I have modified the attached patch to do as you suggested.
 
 Also, I have read through the thread and summarized the positions of the
 posters:
 
   9.3 WARNING ERROR
   SET noneTom, DavidJ, AndresFRobert, Kevin
   SAVEPOINT   error   Tom, DavidJ, 
 Robert, AndresF, Kevin
   LOCK, DECLARE   error   Tom, DavidJ, 
 Robert, AndresF, Kevin
 
 Everyone seems to agree that SAVEPOINT, LOCK, and DECLARE should remain
 as errors.  Everyone also seems to agree that BEGIN and COMMIT should
 remain warnings, and ABORT should be changed from notice to warning.
 
 Our only disagreement seems to be how to handle the SET commands, which
 used to report nothing.  Would anyone else like to correct or express an
 opinion?  Given the current vote count and backward-compatibility,
 warning seems to be the direction we are heading.

Patch applied.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-25 Thread Robert Haas
On Mon, Nov 25, 2013 at 7:19 PM, Bruce Momjian br...@momjian.us wrote:
 On Fri, Nov 22, 2013 at 01:19:55PM -0500, Bruce Momjian wrote:
 On Fri, Nov 22, 2013 at 12:17:41PM -0500, Bruce Momjian wrote:
  Good points.  I have modified the attached patch to do as you suggested.

 Also, I have read through the thread and summarized the positions of the
 posters:

   9.3 WARNING ERROR
   SET noneTom, DavidJ, AndresFRobert, Kevin
   SAVEPOINT   error   Tom, DavidJ, 
 Robert, AndresF, Kevin
   LOCK, DECLARE   error   Tom, DavidJ, 
 Robert, AndresF, Kevin

 Everyone seems to agree that SAVEPOINT, LOCK, and DECLARE should remain
 as errors.  Everyone also seems to agree that BEGIN and COMMIT should
 remain warnings, and ABORT should be changed from notice to warning.

 Our only disagreement seems to be how to handle the SET commands, which
 used to report nothing.  Would anyone else like to correct or express an
 opinion?  Given the current vote count and backward-compatibility,
 warning seems to be the direction we are heading.

 Patch applied.

I must be missing something.  The commit message for this patch says:

Also change ABORT outside of a transaction block from notice to
warning.

But the documentation says:

-   Issuing commandABORT/ when not inside a transaction does
-   no harm, but it will provoke a warning message.
+   Issuing commandABORT/ outside of a transaction block has no effect.

Those things are not the same.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-25 Thread Bruce Momjian
On Mon, Nov 25, 2013 at 10:04:19PM -0500, Robert Haas wrote:
 On Mon, Nov 25, 2013 at 7:19 PM, Bruce Momjian br...@momjian.us wrote:
  On Fri, Nov 22, 2013 at 01:19:55PM -0500, Bruce Momjian wrote:
  On Fri, Nov 22, 2013 at 12:17:41PM -0500, Bruce Momjian wrote:
   Good points.  I have modified the attached patch to do as you suggested.
 
  Also, I have read through the thread and summarized the positions of the
  posters:
 
9.3 WARNING ERROR
SET noneTom, DavidJ, AndresFRobert, Kevin
SAVEPOINT   error   Tom, DavidJ, 
  Robert, AndresF, Kevin
LOCK, DECLARE   error   Tom, DavidJ, 
  Robert, AndresF, Kevin
 
  Everyone seems to agree that SAVEPOINT, LOCK, and DECLARE should remain
  as errors.  Everyone also seems to agree that BEGIN and COMMIT should
  remain warnings, and ABORT should be changed from notice to warning.
 
  Our only disagreement seems to be how to handle the SET commands, which
  used to report nothing.  Would anyone else like to correct or express an
  opinion?  Given the current vote count and backward-compatibility,
  warning seems to be the direction we are heading.
 
  Patch applied.
 
 I must be missing something.  The commit message for this patch says:
 
 Also change ABORT outside of a transaction block from notice to
 warning.
 
 But the documentation says:
 
 -   Issuing commandABORT/ when not inside a transaction does
 -   no harm, but it will provoke a warning message.
 +   Issuing commandABORT/ outside of a transaction block has no effect.
 
 Those things are not the same.

Uh, I ended up mentioning no effect to highlight it does nothing,
rather than mention a warning.  Would people prefer I say warning?  Or
should I say issues a warning because it has no effect or something? 
It is easy to change.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-22 Thread Alvaro Herrera
Bruce Momjian escribió:

 OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
 from ERROR (which is new in 9.4) to WARNING.

I don't like that this patch changes RequireTransactionChain() from
actually requiring one, to a function that maybe requires a transaction
chain, and maybe it only complains about there not being one.  I mean,
it's like you had named the new throwError boolean as notReally or
something like that.  Also, the new comment paragraph is bad because it
explains who must pass true/false, instead of what's the effect of each
value (and let the callers choose which value to pass).

I would create a separate function to implement this, maybe
WarnUnlessInTransactionBlock() or something like that.  That would make
the patch a good deal smaller (because not changing existing callers).

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-22 Thread Bruce Momjian
On Fri, Nov 22, 2013 at 10:24:35AM -0300, Alvaro Herrera wrote:
 Bruce Momjian escribió:
 
  OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
  from ERROR (which is new in 9.4) to WARNING.
 
 I don't like that this patch changes RequireTransactionChain() from
 actually requiring one, to a function that maybe requires a transaction
 chain, and maybe it only complains about there not being one.  I mean,
 it's like you had named the new throwError boolean as notReally or
 something like that.  Also, the new comment paragraph is bad because it
 explains who must pass true/false, instead of what's the effect of each
 value (and let the callers choose which value to pass).
 
 I would create a separate function to implement this, maybe
 WarnUnlessInTransactionBlock() or something like that.  That would make
 the patch a good deal smaller (because not changing existing callers).

Good points.  I have modified the attached patch to do as you suggested.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/abort.sgml b/doc/src/sgml/ref/abort.sgml
new file mode 100644
index 246e8f8..f3a2fa8
*** a/doc/src/sgml/ref/abort.sgml
--- b/doc/src/sgml/ref/abort.sgml
*** ABORT [ WORK | TRANSACTION ]
*** 63,70 
/para
  
para
!Issuing commandABORT/ when not inside a transaction does
!no harm, but it will provoke a warning message.
/para
   /refsect1
  
--- 63,69 
/para
  
para
!Issuing commandABORT/ outside of a transaction block has no effect.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/rollback.sgml b/doc/src/sgml/ref/rollback.sgml
new file mode 100644
index b265545..4f79621
*** a/doc/src/sgml/ref/rollback.sgml
--- b/doc/src/sgml/ref/rollback.sgml
*** ROLLBACK [ WORK | TRANSACTION ]
*** 59,66 
/para
  
para
!Issuing commandROLLBACK/ when not inside a transaction does
!no harm, but it will provoke a warning message.
/para
   /refsect1
  
--- 59,66 
/para
  
para
!Issuing commandROLLBACK/ outside of a transaction
!block has no effect.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 6290c9d..5a84f69
*** a/doc/src/sgml/ref/set.sgml
--- b/doc/src/sgml/ref/set.sgml
*** SET [ SESSION | LOCAL ] TIME ZONE { rep
*** 110,118 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.
!   productnamePostgreSQL/productname reports an error if
!   commandSET LOCAL/ is used outside a transaction block.
   /para
  /listitem
 /varlistentry
--- 110,117 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.  This has no effect
!   outside of a transaction block.
   /para
  /listitem
 /varlistentry
diff --git a/doc/src/sgml/ref/set_constraints.sgml b/doc/src/sgml/ref/set_constraints.sgml
new file mode 100644
index 895a5fd..a33190c
*** a/doc/src/sgml/ref/set_constraints.sgml
--- b/doc/src/sgml/ref/set_constraints.sgml
*** SET CONSTRAINTS { ALL | replaceable cla
*** 99,108 
  
para
 This command only alters the behavior of constraints within the
!current transaction. Thus, if you execute this command outside of a
!transaction block
!(commandBEGIN/command/commandCOMMIT/command pair), it will
!generate an error.
/para
   /refsect1
  
--- 99,105 
  
para
 This command only alters the behavior of constraints within the
!current transaction.  This has no effect outside of a transaction block.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/set_transaction.sgml b/doc/src/sgml/ref/set_transaction.sgml
new file mode 100644
index 391464a..e90ff4a
*** a/doc/src/sgml/ref/set_transaction.sgml
--- b/doc/src/sgml/ref/set_transaction.sgml
*** SET SESSION CHARACTERISTICS AS TRANSACTI
*** 185,191 
para
 If commandSET TRANSACTION/command is executed without a prior
 commandSTART TRANSACTION/command or commandBEGIN/command,
!it will generate an error.
/para
  
para
--- 185,191 
para
 If commandSET TRANSACTION/command is executed without a prior
 commandSTART TRANSACTION/command or commandBEGIN/command,
!it will have no effect.
/para
  
para
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
new file mode 100644
index 0591f3f..bab048d
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*** static void CallSubXactCallbacks(SubXact
*** 265,270 
--- 265,272 
  	 

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

2013-11-22 Thread Bruce Momjian
On Fri, Nov 22, 2013 at 12:17:41PM -0500, Bruce Momjian wrote:
 Good points.  I have modified the attached patch to do as you suggested.

Also, I have read through the thread and summarized the positions of the
posters:

  9.3 WARNING ERROR
  SET noneTom, DavidJ, AndresFRobert, Kevin
  SAVEPOINT   error   Tom, DavidJ, Robert, 
AndresF, Kevin
  LOCK, DECLARE   error   Tom, DavidJ, Robert, 
AndresF, Kevin

Everyone seems to agree that SAVEPOINT, LOCK, and DECLARE should remain
as errors.  Everyone also seems to agree that BEGIN and COMMIT should
remain warnings, and ABORT should be changed from notice to warning.

Our only disagreement seems to be how to handle the SET commands, which
used to report nothing.  Would anyone else like to correct or express an
opinion?  Given the current vote count and backward-compatibility,
warning seems to be the direction we are heading.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-20 Thread Bruce Momjian
On Tue, Nov 19, 2013 at 10:21:47PM -0500, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  Does anyone know if this C comment justifies why ABORT is a NOTICE and
  not WARNING?
 
  /*
   * The user issued ABORT when not inside a transaction. Issue a
   * NOTICE and go to abort state.  The upcoming call to
   * CommitTransactionCommand() will then put us back into the
   * default state.
   */
 
 It's just describing the implementation, not defending the design choice.
 
 My personal standpoint is that I don't care much whether these messages
 are NOTICE or WARNING.  What I'm not happy about is promoting cases that
 have been non-error conditions for years into ERRORs.

I don't remember any cases where that was suggested.

 Now, if we wanted to go the other way (downgrade some ERRORs to WARNINGs),
 that would not create an application compatibility problem in my view.

Yes, that was my initial plan, but many didn't like it.

 And on the third hand, there's Emerson's excellent advice: A foolish
 consistency is the hobgoblin of little minds.  I'm not convinced that
 trying to make all these cases have the same message level is actually
 a good goal.  It seems entirely plausible that some are more dangerous
 than others.

The attached patch changes ABORT from NOTICE to WARNING, and documents
that all other are errors.  This top-level logic idea came from Robert
Haas, and it has some level of consistency.

Interesting that ABORT was already documented as returning a warning:

   Issuing commandABORT/ when not inside a transaction does
   no harm, but it will provoke a warning message.
  ---

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
new file mode 100644
index 0591f3f..495684e
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*** PreventTransactionChain(bool isTopLevel,
*** 2961,2966 
--- 2961,2969 
   *	use of the current statement's results.  Likewise subtransactions.
   *	Thus this is an inverse for PreventTransactionChain.
   *
+  *	While top-level transaction control commands (BEGIN/COMMIT/ABORT)
+  *	outside of transactions issue warnings, these generate errors.
+  *
   *	isTopLevel: passed down from ProcessUtility to determine whether we are
   *	inside a function.
   *	stmtType: statement type name, for error messages.
*** UserAbortTransactionBlock(void)
*** 3425,3436 
  
  			/*
  			 * The user issued ABORT when not inside a transaction. Issue a
! 			 * NOTICE and go to abort state.  The upcoming call to
  			 * CommitTransactionCommand() will then put us back into the
  			 * default state.
  			 */
  		case TBLOCK_STARTED:
! 			ereport(NOTICE,
  	(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
  	 errmsg(there is no transaction in progress)));
  			s-blockState = TBLOCK_ABORT_PENDING;
--- 3428,3439 
  
  			/*
  			 * The user issued ABORT when not inside a transaction. Issue a
! 			 * WARNING and go to abort state.  The upcoming call to
  			 * CommitTransactionCommand() will then put us back into the
  			 * default state.
  			 */
  		case TBLOCK_STARTED:
! 			ereport(WARNING,
  	(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
  	 errmsg(there is no transaction in progress)));
  			s-blockState = TBLOCK_ABORT_PENDING;
diff --git a/src/test/regress/expected/errors.out b/src/test/regress/expected/errors.out
new file mode 100644
index fa0bd82..4061512
*** a/src/test/regress/expected/errors.out
--- b/src/test/regress/expected/errors.out
*** ERROR:  column name oid conflicts with
*** 114,120 
  -- TRANSACTION STUFF
  -- not in a xact
  abort;
! NOTICE:  there is no transaction in progress
  -- not in a xact
  end;
  WARNING:  there is no transaction in progress
--- 114,120 
  -- TRANSACTION STUFF
  -- not in a xact
  abort;
! WARNING:  there is no transaction in progress
  -- not in a xact
  end;
  WARNING:  there is no transaction in progress

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-20 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 On Tue, Nov 19, 2013 at 10:21:47PM -0500, Tom Lane wrote:
 My personal standpoint is that I don't care much whether these messages
 are NOTICE or WARNING.  What I'm not happy about is promoting cases that
 have been non-error conditions for years into ERRORs.

 I don't remember any cases where that was suggested.

Apparently you've forgotten the commit that was the subject of this
thread.  You took a bunch of SET cases that we've always accepted
without any complaint whatsoever, and made them into ERRORs, thereby
breaking any applications that might've expected such usage to be
harmless.  I would be okay if that patch had issued WARNINGs, which
as you can see from the thread title was the original suggestion.

 The attached patch changes ABORT from NOTICE to WARNING, and documents
 that all other are errors.  This top-level logic idea came from Robert
 Haas, and it has some level of consistency.

This patch utterly fails to address my complaint.

More to the point, I think it's a waste of time to make these sorts of
adjustments.  The only thanks you're likely to get for it is complaints
about cross-version behavioral changes.  Also, you're totally ignoring
the thought that these different message levels might've been selected
intentionally, back when the code was written.  Since there have been
no field complaints about the inconsistency, what's your hurry to
change it?  See Emerson.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-20 Thread Bruce Momjian
On Wed, Nov 20, 2013 at 10:04:22AM -0500, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  On Tue, Nov 19, 2013 at 10:21:47PM -0500, Tom Lane wrote:
  My personal standpoint is that I don't care much whether these messages
  are NOTICE or WARNING.  What I'm not happy about is promoting cases that
  have been non-error conditions for years into ERRORs.
 
  I don't remember any cases where that was suggested.
 
 Apparently you've forgotten the commit that was the subject of this
 thread.  You took a bunch of SET cases that we've always accepted
 without any complaint whatsoever, and made them into ERRORs, thereby
 breaking any applications that might've expected such usage to be
 harmless.  I would be okay if that patch had issued WARNINGs, which
 as you can see from the thread title was the original suggestion.

Oh, those changes.  I thought we were just looking at _additional_
changes.

  The attached patch changes ABORT from NOTICE to WARNING, and documents
  that all other are errors.  This top-level logic idea came from Robert
  Haas, and it has some level of consistency.
 
 This patch utterly fails to address my complaint.
 
 More to the point, I think it's a waste of time to make these sorts of
 adjustments.  The only thanks you're likely to get for it is complaints
 about cross-version behavioral changes.  Also, you're totally ignoring
 the thought that these different message levels might've been selected
 intentionally, back when the code was written.  Since there have been
 no field complaints about the inconsistency, what's your hurry to
 change it?  See Emerson.

My problem was that they issued _no_ message at all.  I am fine with
them issuing a warning if that's what people prefer.  As they are all
SET commands, they will be consistent.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-20 Thread Bruce Momjian
On Wed, Nov 20, 2013 at 10:16:00AM -0500, Bruce Momjian wrote:
   The attached patch changes ABORT from NOTICE to WARNING, and documents
   that all other are errors.  This top-level logic idea came from Robert
   Haas, and it has some level of consistency.
  
  This patch utterly fails to address my complaint.
  
  More to the point, I think it's a waste of time to make these sorts of
  adjustments.  The only thanks you're likely to get for it is complaints
  about cross-version behavioral changes.  Also, you're totally ignoring
  the thought that these different message levels might've been selected
  intentionally, back when the code was written.  Since there have been
  no field complaints about the inconsistency, what's your hurry to
  change it?  See Emerson.
 
 My problem was that they issued _no_ message at all.  I am fine with
 them issuing a warning if that's what people prefer.  As they are all
 SET commands, they will be consistent.

OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
from ERROR (which is new in 9.4) to WARNING.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/abort.sgml b/doc/src/sgml/ref/abort.sgml
new file mode 100644
index 246e8f8..f3a2fa8
*** a/doc/src/sgml/ref/abort.sgml
--- b/doc/src/sgml/ref/abort.sgml
*** ABORT [ WORK | TRANSACTION ]
*** 63,70 
/para
  
para
!Issuing commandABORT/ when not inside a transaction does
!no harm, but it will provoke a warning message.
/para
   /refsect1
  
--- 63,69 
/para
  
para
!Issuing commandABORT/ outside of a transaction block has no effect.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/rollback.sgml b/doc/src/sgml/ref/rollback.sgml
new file mode 100644
index b265545..4f79621
*** a/doc/src/sgml/ref/rollback.sgml
--- b/doc/src/sgml/ref/rollback.sgml
*** ROLLBACK [ WORK | TRANSACTION ]
*** 59,66 
/para
  
para
!Issuing commandROLLBACK/ when not inside a transaction does
!no harm, but it will provoke a warning message.
/para
   /refsect1
  
--- 59,66 
/para
  
para
!Issuing commandROLLBACK/ outside of a transaction
!block has no effect.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 6290c9d..5a84f69
*** a/doc/src/sgml/ref/set.sgml
--- b/doc/src/sgml/ref/set.sgml
*** SET [ SESSION | LOCAL ] TIME ZONE { rep
*** 110,118 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.
!   productnamePostgreSQL/productname reports an error if
!   commandSET LOCAL/ is used outside a transaction block.
   /para
  /listitem
 /varlistentry
--- 110,117 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.  This has no effect
!   outside of a transaction block.
   /para
  /listitem
 /varlistentry
diff --git a/doc/src/sgml/ref/set_constraints.sgml b/doc/src/sgml/ref/set_constraints.sgml
new file mode 100644
index 895a5fd..a33190c
*** a/doc/src/sgml/ref/set_constraints.sgml
--- b/doc/src/sgml/ref/set_constraints.sgml
*** SET CONSTRAINTS { ALL | replaceable cla
*** 99,108 
  
para
 This command only alters the behavior of constraints within the
!current transaction. Thus, if you execute this command outside of a
!transaction block
!(commandBEGIN/command/commandCOMMIT/command pair), it will
!generate an error.
/para
   /refsect1
  
--- 99,105 
  
para
 This command only alters the behavior of constraints within the
!current transaction.  This has no effect outside of a transaction block.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/set_transaction.sgml b/doc/src/sgml/ref/set_transaction.sgml
new file mode 100644
index 391464a..e90ff4a
*** a/doc/src/sgml/ref/set_transaction.sgml
--- b/doc/src/sgml/ref/set_transaction.sgml
*** SET SESSION CHARACTERISTICS AS TRANSACTI
*** 185,191 
para
 If commandSET TRANSACTION/command is executed without a prior
 commandSTART TRANSACTION/command or commandBEGIN/command,
!it will generate an error.
/para
  
para
--- 185,191 
para
 If commandSET TRANSACTION/command is executed without a prior
 commandSTART TRANSACTION/command or commandBEGIN/command,
!it will have no effect.
/para
  
para
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
new file mode 100644
index 0591f3f..2cdcc98
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c

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

2013-11-20 Thread Robert Haas
On Wed, Nov 20, 2013 at 4:19 PM, Bruce Momjian br...@momjian.us wrote:
 On Wed, Nov 20, 2013 at 10:16:00AM -0500, Bruce Momjian wrote:
   The attached patch changes ABORT from NOTICE to WARNING, and documents
   that all other are errors.  This top-level logic idea came from Robert
   Haas, and it has some level of consistency.
 
  This patch utterly fails to address my complaint.
 
  More to the point, I think it's a waste of time to make these sorts of
  adjustments.  The only thanks you're likely to get for it is complaints
  about cross-version behavioral changes.  Also, you're totally ignoring
  the thought that these different message levels might've been selected
  intentionally, back when the code was written.  Since there have been
  no field complaints about the inconsistency, what's your hurry to
  change it?  See Emerson.

 My problem was that they issued _no_ message at all.  I am fine with
 them issuing a warning if that's what people prefer.  As they are all
 SET commands, they will be consistent.

 OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
 from ERROR (which is new in 9.4) to WARNING.

Well, Tom and I are on opposite sides of this, I suppose.  I prefer
ERROR for everything other than the top-level transaction commands,
and see no benefit from opting for a wishy-washy warning.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-20 Thread Bruce Momjian
On Wed, Nov 20, 2013 at 04:31:12PM -0500, Robert Haas wrote:
 On Wed, Nov 20, 2013 at 4:19 PM, Bruce Momjian br...@momjian.us wrote:
  On Wed, Nov 20, 2013 at 10:16:00AM -0500, Bruce Momjian wrote:
The attached patch changes ABORT from NOTICE to WARNING, and documents
that all other are errors.  This top-level logic idea came from 
Robert
Haas, and it has some level of consistency.
  
   This patch utterly fails to address my complaint.
  
   More to the point, I think it's a waste of time to make these sorts of
   adjustments.  The only thanks you're likely to get for it is complaints
   about cross-version behavioral changes.  Also, you're totally ignoring
   the thought that these different message levels might've been selected
   intentionally, back when the code was written.  Since there have been
   no field complaints about the inconsistency, what's your hurry to
   change it?  See Emerson.
 
  My problem was that they issued _no_ message at all.  I am fine with
  them issuing a warning if that's what people prefer.  As they are all
  SET commands, they will be consistent.
 
  OK, here is a patch which changes ABORT from NOTICE to WARNING, and SET
  from ERROR (which is new in 9.4) to WARNING.
 
 Well, Tom and I are on opposite sides of this, I suppose.  I prefer
 ERROR for everything other than the top-level transaction commands,
 and see no benefit from opting for a wishy-washy warning.

Well, the only way I can process this is to think of psql with
ON_ERROR_STOP enabled.  Would we want a no-op command to abort psql?  I
can see logic that top-level transaction commands and SET to not, but
other commands do.  I can also see them all aborting psql, or none of
them.  :-(

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-20 Thread Kevin Grittner
Robert Haas robertmh...@gmail.com wrote:

 Well, Tom and I are on opposite sides of this, I suppose.  I
 prefer ERROR for everything other than the top-level transaction
 commands, and see no benefit from opting for a wishy-washy
 warning.

+1

If the user issued a local command outside of a transaction there
is an extremely high probability that they intended to either set
session state or affect the next transaction.  Either way,
modifying the database with the wrong setting could lead to data
corruption -- at least for some of these settings.  IMO it would be
foolish to insist on consistency with prior releases rather than on
giving people the best shot at preventing, or at least promptly
identifying the cause of, data corruption.

Obviously, changing the level of these is not material for back-
patching.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Bruce Momjian
On Sat, Nov  9, 2013 at 02:15:52PM -0500, Robert Haas wrote:
 On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  [ I'm so far behind ... ]
 
  Bruce Momjian br...@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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/declare.sgml b/doc/src/sgml/ref/declare.sgml
new file mode 100644
index d500faa..7b8c6b6
*** a/doc/src/sgml/ref/declare.sgml
--- b/doc/src/sgml/ref/declare.sgml
*** DECLARE replaceable class=parametern
*** 179,187 
  created by this command can only be used within the current
  transaction.  Thus, commandDECLARE/ without literalWITH
  HOLD/literal is useless outside a transaction block: the cursor would
! survive only to the completion of the statement.  Therefore
! productnamePostgreSQL/productname reports an error if such a
! command is used outside a transaction block.
  Use
  xref linkend=sql-begin and
  xref linkend=sql-commit
--- 179,185 
  created by this command can only be used within the current
  transaction.  Thus, commandDECLARE/ without literalWITH
  HOLD/literal is useless outside a transaction block: the cursor would
! survive only to the completion of the statement.
  Use
  xref linkend=sql-begin and
  xref linkend=sql-commit
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
new file mode 100644
index 95d6767..6a0ad20
*** a/doc/src/sgml/ref/lock.sgml
--- b/doc/src/sgml/ref/lock.sgml
*** LOCK [ TABLE ] [ ONLY ] replaceable cla
*** 168,176 
  
 para
  commandLOCK TABLE/ is useless outside a transaction block: the lock
! would remain held only to the completion of the statement.  Therefore
! productnamePostgreSQL/productname reports an error if commandLOCK/
! is used outside a transaction block.
  Use
  xref linkend=sql-begin and
  xref linkend=sql-commit
--- 168,174 
  
 para
  commandLOCK TABLE/ is useless outside a transaction block: the lock
! would remain held only to the completion of the statement.
  Use
  xref linkend=sql-begin and
  xref linkend=sql-commit
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 6290c9d..aaaf186
*** 

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

2013-11-19 Thread Andres Freund
On 2013-11-19 13:05:01 -0500, Bruce Momjian wrote:
   SAVEPOINT

   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= 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.

Why change the historical behaviour for savepoints?

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Bruce Momjian
On Tue, Nov 19, 2013 at 07:08:05PM +0100, Andres Freund wrote:
 On 2013-11-19 13:05:01 -0500, Bruce Momjian wrote:
  SAVEPOINT
 
  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= 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.
 
 Why change the historical behaviour for savepoints?

Because as Tom stated, we already do warnings for other useless
transaction commands like BEGIN WORK inside a transaction block:

test= begin work;
BEGIN
test= begin work;
-- WARNING:  there is already a transaction in progress
BEGIN
test= commit;
COMMIT
test=

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Robert Haas
On Tue, Nov 19, 2013 at 1:05 PM, Bruce Momjian br...@momjian.us wrote:
 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.

-1 from me.  I don't see this as a step forward in any way.  The
output is a complete muddle, and it's not solving any problem that I
can see.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Andres Freund
On 2013-11-19 13:09:16 -0500, Bruce Momjian wrote:
  Why change the historical behaviour for savepoints?
 
 Because as Tom stated, we already do warnings for other useless
 transaction commands like BEGIN WORK inside a transaction block:

Which imo is a bad, bad historical accident. I've repeatedly seen this
hide bugs causing corrupted data in the end.

But even if that weren't a concern, the fact that BEGIN does it one way
currently doesn't seem very indicative of changing other historical behaviour.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Bruce Momjian
On Tue, Nov 19, 2013 at 07:12:32PM +0100, Andres Freund wrote:
 On 2013-11-19 13:09:16 -0500, Bruce Momjian wrote:
   Why change the historical behaviour for savepoints?
  
  Because as Tom stated, we already do warnings for other useless
  transaction commands like BEGIN WORK inside a transaction block:
 
 Which imo is a bad, bad historical accident. I've repeatedly seen this
 hide bugs causing corrupted data in the end.
 
 But even if that weren't a concern, the fact that BEGIN does it one way
 currently doesn't seem very indicative of changing other historical behaviour.

Look at this gem, which returns notice:

test= ABORT;
NOTICE:  there is no transaction in progress
ROLLBACK
test=

We are all over the map on this!  The big question is whether we want to
add some sanity to this, or just leave it alone, and if we leave it
alone, what pattern do we use for the new checks?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Andres Freund
On 2013-11-19 13:14:34 -0500, Bruce Momjian wrote:
 On Tue, Nov 19, 2013 at 07:12:32PM +0100, Andres Freund wrote:
  But even if that weren't a concern, the fact that BEGIN does it one way
  currently doesn't seem very indicative of changing other historical 
  behaviour.
 
 Look at this gem, which returns notice:
 
   test= ABORT;
   NOTICE:  there is no transaction in progress
   ROLLBACK
   test=
 
 We are all over the map on this!  The big question is whether we want to
 add some sanity to this, or just leave it alone, and if we leave it
 alone, what pattern do we use for the new checks?

I think changing a NOTICE into a WARNING or the reverse is fine,
changing a WARNING/NOTICE into an ERROR or the reverse is something
which should be done only very hesitantly.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Robert Haas
On Tue, Nov 19, 2013 at 1:14 PM, Bruce Momjian br...@momjian.us wrote:
 On Tue, Nov 19, 2013 at 07:12:32PM +0100, Andres Freund wrote:
 On 2013-11-19 13:09:16 -0500, Bruce Momjian wrote:
   Why change the historical behaviour for savepoints?
 
  Because as Tom stated, we already do warnings for other useless
  transaction commands like BEGIN WORK inside a transaction block:

 Which imo is a bad, bad historical accident. I've repeatedly seen this
 hide bugs causing corrupted data in the end.

 But even if that weren't a concern, the fact that BEGIN does it one way
 currently doesn't seem very indicative of changing other historical 
 behaviour.

 Look at this gem, which returns notice:

 test= ABORT;
 NOTICE:  there is no transaction in progress
 ROLLBACK
 test=

 We are all over the map on this!  The big question is whether we want to
 add some sanity to this, or just leave it alone, and if we leave it
 alone, what pattern do we use for the new checks?

I think the pattern is and should be different for toplevel
transaction control commands than for other things.  If you issue a
BEGIN, we want it to end up that you're definitely in a transaction at
that point, and if you issue a COMMIT or ROLLBACK or ABORT, we want
you to definitely be out of a transaction after that.  This is
important for reasons discussed on Andrew's thread about pre-commit
triggers just today.

The same considerations don't apply elsewhere; the user has made a
mistake, and there's no particular reason not to throw an ERROR.  We
could throw a WARNING or NOTICE and pretend like things are OK, but
there doesn't seem to be much point, certainly not enough to justify
changing long-established behavior.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Bruce Momjian
On Tue, Nov 19, 2013 at 01:20:47PM -0500, Robert Haas wrote:
 I think the pattern is and should be different for toplevel
 transaction control commands than for other things.  If you issue a
 BEGIN, we want it to end up that you're definitely in a transaction at
 that point, and if you issue a COMMIT or ROLLBACK or ABORT, we want
 you to definitely be out of a transaction after that.  This is
 important for reasons discussed on Andrew's thread about pre-commit
 triggers just today.
 
 The same considerations don't apply elsewhere; the user has made a
 mistake, and there's no particular reason not to throw an ERROR.  We
 could throw a WARNING or NOTICE and pretend like things are OK, but
 there doesn't seem to be much point, certainly not enough to justify
 changing long-established behavior.

OK, what I am hearing you say is that we should change ABORT from NOTICE
to WARNING, leave SAVEPOINT/ROLLBACK TO SAVEPOINT as WARNING (so all
transaction control commands are warnings), and leave the new SET
commands as ERRORs.  Works for me.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Bruce Momjian
On Tue, Nov 19, 2013 at 01:31:55PM -0500, Bruce Momjian wrote:
 On Tue, Nov 19, 2013 at 01:20:47PM -0500, Robert Haas wrote:
  I think the pattern is and should be different for toplevel
  transaction control commands than for other things.  If you issue a
  BEGIN, we want it to end up that you're definitely in a transaction at
  that point, and if you issue a COMMIT or ROLLBACK or ABORT, we want
  you to definitely be out of a transaction after that.  This is
  important for reasons discussed on Andrew's thread about pre-commit
  triggers just today.
  
  The same considerations don't apply elsewhere; the user has made a
  mistake, and there's no particular reason not to throw an ERROR.  We
  could throw a WARNING or NOTICE and pretend like things are OK, but
  there doesn't seem to be much point, certainly not enough to justify
  changing long-established behavior.
 
 OK, what I am hearing you say is that we should change ABORT from NOTICE
 to WARNING, leave SAVEPOINT/ROLLBACK TO SAVEPOINT as WARNING (so all
 transaction control commands are warnings), and leave the new SET
 commands as ERRORs.  Works for me.

Sorry, even I am getting confused.  SAVEPOINT/ROLLBACK TO SAVEPOINT stay
as ERROR, so effectively only top-level transaction control commands
BEGIN WORK/ABORT/COMMIT are WARNINGS.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Bruce Momjian
On Tue, Nov 19, 2013 at 01:37:56PM -0500, Bruce Momjian wrote:
 On Tue, Nov 19, 2013 at 01:31:55PM -0500, Bruce Momjian wrote:
  On Tue, Nov 19, 2013 at 01:20:47PM -0500, Robert Haas wrote:
   I think the pattern is and should be different for toplevel
   transaction control commands than for other things.  If you issue a
   BEGIN, we want it to end up that you're definitely in a transaction at
   that point, and if you issue a COMMIT or ROLLBACK or ABORT, we want
   you to definitely be out of a transaction after that.  This is
   important for reasons discussed on Andrew's thread about pre-commit
   triggers just today.
   
   The same considerations don't apply elsewhere; the user has made a
   mistake, and there's no particular reason not to throw an ERROR.  We
   could throw a WARNING or NOTICE and pretend like things are OK, but
   there doesn't seem to be much point, certainly not enough to justify
   changing long-established behavior.
  
  OK, what I am hearing you say is that we should change ABORT from NOTICE
  to WARNING, leave SAVEPOINT/ROLLBACK TO SAVEPOINT as WARNING (so all
  transaction control commands are warnings), and leave the new SET
  commands as ERRORs.  Works for me.
 
 Sorry, even I am getting confused.  SAVEPOINT/ROLLBACK TO SAVEPOINT stay
 as ERROR, so effectively only top-level transaction control commands
 BEGIN WORK/ABORT/COMMIT are WARNINGS.

Does anyone know if this C comment justifies why ABORT is a NOTICE and
not WARNING?

/*
 * The user issued ABORT when not inside a transaction. Issue a
 * NOTICE and go to abort state.  The upcoming call to
 * CommitTransactionCommand() will then put us back into the
 * default state.
 */

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Dimitri Fontaine
Andres Freund and...@2ndquadrant.com writes:
 On 2013-11-19 13:09:16 -0500, Bruce Momjian wrote:
 Because as Tom stated, we already do warnings for other useless
 transaction commands like BEGIN WORK inside a transaction block:

 Which imo is a bad, bad historical accident. I've repeatedly seen this
 hide bugs causing corrupted data in the end.

+1

-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-19 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 Does anyone know if this C comment justifies why ABORT is a NOTICE and
 not WARNING?

 /*
  * The user issued ABORT when not inside a transaction. Issue a
  * NOTICE and go to abort state.  The upcoming call to
  * CommitTransactionCommand() will then put us back into the
  * default state.
  */

It's just describing the implementation, not defending the design choice.

My personal standpoint is that I don't care much whether these messages
are NOTICE or WARNING.  What I'm not happy about is promoting cases that
have been non-error conditions for years into ERRORs.

Now, if we wanted to go the other way (downgrade some ERRORs to WARNINGs),
that would not create an application compatibility problem in my view.

And on the third hand, there's Emerson's excellent advice: A foolish
consistency is the hobgoblin of little minds.  I'm not convinced that
trying to make all these cases have the same message level is actually
a good goal.  It seems entirely plausible that some are more dangerous
than others.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-18 Thread Bruce Momjian
On Sat, Nov  9, 2013 at 02:15:52PM -0500, Robert Haas wrote:
 On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  [ I'm so far behind ... ]
 
  Bruce Momjian br...@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.

Yeah, I just copied the LOCK TABLE usage.  You could argue that SET
SESSION ISOLATION only affects later commands and doesn't do something
like LOCK, so it should be a warning.  Not sure how to interpret the
COMMIT/ROLLBACK behavior you mentioned.

Considering we are doing this outside of a transaction, and WARNING or
ERROR is pretty much the same, from a behavioral perspective.

Should we change this and LOCK to be a warning?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-09 Thread Robert Haas
On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 [ I'm so far behind ... ]

 Bruce Momjian br...@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.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-08 Thread Tom Lane
[ I'm so far behind ... ]

Bruce Momjian br...@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.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-10-04 Thread Bruce Momjian
On Fri, Oct  4, 2013 at 09:40:38AM +0530, Amit Kapila wrote:
 On Thu, Oct 3, 2013 at 8:35 PM, Andres Freund and...@2ndquadrant.com wrote:
  On 2013-09-30 22:19:31 -0400, Bruce Momjian wrote:
  On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
Shouldn't we do it for Set Constraints as well?
   
Oh, very good point.  I missed that one.  Updated patch attached.
 
  I am glad you are seeing things I am not.  :-)
 
   1. The function set_config also needs similar functionality, else
   there will be inconsistency, the SQL statement will give error but
   equivalent function set_config() will succeed.
  
   SQL Command
   postgres=# set local search_path='public';
   ERROR:  SET LOCAL can only be used in transaction blocks
  
   Function
   postgres=# select set_config('search_path', 'public', true);
set_config
   
public
   (1 row)
 
  I looked at this but could not see how to easily pass the value of
  'isTopLevel' down to the SELECT.  All the other checks have isTopLevel
  passed down from the utility case statement.
 
  Doesn't sound like a good idea to prohibit that anyway, it might
  intentionally be used as part of a more complex statement where it only
  should take effect during that single statement.
 
Agreed and I think it is good reason for not changing behaviour of
 set_config().

Applied.  Thank you for all your suggestions.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-10-03 Thread Amit Kapila
On Tue, Oct 1, 2013 at 7:49 AM, Bruce Momjian br...@momjian.us wrote:
 On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
  Shouldn't we do it for Set Constraints as well?
 
  Oh, very good point.  I missed that one.  Updated patch attached.

 I am glad you are seeing things I am not.  :-)

 1. The function set_config also needs similar functionality, else
 there will be inconsistency, the SQL statement will give error but
 equivalent function set_config() will succeed.

 SQL Command
 postgres=# set local search_path='public';
 ERROR:  SET LOCAL can only be used in transaction blocks

 Function
 postgres=# select set_config('search_path', 'public', true);
  set_config
 
  public
 (1 row)

 I looked at this but could not see how to easily pass the value of
 'isTopLevel' down to the SELECT.  All the other checks have isTopLevel
 passed down from the utility case statement.

Yes, we cannot pass isTopLevel, but as isTopLevel is used to decide
whether we are in function (user defined) call, so if we can find
during statement execution (current case set_config execution) that
current statement is inside user function execution, then it can be
handled.
For example, one of the ways could be to use a mechanism similar to
setting of user id and sec context used by fmgr_security_definer() (by
calling function SetUserIdAndSecContext()), once userid and sec
context are set by fmgr_security_definer(), later we can use
InSecurityRestrictedOperation() anywhere to give error.

For current case, what we can do is after analyze
(pg_analyze_and_rewrite), check if its not a builtin function (as we
can have functionid after analyze, so it can be checked
fmgr_isbuiltin(functionId)) and set variable to indicate that we are
in function call.

Any better or simpler idea can also be used to identify isTopLevel
during function execution.

Doing it only for detection of transaction chain in set_config path
might seem to be more work, but I think it can be used at other places
for detection of transaction chain as well.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-10-03 Thread Bruce Momjian
On Thu, Oct  3, 2013 at 11:50:09AM +0530, Amit Kapila wrote:
  I looked at this but could not see how to easily pass the value of
  'isTopLevel' down to the SELECT.  All the other checks have isTopLevel
  passed down from the utility case statement.
 
 Yes, we cannot pass isTopLevel, but as isTopLevel is used to decide
 whether we are in function (user defined) call, so if we can find
 during statement execution (current case set_config execution) that
 current statement is inside user function execution, then it can be
 handled.
 For example, one of the ways could be to use a mechanism similar to
 setting of user id and sec context used by fmgr_security_definer() (by
 calling function SetUserIdAndSecContext()), once userid and sec
 context are set by fmgr_security_definer(), later we can use
 InSecurityRestrictedOperation() anywhere to give error.
 
 For current case, what we can do is after analyze
 (pg_analyze_and_rewrite), check if its not a builtin function (as we
 can have functionid after analyze, so it can be checked
 fmgr_isbuiltin(functionId)) and set variable to indicate that we are
 in function call.
 
 Any better or simpler idea can also be used to identify isTopLevel
 during function execution.
 
 Doing it only for detection of transaction chain in set_config path
 might seem to be more work, but I think it can be used at other places
 for detection of transaction chain as well.

I am also worried about over-engineering this.  I will wait to see if
anyone else would find top-level detection useful, and if not, I will
just apply my version of that patch that does not handle set_config.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-10-03 Thread Andres Freund
On 2013-09-30 22:19:31 -0400, Bruce Momjian wrote:
 On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
   Shouldn't we do it for Set Constraints as well?
  
   Oh, very good point.  I missed that one.  Updated patch attached.
 
 I am glad you are seeing things I am not.  :-)
 
  1. The function set_config also needs similar functionality, else
  there will be inconsistency, the SQL statement will give error but
  equivalent function set_config() will succeed.
  
  SQL Command
  postgres=# set local search_path='public';
  ERROR:  SET LOCAL can only be used in transaction blocks
  
  Function
  postgres=# select set_config('search_path', 'public', true);
   set_config
  
   public
  (1 row)
 
 I looked at this but could not see how to easily pass the value of
 'isTopLevel' down to the SELECT.  All the other checks have isTopLevel
 passed down from the utility case statement.

Doesn't sound like a good idea to prohibit that anyway, it might
intentionally be used as part of a more complex statement where it only
should take effect during that single statement.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-10-03 Thread Amit Kapila
On Thu, Oct 3, 2013 at 8:35 PM, Andres Freund and...@2ndquadrant.com wrote:
 On 2013-09-30 22:19:31 -0400, Bruce Momjian wrote:
 On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
   Shouldn't we do it for Set Constraints as well?
  
   Oh, very good point.  I missed that one.  Updated patch attached.

 I am glad you are seeing things I am not.  :-)

  1. The function set_config also needs similar functionality, else
  there will be inconsistency, the SQL statement will give error but
  equivalent function set_config() will succeed.
 
  SQL Command
  postgres=# set local search_path='public';
  ERROR:  SET LOCAL can only be used in transaction blocks
 
  Function
  postgres=# select set_config('search_path', 'public', true);
   set_config
  
   public
  (1 row)

 I looked at this but could not see how to easily pass the value of
 'isTopLevel' down to the SELECT.  All the other checks have isTopLevel
 passed down from the utility case statement.

 Doesn't sound like a good idea to prohibit that anyway, it might
 intentionally be used as part of a more complex statement where it only
 should take effect during that single statement.

   Agreed and I think it is good reason for not changing behaviour of
set_config().

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-10-03 Thread Amit Kapila
On Thu, Oct 3, 2013 at 8:32 PM, Bruce Momjian br...@momjian.us wrote:
 On Thu, Oct  3, 2013 at 11:50:09AM +0530, Amit Kapila wrote:
  I looked at this but could not see how to easily pass the value of
  'isTopLevel' down to the SELECT.  All the other checks have isTopLevel
  passed down from the utility case statement.

 Yes, we cannot pass isTopLevel, but as isTopLevel is used to decide
 whether we are in function (user defined) call, so if we can find
 during statement execution (current case set_config execution) that
 current statement is inside user function execution, then it can be
 handled.
 For example, one of the ways could be to use a mechanism similar to
 setting of user id and sec context used by fmgr_security_definer() (by
 calling function SetUserIdAndSecContext()), once userid and sec
 context are set by fmgr_security_definer(), later we can use
 InSecurityRestrictedOperation() anywhere to give error.

 For current case, what we can do is after analyze
 (pg_analyze_and_rewrite), check if its not a builtin function (as we
 can have functionid after analyze, so it can be checked
 fmgr_isbuiltin(functionId)) and set variable to indicate that we are
 in function call.

 Any better or simpler idea can also be used to identify isTopLevel
 during function execution.

 Doing it only for detection of transaction chain in set_config path
 might seem to be more work, but I think it can be used at other places
 for detection of transaction chain as well.

 I am also worried about over-engineering this.

   I had tried to think hard but could not come up with a simpler
change which could have handled all cases.
   We can leave the handling for set_config() and proceed with patch
as Andres already given a reason where set_config() can be useful
within a
   statement as well.

  I will wait to see if
 anyone else would find top-level detection useful, and if not, I will
 just apply my version of that patch that does not handle set_config.

  I had verified the patch once again and ran regression, everything looks fine.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-09-30 Thread Bruce Momjian
On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
  Shouldn't we do it for Set Constraints as well?
 
  Oh, very good point.  I missed that one.  Updated patch attached.

I am glad you are seeing things I am not.  :-)

 1. The function set_config also needs similar functionality, else
 there will be inconsistency, the SQL statement will give error but
 equivalent function set_config() will succeed.
 
 SQL Command
 postgres=# set local search_path='public';
 ERROR:  SET LOCAL can only be used in transaction blocks
 
 Function
 postgres=# select set_config('search_path', 'public', true);
  set_config
 
  public
 (1 row)

I looked at this but could not see how to easily pass the value of
'isTopLevel' down to the SELECT.  All the other checks have isTopLevel
passed down from the utility case statement.

 2. I think we should update the documentation as well.
 
 For example:
 The documentation of LOCK TABLE
 (http://www.postgresql.org/docs/devel/static/sql-lock.html) clearly
 indicates that it will give error if used outside transaction block.
 
 LOCK TABLE is useless outside a transaction block: the lock would
 remain held only to the completion of the statement. Therefore
 PostgreSQL reports an error if LOCK is used outside a transaction
 block. Use BEGINand COMMIT (or ROLLBACK) to define a transaction
 block.
 
 
 The documentation of SET TRANSACTION
 (http://www.postgresql.org/docs/devel/static/sql-set-transaction.html)
 has below line which doesn't indicate that it will give error if used
 outside transaction block.
 
 If SET TRANSACTION is executed without a prior START TRANSACTION or
 BEGIN, it will appear to have no effect, since the transaction will
 immediately end.

Yes, you are right. All cases I changed had mentions of the command
having no effect;  I have updated it to mention an error is generated.

 3.
 
 void
 ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
 {
 ..
 ..
 else if (strcmp(stmt-name, TRANSACTION SNAPSHOT) == 0)
 {
 A_Const*con = (A_Const *) linitial(stmt-args);
 
 RequireTransactionChain(isTopLevel, SET TRANSACTION);
 
 if (stmt-is_local)
 ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 errmsg(SET LOCAL TRANSACTION SNAPSHOT is not implemented)));
 ..
 }
 ..
 ..
 }
 
 Wouldn't it be better if call to RequireTransactionChain() is done
 after if (stmt-is_local)?

Yes, good point.  Done.

Thanks much for your review.  Updated patch attached.  

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 21745db..d108dd4
*** a/doc/src/sgml/ref/set.sgml
--- b/doc/src/sgml/ref/set.sgml
*** SET [ SESSION | LOCAL ] TIME ZONE { rep
*** 110,119 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.  Note that
!   commandSET LOCAL/ will appear to have no effect if it is
!   executed outside a commandBEGIN/ block, since the
!   transaction will end immediately.
   /para
  /listitem
 /varlistentry
--- 110,118 
   para
Specifies that the command takes effect for only the current
transaction.  After commandCOMMIT/ or commandROLLBACK/,
!   the session-level setting takes effect again.
!   productnamePostgreSQL/productname reports an error if
!   commandSET LOCAL/ is used outside a transaction block.
   /para
  /listitem
 /varlistentry
diff --git a/doc/src/sgml/ref/set_constraints.sgml b/doc/src/sgml/ref/set_constraints.sgml
new file mode 100644
index 8098b7b..895a5fd
*** a/doc/src/sgml/ref/set_constraints.sgml
--- b/doc/src/sgml/ref/set_constraints.sgml
*** SET CONSTRAINTS { ALL | replaceable cla
*** 102,108 
 current transaction. Thus, if you execute this command outside of a
 transaction block
 (commandBEGIN/command/commandCOMMIT/command pair), it will
!not appear to have any effect.
/para
   /refsect1
  
--- 102,108 
 current transaction. Thus, if you execute this command outside of a
 transaction block
 (commandBEGIN/command/commandCOMMIT/command pair), it will
!generate an error.
/para
   /refsect1
  
diff --git a/doc/src/sgml/ref/set_transaction.sgml b/doc/src/sgml/ref/set_transaction.sgml
new file mode 100644
index f060729..391464a
*** a/doc/src/sgml/ref/set_transaction.sgml
--- b/doc/src/sgml/ref/set_transaction.sgml
*** SET SESSION CHARACTERISTICS AS TRANSACTI
*** 184,192 
  
para
 If commandSET TRANSACTION/command is executed without a prior
!commandSTART TRANSACTION/command or  commandBEGIN/command,
!it will appear to have no effect, since the transaction will immediately
!end.
/para
  
para
--- 

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

2013-09-29 Thread Amit Kapila
On Wed, Sep 25, 2013 at 2:55 AM, Bruce Momjian br...@momjian.us wrote:
 On Thu, Sep 12, 2013 at 09:38:43AM +0530, Amit Kapila wrote:
  I have created the attached patch which issues an error when SET
  TRANSACTION and SET LOCAL are used outside of transactions:
 
  test= set transaction isolation level serializable;
  ERROR:  SET TRANSACTION can only be used in transaction blocks
  test= reset transaction isolation level;
  ERROR:  RESET TRANSACTION can only be used in transaction blocks
 
  test= set local effective_cache_size = '3MB';
  ERROR:  SET LOCAL can only be used in transaction blocks
  test= set local effective_cache_size = default;
  ERROR:  SET LOCAL can only be used in transaction blocks

 Shouldn't we do it for Set Constraints as well?

 Oh, very good point.  I missed that one.  Updated patch attached.

1. The function set_config also needs similar functionality, else
there will be inconsistency, the SQL statement will give error but
equivalent function set_config() will succeed.

SQL Command
postgres=# set local search_path='public';
ERROR:  SET LOCAL can only be used in transaction blocks

Function
postgres=# select set_config('search_path', 'public', true);
 set_config

 public
(1 row)

2. I think we should update the documentation as well.

For example:
The documentation of LOCK TABLE
(http://www.postgresql.org/docs/devel/static/sql-lock.html) clearly
indicates that it will give error if used outside transaction block.

LOCK TABLE is useless outside a transaction block: the lock would
remain held only to the completion of the statement. Therefore
PostgreSQL reports an error if LOCK is used outside a transaction
block. Use BEGINand COMMIT (or ROLLBACK) to define a transaction
block.


The documentation of SET TRANSACTION
(http://www.postgresql.org/docs/devel/static/sql-set-transaction.html)
has below line which doesn't indicate that it will give error if used
outside transaction block.

If SET TRANSACTION is executed without a prior START TRANSACTION or
BEGIN, it will appear to have no effect, since the transaction will
immediately end.


3.

void
ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
{
..
..
else if (strcmp(stmt-name, TRANSACTION SNAPSHOT) == 0)
{
A_Const*con = (A_Const *) linitial(stmt-args);

RequireTransactionChain(isTopLevel, SET TRANSACTION);

if (stmt-is_local)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg(SET LOCAL TRANSACTION SNAPSHOT is not implemented)));
..
}
..
..
}

Wouldn't it be better if call to RequireTransactionChain() is done
after if (stmt-is_local)?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-09-24 Thread Bruce Momjian
On Thu, Sep 12, 2013 at 09:38:43AM +0530, Amit Kapila wrote:
  I have created the attached patch which issues an error when SET
  TRANSACTION and SET LOCAL are used outside of transactions:
 
  test= set transaction isolation level serializable;
  ERROR:  SET TRANSACTION can only be used in transaction blocks
  test= reset transaction isolation level;
  ERROR:  RESET TRANSACTION can only be used in transaction blocks
 
  test= set local effective_cache_size = '3MB';
  ERROR:  SET LOCAL can only be used in transaction blocks
  test= set local effective_cache_size = default;
  ERROR:  SET LOCAL can only be used in transaction blocks
 
 Shouldn't we do it for Set Constraints as well?

Oh, very good point.  I missed that one.  Updated patch attached.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
new file mode 100644
index b1023c4..3ffdfe0
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*** standard_ProcessUtility(Node *parsetree,
*** 688,694 
  			break;
  
  		case T_VariableSetStmt:
! 			ExecSetVariableStmt((VariableSetStmt *) parsetree);
  			break;
  
  		case T_VariableShowStmt:
--- 688,694 
  			break;
  
  		case T_VariableSetStmt:
! 			ExecSetVariableStmt((VariableSetStmt *) parsetree, isTopLevel);
  			break;
  
  		case T_VariableShowStmt:
*** standard_ProcessUtility(Node *parsetree,
*** 754,759 
--- 754,760 
  			break;
  
  		case T_ConstraintsSetStmt:
+ 			RequireTransactionChain(isTopLevel, SET CONSTRAINTS);
  			AfterTriggerSetState((ConstraintsSetStmt *) parsetree);
  			break;
  
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 3107f9c..ff39920
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** flatten_set_variable_args(const char *na
*** 6252,6258 
   * SET command
   */
  void
! ExecSetVariableStmt(VariableSetStmt *stmt)
  {
  	GucAction	action = stmt-is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET;
  
--- 6252,6258 
   * SET command
   */
  void
! ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
  {
  	GucAction	action = stmt-is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET;
  
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6260,6265 
--- 6260,6267 
  	{
  		case VAR_SET_VALUE:
  		case VAR_SET_CURRENT:
+ 			if (stmt-is_local)
+ RequireTransactionChain(isTopLevel, SET LOCAL);
  			(void) set_config_option(stmt-name,
  	 ExtractSetVariableArgs(stmt),
  	 (superuser() ? PGC_SUSET : PGC_USERSET),
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6269,6275 
  	 0);
  			break;
  		case VAR_SET_MULTI:
- 
  			/*
  			 * Special-case SQL syntaxes.  The TRANSACTION and SESSION
  			 * CHARACTERISTICS cases effectively set more than one variable
--- 6271,6276 
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6281,6286 
--- 6282,6289 
  			{
  ListCell   *head;
  
+ RequireTransactionChain(isTopLevel, SET TRANSACTION);
+ 
  foreach(head, stmt-args)
  {
  	DefElem*item = (DefElem *) lfirst(head);
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6325,6330 
--- 6328,6335 
  			{
  A_Const*con = (A_Const *) linitial(stmt-args);
  
+ RequireTransactionChain(isTopLevel, SET TRANSACTION);
+ 
  if (stmt-is_local)
  	ereport(ERROR,
  			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6338,6344 
--- 6343,6355 
  	 stmt-name);
  			break;
  		case VAR_SET_DEFAULT:
+ 			if (stmt-is_local)
+ RequireTransactionChain(isTopLevel, SET LOCAL);
+ 			/* fall through */
  		case VAR_RESET:
+ 			if (strcmp(stmt-name, transaction_isolation) == 0)
+ RequireTransactionChain(isTopLevel, RESET TRANSACTION);
+ 
  			(void) set_config_option(stmt-name,
  	 NULL,
  	 (superuser() ? PGC_SUSET : PGC_USERSET),
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
new file mode 100644
index 99211c1..89ee40c
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
*** extern void SetPGVariable(const char *na
*** 334,340 
  extern void GetPGVariable(const char *name, DestReceiver *dest);
  extern TupleDesc GetPGVariableResultDesc(const char *name);
  
! extern void ExecSetVariableStmt(VariableSetStmt *stmt);
  extern char *ExtractSetVariableArgs(VariableSetStmt *stmt);
  
  extern void ProcessGUCArray(ArrayType *array,
--- 334,340 
  extern void GetPGVariable(const char *name, DestReceiver *dest);
  extern TupleDesc GetPGVariableResultDesc(const char *name);
  
! extern void ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel);
  extern 

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

2013-09-11 Thread Amit Kapila
On Wed, Sep 11, 2013 at 4:19 AM, Bruce Momjian br...@momjian.us wrote:
 On Sun, Feb  3, 2013 at 07:19:14AM +, Amit kapila wrote:

 On Saturday, February 02, 2013 9:08 PM Robert Haas wrote:
 On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila amit.kap...@huawei.com wrote:
  I think user should be aware of effect before using SET commands, as 
  these are used at various levels (TRANSACTION, SESSION, ...).

  Ideally, sure.  But these kinds of mistakes are easy to make.

   You mean to say that after using SET Transaction, user can think below 
 statements will
   use modified transaction property. But I think if user doesn't understand 
 by default
   transaction will be committed after the statement execution, he might 
 expect after
   few statements he can rollback.

  That's why LOCK and DECLARE CURSOR already emit errors in this case - why
  should this one be any different?

 IMO, I think error should be given when it is not possible to execute 
 current statement.

 Not only LOCK,DECLARE CURSOR but SAVEPOINT and some other statements also 
 give the same error,
 so if we want to throw error for such behavior, we can find all such similar 
 statements
 (SET TRANSACTION, SET LOCAL, etc) and do it for all.

 This can be helpful to some users, but not sure if such behavior (statement 
 can be executed but it will not have any sense)
 can be always detectable and maintaible.

 I have created the attached patch which issues an error when SET
 TRANSACTION and SET LOCAL are used outside of transactions:

 test= set transaction isolation level serializable;
 ERROR:  SET TRANSACTION can only be used in transaction blocks
 test= reset transaction isolation level;
 ERROR:  RESET TRANSACTION can only be used in transaction blocks

 test= set local effective_cache_size = '3MB';
 ERROR:  SET LOCAL can only be used in transaction blocks
 test= set local effective_cache_size = default;
 ERROR:  SET LOCAL can only be used in transaction blocks

Shouldn't we do it for Set Constraints as well?


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-09-10 Thread Bruce Momjian
On Sun, Feb  3, 2013 at 07:19:14AM +, Amit kapila wrote:
 
 On Saturday, February 02, 2013 9:08 PM Robert Haas wrote:
 On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila amit.kap...@huawei.com wrote:
  I think user should be aware of effect before using SET commands, as these 
  are used at various levels (TRANSACTION, SESSION, ...).
 
  Ideally, sure.  But these kinds of mistakes are easy to make.  
 
   You mean to say that after using SET Transaction, user can think below 
 statements will
   use modified transaction property. But I think if user doesn't understand 
 by default
   transaction will be committed after the statement execution, he might 
 expect after 
   few statements he can rollback.
 
  That's why LOCK and DECLARE CURSOR already emit errors in this case - why
  should this one be any different?
 
 IMO, I think error should be given when it is not possible to execute current 
 statement.
 
 Not only LOCK,DECLARE CURSOR but SAVEPOINT and some other statements also 
 give the same error,
 so if we want to throw error for such behavior, we can find all such similar 
 statements 
 (SET TRANSACTION, SET LOCAL, etc) and do it for all.
 
 This can be helpful to some users, but not sure if such behavior (statement 
 can be executed but it will not have any sense) 
 can be always detectable and maintaible.

I have created the attached patch which issues an error when SET
TRANSACTION and SET LOCAL are used outside of transactions:

test= set transaction isolation level serializable;
ERROR:  SET TRANSACTION can only be used in transaction blocks
test= reset transaction isolation level;
ERROR:  RESET TRANSACTION can only be used in transaction blocks

test= set local effective_cache_size = '3MB';
ERROR:  SET LOCAL can only be used in transaction blocks
test= set local effective_cache_size = default;
ERROR:  SET LOCAL can only be used in transaction blocks

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
new file mode 100644
index b1023c4..0da041b
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*** standard_ProcessUtility(Node *parsetree,
*** 688,694 
  			break;
  
  		case T_VariableSetStmt:
! 			ExecSetVariableStmt((VariableSetStmt *) parsetree);
  			break;
  
  		case T_VariableShowStmt:
--- 688,694 
  			break;
  
  		case T_VariableSetStmt:
! 			ExecSetVariableStmt((VariableSetStmt *) parsetree, isTopLevel);
  			break;
  
  		case T_VariableShowStmt:
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 7d297bc..168cd95
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** flatten_set_variable_args(const char *na
*** 6240,6246 
   * SET command
   */
  void
! ExecSetVariableStmt(VariableSetStmt *stmt)
  {
  	GucAction	action = stmt-is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET;
  
--- 6240,6246 
   * SET command
   */
  void
! ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
  {
  	GucAction	action = stmt-is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET;
  
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6248,6253 
--- 6248,6255 
  	{
  		case VAR_SET_VALUE:
  		case VAR_SET_CURRENT:
+ 			if (stmt-is_local)
+ RequireTransactionChain(isTopLevel, SET LOCAL);
  			(void) set_config_option(stmt-name,
  	 ExtractSetVariableArgs(stmt),
  	 (superuser() ? PGC_SUSET : PGC_USERSET),
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6257,6263 
  	 0);
  			break;
  		case VAR_SET_MULTI:
- 
  			/*
  			 * Special-case SQL syntaxes.  The TRANSACTION and SESSION
  			 * CHARACTERISTICS cases effectively set more than one variable
--- 6259,6264 
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6269,6274 
--- 6270,6277 
  			{
  ListCell   *head;
  
+ RequireTransactionChain(isTopLevel, SET TRANSACTION);
+ 
  foreach(head, stmt-args)
  {
  	DefElem*item = (DefElem *) lfirst(head);
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6313,6318 
--- 6316,6323 
  			{
  A_Const*con = (A_Const *) linitial(stmt-args);
  
+ RequireTransactionChain(isTopLevel, SET TRANSACTION);
+ 
  if (stmt-is_local)
  	ereport(ERROR,
  			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
*** ExecSetVariableStmt(VariableSetStmt *stm
*** 6326,6332 
--- 6331,6343 
  	 stmt-name);
  			break;
  		case VAR_SET_DEFAULT:
+ 			if (stmt-is_local)
+ RequireTransactionChain(isTopLevel, SET LOCAL);
+ 			/* fall through */
  		case VAR_RESET:
+ 			if (strcmp(stmt-name, transaction_isolation) == 0)
+ RequireTransactionChain(isTopLevel, RESET TRANSACTION);
+ 
  			(void) 

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

2013-02-02 Thread Robert Haas
On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila amit.kap...@huawei.com wrote:
 I think user should be aware of effect before using SET commands, as these 
 are used at various levels (TRANSACTION, SESSION, ...).

Ideally, sure.  But these kinds of mistakes are easy to make.  That's
why LOCK and DECLARE CURSOR already emit errors in this case - why
should this one be any different?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-02-02 Thread Amit kapila

On Saturday, February 02, 2013 9:08 PM Robert Haas wrote:
On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila amit.kap...@huawei.com wrote:
 I think user should be aware of effect before using SET commands, as these 
 are used at various levels (TRANSACTION, SESSION, ...).

 Ideally, sure.  But these kinds of mistakes are easy to make.  

  You mean to say that after using SET Transaction, user can think below 
statements will
  use modified transaction property. But I think if user doesn't understand by 
default
  transaction will be committed after the statement execution, he might expect 
after 
  few statements he can rollback.

 That's why LOCK and DECLARE CURSOR already emit errors in this case - why
 should this one be any different?

IMO, I think error should be given when it is not possible to execute current 
statement.

Not only LOCK,DECLARE CURSOR but SAVEPOINT and some other statements also give 
the same error,
so if we want to throw error for such behavior, we can find all such similar 
statements 
(SET TRANSACTION, SET LOCAL, etc) and do it for all.

This can be helpful to some users, but not sure if such behavior (statement can 
be executed but it will not have any sense) 
can be always detectable and maintaible.

With Regards,
Amit Kapila.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-01-31 Thread Amit Kapila
On Wednesday, January 30, 2013 6:53 AM Morten Hustveit wrote:
 Hi!
 
 Calling SET TRANSACTION ISOLATION LEVEL ... outside a transaction
 block has no effect.  This is unlike LOCK ... and DECLARE foo
 CURSOR FOR ..., which both raise an error.  This is also unlike
 MySQL, where such a statement will affect the next transaction
 performed.  There's some risk of data corruption, as a user might
 assume he's working on a snapshot, while in fact he's not.

The behavior of SET TRANSACTION ISOLATION LEVEL ... needs to be compared with 
SET LOCAL ...
These commands are used to set property of current transaction in which they 
are executed.

The usage can be clear with below function, where it is used to set the current 
transaction property.

Create or Replace function temp_trans() Returns boolean AS $$ 
Declare sync_status boolean; 
Begin 
Set LOCAL synchronous_commit=off; 
show synchronous_commit into sync_status; 
return sync_status; 
End; 
$$ Language plpgsql;
 
 I suggest issuing a warning, notice or error message when SET
 TRANSACTION ... is called outside a transaction block, possibly
 directing the user to the SET SESSION CHARACTERISTICS AS TRANSACTION
 ... syntax.
 
It is already mentioned in documentation that SET Transaction command is used 
to set characteristics of current transaction 
(http://www.postgresql.org/docs/9.2/static/sql-set-transaction.html). 

I think user should be aware of effect before using SET commands, as these are 
used at various levels (TRANSACTION, SESSION, ...).

With Regards,
Amit Kapila.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers