BEGIN inside transaction should be an error

Hi

Yesterday I helped a guy on irc with a locking problem, he thought
that locking in postgresql was broken. It turned out that he had a PHP
function that he called inside his transaction and the function did BEGIN
and COMMIT. Since BEGIN inside a transaction is just a warning what
happend was that the inner COMMIT ended the transaction and
released the locks. The rest of his commands ran with autocommit
and no locks and he got broken data into the database.

Could we make BEGIN fail when we already are in a transaction?

Looking it up in the sql99 standard I find this:

"If a <start transaction statement> statement is executed when an
SQL-transaction is currently active, then an exception condition is
raised: invalid transaction state - active SQL-transaction."

/Dennis

Dennis Bjorklund <db(at)zigo(dot)dhs(dot)org> writes:
> Yesterday I helped a guy on irc with a locking problem, he thought
> that locking in postgresql was broken. It turned out that he had a PHP
> function that he called inside his transaction and the function did BEGIN
> and COMMIT. Since BEGIN inside a transaction is just a warning what
> happend was that the inner COMMIT ended the transaction and
> released the locks. The rest of his commands ran with autocommit
> and no locks and he got broken data into the database.

> Could we make BEGIN fail when we already are in a transaction?

We could, but it'd probably break about as many apps as it fixed.
I wonder whether php shouldn't be complaining about this, instead
--- doesn't php have its own ideas about controlling where the
transaction commit points are?

regards, tom lane

Tom Lane wrote:
> Dennis Bjorklund <db(at)zigo(dot)dhs(dot)org> writes:
>> Yesterday I helped a guy on irc with a locking problem, he thought
>> that locking in postgresql was broken. It turned out that he had a PHP
>> function that he called inside his transaction and the function did BEGIN
>> and COMMIT. Since BEGIN inside a transaction is just a warning what
>> happend was that the inner COMMIT ended the transaction and
>> released the locks. The rest of his commands ran with autocommit
>> and no locks and he got broken data into the database.
>
>> Could we make BEGIN fail when we already are in a transaction?
>
> We could, but it'd probably break about as many apps as it fixed.
> I wonder whether php shouldn't be complaining about this, instead
> --- doesn't php have its own ideas about controlling where the
> transaction commit points are?

There are no API calls to start/end transactions in php. However there
is a way to get the current transaction status:
http://de3.php.net/manual/en/function.pg-transaction-status.php

Also whatever decision is made one day PostGreSQL might want to
supported nested transactions similar to firebird.

regards,
Lukas

On 5/10/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dennis Bjorklund <db(at)zigo(dot)dhs(dot)org> writes:
> > Yesterday I helped a guy on irc with a locking problem, he thought
> > that locking in postgresql was broken. It turned out that he had a PHP
> > function that he called inside his transaction and the function did BEGIN
> > and COMMIT. Since BEGIN inside a transaction is just a warning what
> > happend was that the inner COMMIT ended the transaction and
> > released the locks. The rest of his commands ran with autocommit
> > and no locks and he got broken data into the database.
>
> > Could we make BEGIN fail when we already are in a transaction?
>
> We could, but it'd probably break about as many apps as it fixed.
> I wonder whether php shouldn't be complaining about this, instead
> --- doesn't php have its own ideas about controlling where the
> transaction commit points are?
>
> regards, tom lane
>

AFAIK php doesn't care about that... it just see for success or
failure conditions, so if postgres said everything is ok it will
continue...

--
Atentamente,
Jaime Casanova

"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs and the universe trying
to produce bigger and better idiots.
So far, the universe is winning."
Richard Cook

> We could, but it'd probably break about as many apps as it fixed.
> I wonder whether php shouldn't be complaining about this, instead
> --- doesn't php have its own ideas about controlling where the
> transaction commit points are?

All PHP does is when the connection is returned to the pool, if it is
still in a transaction, a rollback is issued.

The guy needs to do his own tracking of transaction state if he wants to
avoid these problems...

Chris

Am Mittwoch, 10. Mai 2006 08:19 schrieb Tom Lane:
> Dennis Bjorklund <db(at)zigo(dot)dhs(dot)org> writes:
> > Yesterday I helped a guy on irc with a locking problem, he thought
> > that locking in postgresql was broken. It turned out that he had a PHP
> > function that he called inside his transaction and the function did BEGIN
> > and COMMIT. Since BEGIN inside a transaction is just a warning what
> > happend was that the inner COMMIT ended the transaction and
> > released the locks. The rest of his commands ran with autocommit
> > and no locks and he got broken data into the database.
> >
> > Could we make BEGIN fail when we already are in a transaction?
>
> We could, but it'd probably break about as many apps as it fixed.
> I wonder whether php shouldn't be complaining about this, instead
> --- doesn't php have its own ideas about controlling where the
> transaction commit points are?

In fact it would break many application, so it should be at least controllable
by a setting or GUC.

On Wed, May 10, 2006 at 09:41:46AM +0200, Mario Weilguni wrote:
> > > Could we make BEGIN fail when we already are in a transaction?
> >
> > We could, but it'd probably break about as many apps as it fixed.
> > I wonder whether php shouldn't be complaining about this, instead
> > --- doesn't php have its own ideas about controlling where the
> > transaction commit points are?
>
> In fact it would break many application, so it should be at least controllable
> by a setting or GUC.

You want to make a GUC that makes:

BEGIN;
BEGIN;

Leave you with an aborted transaction? That seems like a singularly
useless feature...

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Am Mittwoch, 10. Mai 2006 09:41 schrieb Mario Weilguni:
> Am Mittwoch, 10. Mai 2006 08:19 schrieb Tom Lane:
> > Dennis Bjorklund <db(at)zigo(dot)dhs(dot)org> writes:
> > > Yesterday I helped a guy on irc with a locking problem, he thought
> > > that locking in postgresql was broken. It turned out that he had a PHP
> > > function that he called inside his transaction and the function did
> > > BEGIN and COMMIT. Since BEGIN inside a transaction is just a warning
> > > what happend was that the inner COMMIT ended the transaction and
> > > released the locks. The rest of his commands ran with autocommit
> > > and no locks and he got broken data into the database.
> > >
> > > Could we make BEGIN fail when we already are in a transaction?
> >
> > We could, but it'd probably break about as many apps as it fixed.
> > I wonder whether php shouldn't be complaining about this, instead
> > --- doesn't php have its own ideas about controlling where the
> > transaction commit points are?
>
> In fact it would break many application, so it should be at least
> controllable by a setting or GUC.
>

No, I want that there is a setting or GUC that controls whether an error or a
warning is raised when "begin" is executed within a transaction. I know of
several php database wrappers that will be seriously broken when errors are
raised...

Am Mittwoch, 10. Mai 2006 10:10 schrieb Martijn van Oosterhout:
> You want to make a GUC that makes:
>
> BEGIN;
> BEGIN;
>
> Leave you with an aborted transaction? That seems like a singularly
> useless feature...

If a command doesn't do what it is supposed to do, then it should be an error.
That seems like a throroughly useful feature to me.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

--On Mittwoch, Mai 10, 2006 10:14:22 +0200 Mario Weilguni
<mweilguni(at)sime(dot)com> wrote:

> No, I want that there is a setting or GUC that controls whether an error
> or a warning is raised when "begin" is executed within a transaction. I
> know of several php database wrappers that will be seriously broken when
> errors are raised...

Such a behavior is already broken by design. I think it's not desirable to
blindly do
transaction start or commit without tracking the current transaction state.
So these wrappers
need to be fixed first.

--

Bernd

Am Mittwoch, 10. Mai 2006 10:59 schrieb Peter Eisentraut:
> Am Mittwoch, 10. Mai 2006 10:10 schrieb Martijn van Oosterhout:
> > You want to make a GUC that makes:
> >
> > BEGIN;
> > BEGIN;
> >
> > Leave you with an aborted transaction? That seems like a singularly
> > useless feature...
>
> If a command doesn't do what it is supposed to do, then it should be an
> error. That seems like a throroughly useful feature to me.

Maybe. I just want to emphasize that it will break existing applications.

Am Mittwoch, 10. Mai 2006 11:44 schrieb Bernd Helmle:
> --On Mittwoch, Mai 10, 2006 10:14:22 +0200 Mario Weilguni
>
> <mweilguni(at)sime(dot)com> wrote:
> > No, I want that there is a setting or GUC that controls whether an error
> > or a warning is raised when "begin" is executed within a transaction. I
> > know of several php database wrappers that will be seriously broken when
> > errors are raised...
>
> Such a behavior is already broken by design. I think it's not desirable to
> blindly do
> transaction start or commit without tracking the current transaction state.
> So these wrappers
> need to be fixed first.

You mean broken like "transform_null_equals"? Or "add_missing_from"?

--On Mittwoch, Mai 10, 2006 12:36:07 +0200 Mario Weilguni
<mweilguni(at)sime(dot)com> wrote:

>> Such a behavior is already broken by design. I think it's not desirable
>> to blindly do
>> transaction start or commit without tracking the current transaction
>> state. So these wrappers
>> need to be fixed first.
>
> You mean broken like "transform_null_equals"? Or "add_missing_from"?

You missed my point. I don't say that such a GUC won't be useful, but
applications which
don't care about what they are currently doing with a database are broken.

--

Bernd

Peter Eisentraut skrev:
> Am Mittwoch, 10. Mai 2006 10:10 schrieb Martijn van Oosterhout:
>
>> You want to make a GUC that makes:
>>
>> BEGIN;
>> BEGIN;
>>
>> Leave you with an aborted transaction? That seems like a singularly
>> useless feature...
>>
>
> If a command doesn't do what it is supposed to do, then it should be an error.
> That seems like a throroughly useful feature to me.
>
>
And it would follow sql99 that demand an error. I'm surprised
everyone seems to ignore that part (except maybe Peter who is the
one I happend to reply to :-).

A guc that people can turn off if they have old broken code, that
would work for me.

/Dennis

I dont think anyone is arguing that such an application is not
broken. We should see how we can stop a developer from writing buggy
code.

IMO, such a GUC variable _should_ be created and turned on by default.

In case an application fails, at the least, the developer knows
that his application is broken; then he can choose to turn off the GUC
variable to let the old behaviour prevail (he might want to do this to
let a production env. continue).

In the absence of such a feature, we are encouraging developers to
write buggy code. This GUC variable can be removed and the behaviour
can be made default over the next couple of releases.

My two paise...

On 5/10/06, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>
>
> --On Mittwoch, Mai 10, 2006 12:36:07 +0200 Mario Weilguni
> <mweilguni(at)sime(dot)com> wrote:
>
> >> Such a behavior is already broken by design. I think it's not desirable
> >> to blindly do
> >> transaction start or commit without tracking the current transaction
> >> state. So these wrappers
> >> need to be fixed first.
> >
> > You mean broken like "transform_null_equals"? Or "add_missing_from"?
>
> You missed my point. I don't say that such a GUC won't be useful, but
> applications which
> don't care about what they are currently doing with a database are broken.
>
>
> --
>
> Bernd
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
>

I would suggest the guy simply use the popular ADODB package for his
database abstraction layer so he can make use of its "Smart Transaction"
feature.

http://phplens.com/lens/adodb/docs-adodb.htm#ex11

<quote>
Lastly, StartTrans/CompleteTrans is nestable, and only the outermost
block is executed. In contrast, BeginTrans/CommitTrans/RollbackTrans is
NOT nestable.

$conn->StartTrans();
$conn->Execute($sql);
$conn->StartTrans(); # ignored <--------------
if (!CheckRecords()) $conn->FailTrans();
$conn->CompleteTrans(); # ignored <--------------
$conn->Execute($Sql2);
$conn->CompleteTrans();
</quote>

The commands marked "ignored" aren't really ignored, since it keeps
track of what level the transactions are nested to, and won't actually
commit the transaction until the StartTrans() calls == CompleteTrans()
calls.

Its worked great for me for many years now.

On Wed, 2006-05-10 at 06:19 +0200, Dennis Bjorklund wrote:
> Hi
>
> Yesterday I helped a guy on irc with a locking problem, he thought
> that locking in postgresql was broken. It turned out that he had a PHP
> function that he called inside his transaction and the function did BEGIN
> and COMMIT. Since BEGIN inside a transaction is just a warning what
> happend was that the inner COMMIT ended the transaction and
> released the locks. The rest of his commands ran with autocommit
> and no locks and he got broken data into the database.
>
> Could we make BEGIN fail when we already are in a transaction?
>
> Looking it up in the sql99 standard I find this:
>
> "If a <start transaction statement> statement is executed when an
> SQL-transaction is currently active, then an exception condition is
> raised: invalid transaction state - active SQL-transaction."
>
> /Dennis
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend
--
Mike Benoit <ipso(at)snappymail(dot)ca>

Martijn van Oosterhout wrote:
> On Wed, May 10, 2006 at 09:41:46AM +0200, Mario Weilguni wrote:
>
>>>>Could we make BEGIN fail when we already are in a transaction?
>>>
>>>We could, but it'd probably break about as many apps as it fixed.
>>>I wonder whether php shouldn't be complaining about this, instead
>>>--- doesn't php have its own ideas about controlling where the
>>>transaction commit points are?
>>
>>In fact it would break many application, so it should be at least controllable
>>by a setting or GUC.
>
>
> You want to make a GUC that makes:
>
> BEGIN;
> BEGIN;
>
> Leave you with an aborted transaction? That seems like a singularly
> useless feature...
>
> Have a nice day,

Or if you really want to screw things up, you could require COMMIT; COMMIT; to
finish off the transaction started by BEGIN; BEGIN; We could just silently keep
the transaction alive after the first COMMIT; ;)

On Wed, May 10, 2006 at 12:31:52PM +0200, Mario Weilguni wrote:
> Am Mittwoch, 10. Mai 2006 10:59 schrieb Peter Eisentraut:
> > Am Mittwoch, 10. Mai 2006 10:10 schrieb Martijn van Oosterhout:
> > > You want to make a GUC that makes:
> > >
> > > BEGIN;
> > > BEGIN;
> > >
> > > Leave you with an aborted transaction? That seems like a singularly
> > > useless feature...
> >
> > If a command doesn't do what it is supposed to do, then it should be an
> > error. That seems like a throroughly useful feature to me.
>
> Maybe. I just want to emphasize that it will break existing applications.

If the existing application is trying to start a new transaction from
within an existing one, I'd say it's already broken and we're just
hiding that fact.
--
Jim C. Nasby, Sr. Engineering Consultant jnasby(at)pervasive(dot)com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

On Wed, May 10, 2006 at 04:03:51PM -0500, Jim C. Nasby wrote:
> On Wed, May 10, 2006 at 12:31:52PM +0200, Mario Weilguni wrote:
> > Maybe. I just want to emphasize that it will break existing applications.
>
> If the existing application is trying to start a new transaction from
> within an existing one, I'd say it's already broken and we're just
> hiding that fact.

Well maybe, except the extra BEGIN is harmless. I'm thinking of the
situation where a connection library sends a BEGIN on startup because
it wants to emulate a non-autocommit mode. The application then
proceeds to handle transactions itself, sending another BEGIN and going
from there.

We'll have just broken this perfectly working application because it
failed the purity test. The backward compatability issues are huge and
it doesn't actually bring any benefits.

How do other database deal with this? Either they nest BEGIN/COMMIT or
they probably throw an error without aborting the transaction, which is
pretty much what we do. Is there a database that actually aborts a
whole transaction just for an extraneous begin?

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> How do other database deal with this? Either they nest BEGIN/COMMIT or
> they probably throw an error without aborting the transaction, which is
> pretty much what we do. Is there a database that actually aborts a
> whole transaction just for an extraneous begin?

Probably not. The SQL99 spec does say (in describing START TRANSACTION,
which is the standard spelling of BEGIN)

1) If a <start transaction statement> statement is executed when an
SQL-transaction is currently active, then an exception condition
is raised: invalid transaction state - active SQL-transaction.

*However*, they are almost certainly expecting that that condition only
causes the START command to be ignored; not that it should bounce the
whole transaction. So I think the argument that this is required by
the spec is a bit off base.

regards, tom lane

On 5/10/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > How do other database deal with this? Either they nest BEGIN/COMMIT or
> > they probably throw an error without aborting the transaction, which is
> > pretty much what we do. Is there a database that actually aborts a
> > whole transaction just for an extraneous begin?
>
> Probably not. The SQL99 spec does say (in describing START TRANSACTION,
> which is the standard spelling of BEGIN)
>
> 1) If a <start transaction statement> statement is executed when an
> SQL-transaction is currently active, then an exception condition
> is raised: invalid transaction state - active SQL-transaction.
>
> *However*, they are almost certainly expecting that that condition only
> causes the START command to be ignored; not that it should bounce the
> whole transaction. So I think the argument that this is required by
> the spec is a bit off base.
>
> regards, tom lane
>

Well, actually informix throw an error... at least, my 4gl programs
always abort when a second "begin work" is found inside a
transaction...

--
regards,
Jaime Casanova

"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs and the universe trying
to produce bigger and better idiots.
So far, the universe is winning."
Richard Cook

Tom Lane skrev:
> The SQL99 spec does say (in describing START TRANSACTION,
> which is the standard spelling of BEGIN)
>
> 1) If a <start transaction statement> statement is executed when an
> SQL-transaction is currently active, then an exception condition
> is raised: invalid transaction state - active SQL-transaction.
>
> *However*, they are almost certainly expecting that that condition only
> causes the START command to be ignored; not that it should bounce the
> whole transaction.

What is the definition of an "exception condition"?

I thought that it ment that a transaction should fail and that
"completion condition" are
used for warnings that doesn't abort transactions. As an example I
looked up division
by zero in sql99 and it say this:

"If the value of a divisor is zero, then an exception condition
is raised: data exception - division by zero."

Do you mean that some exception conditions fail transactions and some
doesn't?

/Dennis

Am Mittwoch, 10. Mai 2006 22:23 schrieb Mark Dilger:
> Martijn van Oosterhout wrote:
> > On Wed, May 10, 2006 at 09:41:46AM +0200, Mario Weilguni wrote:
> >>>>Could we make BEGIN fail when we already are in a transaction?
...
>
> Or if you really want to screw things up, you could require COMMIT; COMMIT;
> to finish off the transaction started by BEGIN; BEGIN; We could just
> silently keep the transaction alive after the first COMMIT; ;)
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

I would expect after a COMMIT without an error, that my transaction is
committed. When the system accidently issued a second BEGIN, this would not
be the case.

And what about BEGIN; BEGIN; ROLLBACK; COMMIT; then? Should the rollback be
ignored also?

I'd vote for breaking broken applications and leave the database-administrator
reactivate this currently broken behavior of postgresql via GUC.

Tommi

On 5/11/06, Martijn van Oosterhout <kleptog(at)svana(dot)org> wrote:
> On Wed, May 10, 2006 at 04:03:51PM -0500, Jim C. Nasby wrote:
> > If the existing application is trying to start a new transaction from
> > within an existing one, I'd say it's already broken and we're just
> > hiding that fact.
>
> Well maybe, except the extra BEGIN is harmless.

It _not_ harmless as it will be probably followed by 'extra' commit.
Those few cases where it does not happen do not matter in light
of cases where it happens.

--
marko

On Thu, May 11, 2006 at 08:05:57AM +0200, Tommi Maekitalo wrote:
> I'd vote for breaking broken applications and leave the database-administrator
> reactivate this currently broken behavior of postgresql via GUC.

+1...

As for whether this should or shouldn't abort the current transaction,
I'd argue that it should. Otherwise it's likely that your first commit
is actually bogus, which means you just hosed yourself.
--
Jim C. Nasby, Sr. Engineering Consultant jnasby(at)pervasive(dot)com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

On Wed, 2006-05-10 at 21:24 -0400, Tom Lane wrote:
> Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > How do other database deal with this? Either they nest BEGIN/COMMIT or
> > they probably throw an error without aborting the transaction, which is
> > pretty much what we do. Is there a database that actually aborts a
> > whole transaction just for an extraneous begin?
>
> Probably not. The SQL99 spec does say (in describing START TRANSACTION,
> which is the standard spelling of BEGIN)
>
> 1) If a <start transaction statement> statement is executed when an
> SQL-transaction is currently active, then an exception condition
> is raised: invalid transaction state - active SQL-transaction.
>
> *However*, they are almost certainly expecting that that condition only
> causes the START command to be ignored; not that it should bounce the
> whole transaction. So I think the argument that this is required by
> the spec is a bit off base.

If you interpret the standard that way then the correct behaviour in the
face of *any* exception condition should be *not* abort the transaction.
In PostgreSQL, all exception conditions do abort the transaction, so why
not this one? Why would we special-case this?

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Am Donnerstag, 11. Mai 2006 22:16 schrieb Simon Riggs:
> On Wed, 2006-05-10 at 21:24 -0400, Tom Lane wrote:
> > Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > > How do other database deal with this? Either they nest BEGIN/COMMIT or
> > > they probably throw an error without aborting the transaction, which is
> > > pretty much what we do. Is there a database that actually aborts a
> > > whole transaction just for an extraneous begin?
> >
> > Probably not. The SQL99 spec does say (in describing START TRANSACTION,
> > which is the standard spelling of BEGIN)
> >
> > 1) If a <start transaction statement> statement is executed when
> > an SQL-transaction is currently active, then an exception condition is
> > raised: invalid transaction state - active SQL-transaction.
> >
> > *However*, they are almost certainly expecting that that condition only
> > causes the START command to be ignored; not that it should bounce the
> > whole transaction. So I think the argument that this is required by
> > the spec is a bit off base.
>
> If you interpret the standard that way then the correct behaviour in the
> face of *any* exception condition should be *not* abort the transaction.
> In PostgreSQL, all exception conditions do abort the transaction, so why
> not this one? Why would we special-case this?

IMO it's ok to raise an exception - if this is configurable for at least one
releasy cycle - giving developers time to fix applications. It's no good
behaviour to change something like this without any (at least time-limited )
backward compatible option.

regards
mario weilguni

On 5/12/06, Mario Weilguni <mweilguni(at)sime(dot)com> wrote:
> Am Donnerstag, 11. Mai 2006 22:16 schrieb Simon Riggs:
> > On Wed, 2006-05-10 at 21:24 -0400, Tom Lane wrote:
> > > Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > > > How do other database deal with this? Either they nest BEGIN/COMMIT or
> > > > they probably throw an error without aborting the transaction, which is
> > > > pretty much what we do. Is there a database that actually aborts a
> > > > whole transaction just for an extraneous begin?
> > >
> > > Probably not. The SQL99 spec does say (in describing START TRANSACTION,
> > > which is the standard spelling of BEGIN)
> > >
> > > 1) If a <start transaction statement> statement is executed when
> > > an SQL-transaction is currently active, then an exception condition is
> > > raised: invalid transaction state - active SQL-transaction.
> > >
> > > *However*, they are almost certainly expecting that that condition only
> > > causes the START command to be ignored; not that it should bounce the
> > > whole transaction. So I think the argument that this is required by
> > > the spec is a bit off base.
> >
> > If you interpret the standard that way then the correct behaviour in the
> > face of *any* exception condition should be *not* abort the transaction.
> > In PostgreSQL, all exception conditions do abort the transaction, so why
> > not this one? Why would we special-case this?
>
> IMO it's ok to raise an exception - if this is configurable for at least one
> releasy cycle - giving developers time to fix applications. It's no good
> behaviour to change something like this without any (at least time-limited )
> backward compatible option.
>

if an option to change it is put in place, maybe it will be there
forever (with a different default behavior)...

i am all in favor of a second begin to throw an exception "already in
transaction" or something else
(http://archives.postgresql.org/pgsql-hackers/2005-12/msg00813.php),
but if we do it we should do it the only behavior... i don't think
it's good to introduce a new GUC for that things (we will finish with
GUCs to turn off every fix)

--
regards,
Jaime Casanova

"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs and the universe trying
to produce bigger and better idiots.
So far, the universe is winning."
Richard Cook

Added to TODO:

* Add a GUC to control whether BEGIN inside a transcation should abort
the transaction.

---------------------------------------------------------------------------

Jaime Casanova wrote:
> On 5/12/06, Mario Weilguni <mweilguni(at)sime(dot)com> wrote:
> > Am Donnerstag, 11. Mai 2006 22:16 schrieb Simon Riggs:
> > > On Wed, 2006-05-10 at 21:24 -0400, Tom Lane wrote:
> > > > Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > > > > How do other database deal with this? Either they nest BEGIN/COMMIT or
> > > > > they probably throw an error without aborting the transaction, which is
> > > > > pretty much what we do. Is there a database that actually aborts a
> > > > > whole transaction just for an extraneous begin?
> > > >
> > > > Probably not. The SQL99 spec does say (in describing START TRANSACTION,
> > > > which is the standard spelling of BEGIN)
> > > >
> > > > 1) If a <start transaction statement> statement is executed when
> > > > an SQL-transaction is currently active, then an exception condition is
> > > > raised: invalid transaction state - active SQL-transaction.
> > > >
> > > > *However*, they are almost certainly expecting that that condition only
> > > > causes the START command to be ignored; not that it should bounce the
> > > > whole transaction. So I think the argument that this is required by
> > > > the spec is a bit off base.
> > >
> > > If you interpret the standard that way then the correct behaviour in the
> > > face of *any* exception condition should be *not* abort the transaction.
> > > In PostgreSQL, all exception conditions do abort the transaction, so why
> > > not this one? Why would we special-case this?
> >
> > IMO it's ok to raise an exception - if this is configurable for at least one
> > releasy cycle - giving developers time to fix applications. It's no good
> > behaviour to change something like this without any (at least time-limited )
> > backward compatible option.
> >
>
> if an option to change it is put in place, maybe it will be there
> forever (with a different default behavior)...
>
> i am all in favor of a second begin to throw an exception "already in
> transaction" or something else
> (http://archives.postgresql.org/pgsql-hackers/2005-12/msg00813.php),
> but if we do it we should do it the only behavior... i don't think
> it's good to introduce a new GUC for that things (we will finish with
> GUCs to turn off every fix)
>
> --
> regards,
> Jaime Casanova
>
> "Programming today is a race between software engineers striving to
> build bigger and better idiot-proof programs and the universe trying
> to produce bigger and better idiots.
> So far, the universe is winning."
> Richard Cook
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly
>

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

I have made the changes to implement this TODO item. I wish to
know the standard procedure (command) to generate a patch; and from
which level in the source directory should I execute it?

On 5/18/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
>
> Added to TODO:
>
> * Add a GUC to control whether BEGIN inside a transcation should abort
> the transaction.
>
>

Gurjeet Singh wrote:
> I have made the changes to implement this TODO item. I wish to
> know the standard procedure (command) to generate a patch; and from
> which level in the source directory should I execute it?

The toplevel directory. The command is

cvs -q diff -cp

If you created new files to implement a patch, include them separately,
indicating in the patch description in which directory they are meant to
reside.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Please read the developers FAQ:
http://www.postgresql.org/docs/faqs.FAQ_DEV.html

For the most part, patches are probably best generated relative to the
root directory of your CVS checkout.

cheers

andrew

Gurjeet Singh wrote:
> I have made the changes to implement this TODO item. I wish to
> know the standard procedure (command) to generate a patch; and from
> which level in the source directory should I execute it?
>
> On 5/18/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
>>
>> Added to TODO:
>>
>> * Add a GUC to control whether BEGIN inside a transcation
>> should abort
>> the transaction.
>>

On 5/26/06, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Gurjeet Singh wrote:
> > I wish to
> > know the standard procedure (command) to generate a patch; and from
> > which level in the source directory should I execute it?
>
> The toplevel directory. The command is
>
> cvs -q diff -cp
>
> If you created new files to implement a patch, include them separately,
> indicating in the patch description in which directory they are meant to
> reside.

Thanks.

Here's the patch:

Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.220
diff -c -p -r1.220 xact.c
*** src/backend/access/transam/xact.c 25 Apr 2006 00:25:17 -0000 1.220
--- src/backend/access/transam/xact.c 21 May 2006 15:40:00 -0000
*************** bool XactReadOnly;
*** 59,64 ****
--- 59,65 ----
int CommitDelay = 0; /* precommit delay in microseconds */
int CommitSiblings = 5; /* # concurrent xacts needed to sleep */

+ bool BeginInXactIsError = true;

/*
* transaction states - transaction state from server perspective
*************** BeginTransactionBlock(void)
*** 2725,2731 ****
case TBLOCK_SUBINPROGRESS:
case TBLOCK_ABORT:
case TBLOCK_SUBABORT:
! ereport(WARNING,
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
errmsg("there is already a transaction in progress")));
break;
--- 2726,2732 ----
case TBLOCK_SUBINPROGRESS:
case TBLOCK_ABORT:
case TBLOCK_SUBABORT:
! ereport(ERROR,
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
errmsg("there is already a transaction in progress")));
break;
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.318
diff -c -p -r1.318 guc.c
*** src/backend/utils/misc/guc.c 2 May 2006 11:28:55 -0000 1.318
--- src/backend/utils/misc/guc.c 21 May 2006 15:14:22 -0000
***************
*** 65,70 ****
--- 65,71 ----
#include "utils/pg_locale.h"
#include "pgstat.h"
#include "access/gin.h"
+ #include "access/xact.h"

#ifndef PG_KRB_SRVTAB
#define PG_KRB_SRVTAB ""
*************** static struct config_bool ConfigureNames
*** 1005,1010 ****
--- 1006,1021 ----
false, NULL, NULL
},

+ {
+ {"begin_inside_transaction_is_error", PGC_USERSET, COMPAT_OPTIONS,
+ gettext_noop("A BEGIN statement inside a transaction raises ERROR."),
+ gettext_noop("It is provided to catch buggy applications. "
+ "Disable it to let the buggy application run.")
+ },
+ &BeginInXactIsError,
+ true, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL
Index: src/include/access/xact.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/access/xact.h,v
retrieving revision 1.82
diff -c -p -r1.82 xact.h
*** src/include/access/xact.h 25 Apr 2006 00:25:19 -0000 1.82
--- src/include/access/xact.h 21 May 2006 15:13:10 -0000
*************** extern int XactIsoLevel;
*** 41,46 ****
--- 41,49 ----
extern bool DefaultXactReadOnly;
extern bool XactReadOnly;

+ /* A BEGIN statement inside a transaction raises ERROR */
+ extern bool BeginInXactIsError;
+
/*
* start- and end-of-transaction callbacks for dynamically loaded modules
*/

On 5/26/06, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> Please read the developers FAQ:
> http://www.postgresql.org/docs/faqs.FAQ_DEV.html
>
> For the most part, patches are probably best generated relative to the
> root directory of your CVS checkout.
>
> cheers
>
> andrew
>
> Gurjeet Singh wrote:
> > I wish to
> > know the standard procedure (command) to generate a patch; and from
> > which level in the source directory should I execute it?
> >
> > On 5/18/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> >>
> >> Added to TODO:
> >>
> >> * Add a GUC to control whether BEGIN inside a transcation
> >> should abort
> >> the transaction.

Thanks Andrew.

Reposting the patch since my version on guc.c wasn't at the HEAD.

Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.220
diff -c -p -r1.220 xact.c
*** src/backend/access/transam/xact.c 25 Apr 2006 00:25:17 -0000 1.220
--- src/backend/access/transam/xact.c 21 May 2006 15:40:00 -0000
*************** bool XactReadOnly;
*** 59,64 ****
--- 59,65 ----
int CommitDelay = 0; /* precommit delay in microseconds */
int CommitSiblings = 5; /* # concurrent xacts needed to sleep */

+ bool BeginInXactIsError = true;

/*
* transaction states - transaction state from server perspective
*************** BeginTransactionBlock(void)
*** 2725,2731 ****
case TBLOCK_SUBINPROGRESS:
case TBLOCK_ABORT:
case TBLOCK_SUBABORT:
! ereport(WARNING,
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
errmsg("there is already a transaction in progress")));
break;
--- 2726,2732 ----
case TBLOCK_SUBINPROGRESS:
case TBLOCK_ABORT:
case TBLOCK_SUBABORT:
! ereport(ERROR,
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
errmsg("there is already a transaction in progress")));
break;
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.320
diff -c -p -r1.320 guc.c
*** src/backend/utils/misc/guc.c 21 May 2006 20:10:42 -0000 1.320
--- src/backend/utils/misc/guc.c 26 May 2006 16:10:40 -0000
***************
*** 66,71 ****
--- 66,72 ----
#include "utils/pg_locale.h"
#include "pgstat.h"
#include "access/gin.h"
+ #include "access/xact.h"

#ifndef PG_KRB_SRVTAB
#define PG_KRB_SRVTAB ""
*************** static struct config_bool ConfigureNames
*** 1008,1013 ****
--- 1009,1024 ----
false, NULL, NULL
},

+ {
+ {"begin_inside_transaction_is_error", PGC_USERSET, COMPAT_OPTIONS,
+ gettext_noop("A BEGIN statement inside a transaction raises ERROR."),
+ gettext_noop("It is provided to catch buggy applications. "
+ "Disable it to let the buggy application run.")
+ },
+ &BeginInXactIsError,
+ true, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL
Index: src/include/access/xact.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/access/xact.h,v
retrieving revision 1.82
diff -c -p -r1.82 xact.h
*** src/include/access/xact.h 25 Apr 2006 00:25:19 -0000 1.82
--- src/include/access/xact.h 21 May 2006 15:13:10 -0000
*************** extern int XactIsoLevel;
*** 41,46 ****
--- 41,49 ----
extern bool DefaultXactReadOnly;
extern bool XactReadOnly;

+ /* A BEGIN statement inside a transaction raises ERROR */
+ extern bool BeginInXactIsError;
+
/*
* start- and end-of-transaction callbacks for dynamically loaded modules
*/

Gurjeet Singh wrote:

> *************** BeginTransactionBlock(void)
> *** 2725,2731 ****
> case TBLOCK_SUBINPROGRESS:
> case TBLOCK_ABORT:
> case TBLOCK_SUBABORT:
> ! ereport(WARNING,
> (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
> errmsg("there is already a
> transaction in progress")));
> break;
> --- 2726,2732 ----
> case TBLOCK_SUBINPROGRESS:
> case TBLOCK_ABORT:
> case TBLOCK_SUBABORT:
> ! ereport(ERROR,
> (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
> errmsg("there is already a
> transaction in progress")));
> break;

This should depend on the GUC variable for the patch to work at all ...

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

"Gurjeet Singh" <singh(dot)gurjeet(at)gmail(dot)com> writes:
> Here's the patch:

Wrong default (there was no consensus for changing the default behavior,
and you need to tone down the description strings). A less verbose
GUC variable name would be a good idea, too. Something like
"error_double_begin" would be more in keeping with most of our other names.

Doesn't actually *honor* the GUC variable, just changes the behavior
outright. This betrays a certain lack of testing.

Also, lacks documentation. Please grep the tree for all references to
some existing GUC variable(s) to see what you missed.

regards, tom lane