Re: [HACKERS] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
On Nov 28, 2013, at 5:18 PM, Bruce Momjian 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 wrote: Seems broadly reasonable, but I'd use "no other effect" throughout. >>> >>> That sounds awkward, e.g.: >>> >>> Issuing ROLLBACK outside of a transaction >>> block emits a warning but has no other effect. >>> >>> I could live with this: >>> >>> Issuing ROLLBACK 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 ROLLBACK 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 Thu, Nov 28, 2013 at 04:51:14PM -0500, Robert Haas wrote: > On Wed, Nov 27, 2013 at 4:44 PM, Bruce Momjian wrote: > >> Seems broadly reasonable, but I'd use "no other effect" throughout. > > > > That sounds awkward, e.g.: > > > > Issuing ROLLBACK outside of a transaction > > block emits a warning but has no other effect. > > > > I could live with this: > > > > Issuing ROLLBACK 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 ROLLBACK outside of a transaction block has the sole effect of emitting a warning. -- Bruce Momjian http://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 4:44 PM, Bruce Momjian wrote: >> Seems broadly reasonable, but I'd use "no other effect" throughout. > > That sounds awkward, e.g.: > > Issuing ROLLBACK outside of a transaction > block emits a warning but has no other effect. > > I could live with this: > > Issuing ROLLBACK 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 Wed, Nov 27, 2013 at 04:44:02PM -0500, Bruce Momjian wrote:
> I could live with this:
>
> Issuing ROLLBACK outside of a transaction
> block has no effect except emitting a warning.
Proposed doc patch attached.
--
Bruce Momjian http://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
!Issuing ABORT outside of a transaction block has no effect.
--- 63,70
!Issuing ABORT outside of a transaction block
!has no effect except emitting a warning.
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
Issuing ROLLBACK outside of a transaction
!block has no effect.
--- 60,66
Issuing ROLLBACK outside of a transaction
!block has no effect except emitting a warning.
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 {
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again. This has no effect
! outside of a transaction block.
--- 110,117
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again. Issuing this
! outside of a transaction block has no effect except emitting a warning.
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 |
This command only alters the behavior of constraints within the
!current transaction. This has no effect outside of a transaction block.
--- 99,106
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.
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
If SET TRANSACTION is executed without a prior
START TRANSACTION or BEGIN,
!it will have no effect.
--- 185,191
If SET TRANSACTION is executed without a prior
START TRANSACTION or BEGIN,
!ithas no effect except emitting a warning.
--
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 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 ROLLBACK outside of a transaction block emits a warning but has no other effect. I could live with this: Issuing ROLLBACK outside of a transaction block has no effect except emitting a warning. -- Bruce Momjian http://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 26, 2013 at 5:02 PM, Bruce Momjian 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 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 http://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
!Issuing ABORT outside of a transaction block has no effect.
--- 63,70
!Issuing ABORT outside of a transaction block emits a
!warning and has no effect.
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
Issuing ROLLBACK outside of a transaction
!block has no effect.
--- 60,66
Issuing ROLLBACK outside of a transaction
!block emits a warning and has no effect.
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 {
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again. This has no effect
! outside of a transaction block.
--- 110,117
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again. Issuing this
! outside of a transaction block emits a warning and has no effect.
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 |
This command only alters the behavior of constraints within the
!current transaction. This has no effect outside of a transaction block.
--- 99,106
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.
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
If SET TRANSACTION is executed without a prior
START TRANSACTION or BEGIN,
!it will have no effect.
--- 185,191
If SET TRANSACTION is executed without a prior
START TRANSACTION or BEGIN,
!it will emit a warning and have no effect.
--
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 11:22:39AM -0500, Tom Lane wrote: > Bruce Momjian writes: > > On Mon, Nov 25, 2013 at 10:04:19PM -0500, Robert Haas wrote: > >> But the documentation says: > >> > >> - Issuing ABORT when not inside a transaction does > >> - no harm, but it will provoke a warning message. > >> + Issuing ABORT 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 http://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 writes: > On Mon, Nov 25, 2013 at 10:04:19PM -0500, Robert Haas wrote: >> But the documentation says: >> >> - Issuing ABORT when not inside a transaction does >> - no harm, but it will provoke a warning message. >> + Issuing ABORT 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 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 http://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 10:04:19PM -0500, Robert Haas wrote: > On Mon, Nov 25, 2013 at 7:19 PM, Bruce Momjian 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 ABORT when not inside a transaction does > - no harm, but it will provoke a warning message. > + Issuing ABORT 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 http://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 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 ABORT when not inside a transaction does - no harm, but it will provoke a warning message. + Issuing ABORT 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 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 http://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 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 http://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 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 http://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
!Issuing ABORT when not inside a transaction does
!no harm, but it will provoke a warning message.
--- 63,69
!Issuing ABORT outside of a transaction block has no effect.
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
!Issuing ROLLBACK when not inside a transaction does
!no harm, but it will provoke a warning message.
--- 59,66
!Issuing ROLLBACK outside of a transaction
!block has no effect.
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 {
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again.
! PostgreSQL reports an error if
! SET LOCAL is used outside a transaction block.
--- 110,117
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again. This has no effect
! outside of a transaction block.
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 |
This command only alters the behavior of constraints within the
!current transaction. Thus, if you execute this command outside of a
!transaction block
!(BEGIN/COMMIT pair), it will
!generate an error.
--- 99,105
This command only alters the behavior of constraints within the
!current transaction. This has no effect outside of a transaction block.
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
If SET TRANSACTION is executed without a prior
START TRANSACTION or BEGIN,
!it will generate an error.
--- 185,191
If SET TRANSACTION is executed without a prior
START TRANSACTION or BEGIN,
!it will have no effect.
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
SubTransactionId mySubid,
SubTransactionId parentSubid);
static void CleanupTransaction(void);
+ static void CheckTransactionChain(bool isTopLevel, bool throwError,
+ const char *stmtType);
static void CommitTransaction(void);
static TransactionId RecordTransactionAbort(bool isSubXact);
static void StartTransaction(void);
*** PreventTransactionChain(bool isTopLevel,
*** 2949,2954
--- 2951,2976
}
/*
+ * These two functions allow for warnings or er
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
Robert Haas 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 Wed, Nov 20, 2013 at 04:31:12PM -0500, Robert Haas wrote: > On Wed, Nov 20, 2013 at 4:19 PM, Bruce Momjian 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 http://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 4:19 PM, Bruce Momjian 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 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 http://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
!Issuing ABORT when not inside a transaction does
!no harm, but it will provoke a warning message.
--- 63,69
!Issuing ABORT outside of a transaction block has no effect.
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
!Issuing ROLLBACK when not inside a transaction does
!no harm, but it will provoke a warning message.
--- 59,66
!Issuing ROLLBACK outside of a transaction
!block has no effect.
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 {
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again.
! PostgreSQL reports an error if
! SET LOCAL is used outside a transaction block.
--- 110,117
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again. This has no effect
! outside of a transaction block.
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 |
This command only alters the behavior of constraints within the
!current transaction. Thus, if you execute this command outside of a
!transaction block
!(BEGIN/COMMIT pair), it will
!generate an error.
--- 99,105
This command only alters the behavior of constraints within the
!current transaction. This has no effect outside of a transaction block.
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
If SET TRANSACTION is executed without a prior
START TRANSACTION or BEGIN,
!it will generate an error.
--- 185,191
If SET TRANSACTION is executed without a prior
START TRANSACTION or BEGIN,
!it will have no effect.
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
*** PreventTransactionChain(bool isTopLevel,
*** 2961,2972
* use of the current statement's results. Likewise subtransactions.
* Thus this is an inverse for PreventTransactionChain.
*
* isTopLevel: passed down from ProcessUtility to determine whether we are
* inside a function.
* stmtType: statement type name, for error messages.
*/
void
! RequireTransactionChain(bool isTopLevel, const char *stmtType)
{
/*
* xact block already started?
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 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 http://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 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 Tue, Nov 19, 2013 at 10:21:47PM -0500, Tom Lane wrote:
> Bruce Momjian 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 ABORT when not inside a transaction does
no harm, but it will provoke a warning message.
---
--
Bruce Momjian http://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 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
Andres Freund 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
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 http://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 http://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: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 http://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:14 PM, Bruce Momjian 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 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 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 http://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: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 1:05 PM, Bruce Momjian 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 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 http://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: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 Sat, Nov 9, 2013 at 02:15:52PM -0500, Robert Haas wrote:
> On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane wrote:
> > [ I'm so far behind ... ]
> >
> > Bruce Momjian 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 http://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 n
*** 179,187
created by this command can only be used within the current
transaction. Thus, DECLARE without WITH
HOLD is useless outside a transaction block: the cursor would
! survive only to the completion of the statement. Therefore
! PostgreSQL reports an error if such a
! command is used outside a transaction block.
Use
and
--- 179,185
created by this command can only be used within the current
transaction. Thus, DECLARE without WITH
HOLD is useless outside a transaction block: the cursor would
! survive only to the completion of the statement.
Use
and
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 ]
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
and
--- 168,174
LOCK TABLE is useless outside a transaction block: the lock
! would remain held only to the completion of the statement.
Use
and
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 6290c9d..aaaf186
*** a/doc/src/sgml/ref/set.sgml
--- b/doc/src/sgml/ref/set.sgml
*** SET [ SESSION | LOCAL ] TIME ZONE { COMMIT or ROLLBACK,
the session-level setting takes effect again.
! PostgreSQL reports an error if
SET LOCAL is used outside a transaction block.
--- 111,117
Specifies that the command takes effect for only the current
transaction. A
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 wrote: > > [ I'm so far behind ... ] > > > > Bruce Momjian 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 http://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 wrote: > [ I'm so far behind ... ] > > Bruce Momjian 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 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 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 http://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 Thu, Oct 3, 2013 at 8:32 PM, Bruce Momjian 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 Thu, Oct 3, 2013 at 8:35 PM, Andres Freund 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 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 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 http://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 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 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 http://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 {
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again. Note that
! SET LOCAL will appear to have no effect if it is
! executed outside a BEGIN block, since the
! transaction will end immediately.
--- 110,118
Specifies that the command takes effect for only the current
transaction. After COMMIT or ROLLBACK,
! the session-level setting takes effect again.
! PostgreSQL reports an error if
! SET LOCAL is used outside a transaction block.
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 | BEGIN/COMMIT pair), it will
!not appear to have any effect.
--- 102,108
current transaction. Thus, if you execute this command outside of a
transaction block
(BEGIN/COMMIT pair), it will
!generate an error.
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
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.
--- 184,191
If SET TRANSACTION is executed without a prior
!START TRANSACTION or BEGIN,
!it will generate an error.
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,
*
Re: [HACKERS] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
On Wed, Sep 25, 2013 at 2:55 AM, Bruce Momjian 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 http://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(VariableSetS
Re: [HACKERS] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
On Wed, Sep 11, 2013 at 4:19 AM, Bruce Momjian 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 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 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 http://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");
+
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 wrote: >> I think user should be aware of effect before using SET commands, as these >> are used at various levels (TRANSACTION, SESSION, ...). > Ideally, sure. But these kinds of mistakes are easy to make. You mean to say that after using SET Transaction, user can think below statements will use modified transaction property. But I think if user doesn't understand by default transaction will be committed after the statement execution, he might expect after few statements he can rollback. > That's why LOCK and DECLARE CURSOR already emit errors in this case - why > should this one be any different? IMO, I think error should be given when it is not possible to execute current statement. Not only LOCK,DECLARE CURSOR but SAVEPOINT and some other statements also give the same error, so if we want to throw error for such behavior, we can find all such similar statements (SET TRANSACTION, SET LOCAL, etc) and do it for all. This can be helpful to some users, but not sure if such behavior (statement can be executed but it will not have any sense) can be always detectable and maintaible. With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila 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 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
