Re: [HACKERS] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
[ 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
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
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
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
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
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
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
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
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
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
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
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
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
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
[HACKERS] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
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. 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. I'm not familiar with the PostgreSQL source code, but it seems this would have to be added to check_XactIsoLevel() or by calling RequireTransactionChain() at some appropriate location. -- 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
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
