Re: TODO: Add a GUC to control whether BEGIN inside

Hello,

Is this really a TODO or is this someone being overzealous with the TODO
list?

Joshua D. Drake

--

=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997
http://www.commandprompt.com/

Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Joshua D. Drake wrote:
> Hello,
>
> Is this really a TODO or is this someone being overzealous with the TODO
> list?

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

Well, right now, BEGIN inside a transaction just issues a warning:

test=> BEGIN;
BEGIN
test=> BEGIN;
WARNING: there is already a transaction in progress
BEGIN
test=> SELECT 1;
?column?
----------
1
(1 row)

test=> COMMIT;
COMMIT

I think you can make the case that this should be an error, or at least
that's how it got on the TODO list. I can always remove it if people
don't want the item completed.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

> 1
> (1 row)
>
> test=> COMMIT;
> COMMIT
>
> I think you can make the case that this should be an error, or at least
> that's how it got on the TODO list. I can always remove it if people
> don't want the item completed.

Well I can tell you that my customers who are postgresql users ;) would
howl in fury if we did that. They are already significantly irritated
that certain errors are so strict. E.g.,

postgres=# BEGIN;
BEGIN
postgres=# ALTER TABLE baz ADD COLUMN bar text;
ERROR: relation "baz" does not exist
postgres=# SELECT * FROM foo;
ERROR: current transaction is aborted, commands ignored until end of
transaction block

You do not need to argue with me about the purpose :), I understand why
it is just really frustrating for many users.

I would say that a GUC variable for such behavior as listed in the TODO
is overzealous. We should either enforce it, or not. As we do not now,
there is no reason imo to change it.

Sincerely,

Joshua D. Drake

--

=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997
http://www.commandprompt.com/

Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

"Joshua D. Drake" <jd(at)commandprompt(dot)com> writes:
> I would say that a GUC variable for such behavior as listed in the TODO
> is overzealous. We should either enforce it, or not. As we do not now,
> there is no reason imo to change it.

Not only is it overzealous, but the proposal to have one reflects a
failure to learn from history. GUC variables that change
transaction-boundary semantics are a bad idea, period: see autocommit.

regards, tom lane

On Thu, 2006-12-28 at 13:52 -0500, Tom Lane wrote:
> "Joshua D. Drake" <jd(at)commandprompt(dot)com> writes:
> > I would say that a GUC variable for such behavior as listed in the TODO
> > is overzealous. We should either enforce it, or not. As we do not now,
> > there is no reason imo to change it.
>
> Not only is it overzealous, but the proposal to have one reflects a
> failure to learn from history. GUC variables that change
> transaction-boundary semantics are a bad idea, period: see autocommit.

Nod. Let's get this TODO removed.

Sincerely,

Joshua D. Drake

>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
>
--

=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997
http://www.commandprompt.com/

Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Joshua D. Drake wrote:
> On Thu, 2006-12-28 at 13:52 -0500, Tom Lane wrote:
> > "Joshua D. Drake" <jd(at)commandprompt(dot)com> writes:
> > > I would say that a GUC variable for such behavior as listed in the TODO
> > > is overzealous. We should either enforce it, or not. As we do not now,
> > > there is no reason imo to change it.
> >
> > Not only is it overzealous, but the proposal to have one reflects a
> > failure to learn from history. GUC variables that change
> > transaction-boundary semantics are a bad idea, period: see autocommit.
>
> Nod. Let's get this TODO removed.

OK, removed.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

On Thursday 28 December 2006 15:44, Bruce Momjian wrote:
> Joshua D. Drake wrote:
> > On Thu, 2006-12-28 at 13:52 -0500, Tom Lane wrote:
> > > "Joshua D. Drake" <jd(at)commandprompt(dot)com> writes:
> > > > I would say that a GUC variable for such behavior as listed in the
> > > > TODO is overzealous. We should either enforce it, or not. As we do
> > > > not now, there is no reason imo to change it.
> > >
> > > Not only is it overzealous, but the proposal to have one reflects a
> > > failure to learn from history. GUC variables that change
> > > transaction-boundary semantics are a bad idea, period: see autocommit.
> >
> > Nod. Let's get this TODO removed.
>
> OK, removed.

I thought this was needed for spec compliance? If we have no plans to even
attempt to support it, istm that ought to be noted someplace.

--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL

Robert Treat <xzilla(at)users(dot)sourceforge(dot)net> writes:
> I thought this was needed for spec compliance?

BEGIN isn't in the spec at all ...

Now you could point to the spec for START TRANSACTION, which saith

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, seeing that the spec doesn't think that an exception necessarily
aborts the transaction, I think the case for saying that ERROR is more
spec-compliant here than WARNING is mighty thin.

And if you want to hold our feet to the fire about whether errors abort
transactions or not, this particular point is not one thousandth part
of what would need to change.

regards, tom lane

Am Donnerstag, 28. Dezember 2006 19:52 schrieb Tom Lane:
> Not only is it overzealous, but the proposal to have one reflects a
> failure to learn from history. GUC variables that change
> transaction-boundary semantics are a bad idea, period: see autocommit.

But this option would not, in fact, change the transaction-boundary semantics.

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

Am Donnerstag, 28. Dezember 2006 18:57 schrieb Bruce Momjian:
> I think you can make the case that this should be an error, or at least
> that's how it got on the TODO list. I can always remove it if people
> don't want the item completed.

The reason this was added is that modular applications expect that a locally
issued BEGIN ... COMMIT executes a transaction, whereas now you don't know
what you're getting. I think this change would have merit.

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

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Am Donnerstag, 28. Dezember 2006 18:57 schrieb Bruce Momjian:
>> I think you can make the case that this should be an error, or at least
>> that's how it got on the TODO list. I can always remove it if people
>> don't want the item completed.

> The reason this was added is that modular applications expect that a locally
> issued BEGIN ... COMMIT executes a transaction, whereas now you don't know
> what you're getting. I think this change would have merit.

But how is making BEGIN an error going to improve life for such an
application? It'll be just as broken. In fact, if the app depends on
getting an error from BEGIN in any critical way, it'll be *more* broken
if it's ever run under the default warning-only setting.

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> Am Donnerstag, 28. Dezember 2006 18:57 schrieb Bruce Momjian:
>>> I think you can make the case that this should be an error, or at least
>>> that's how it got on the TODO list. I can always remove it if people
>>> don't want the item completed.
>
>> The reason this was added is that modular applications expect that a locally
>> issued BEGIN ... COMMIT executes a transaction, whereas now you don't know
>> what you're getting. I think this change would have merit.
>
> But how is making BEGIN an error going to improve life for such an
> application? It'll be just as broken. In fact, if the app depends on
> getting an error from BEGIN in any critical way, it'll be *more* broken
> if it's ever run under the default warning-only setting.

While we are on the topic, I have implemented a poor mans nested
transaction feature into my database access layer. essentially
subsequent calls to begin a transaction after the initial begin simply
increase an internal counter and set a savepoint. as you commit the
transactions the counter is decreased and the savepoints are released.
maybe this could be implemented inside postgresql to make the life of
modular programmers easier?

regards,
Lukas

news.postgresql.org wrote:

> While we are on the topic, I have implemented a poor mans nested
> transaction feature into my database access layer. essentially
> subsequent calls to begin a transaction after the initial begin simply
> increase an internal counter and set a savepoint. as you commit the
> transactions the counter is decreased and the savepoints are released.
> maybe this could be implemented inside postgresql to make the life of
> modular programmers easier?

Yeah, it's called "SAVEPOINT foo" and "RELEASE foo".

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

Alvaro Herrera wrote:
> news.postgresql.org wrote:
>
>> While we are on the topic, I have implemented a poor mans nested
>> transaction feature into my database access layer. essentially
>> subsequent calls to begin a transaction after the initial begin simply
>> increase an internal counter and set a savepoint. as you commit the
>> transactions the counter is decreased and the savepoints are released.
>> maybe this could be implemented inside postgresql to make the life of
>> modular programmers easier?
>
> Yeah, it's called "SAVEPOINT foo" and "RELEASE foo".

Err, I think you misunderstood what I said. My implementation uses
SAVEPOINTs already. The point is having some API where you do not have
to care of you are already in a transaction or not. Depending on if you
are it will either open a new transaction or simply place a savepoint.

with the following invented commands:
MBEGIN FOO1 // open transaction; set counter to 1
MBEGIN FOO2 // set savepoint FOO2; set counter to 2
MBEGIN FOO3 // set savepoint FOO3; set counter to 3
MROLLBACK FOO3 // rollback to FOO3; set counter to 2
MCOMMIT FOO2 // release FOO2; set counter to 1
MCOMMIT FOO1 // commit

regards,
Lukas

regards,
Lukas

On Tue, 2007-01-02 at 11:53 +0100, Peter Eisentraut wrote:
> Am Donnerstag, 28. Dezember 2006 18:57 schrieb Bruce Momjian:
> > I think you can make the case that this should be an error, or at least
> > that's how it got on the TODO list. I can always remove it if people
> > don't want the item completed.
>
> The reason this was added is that modular applications expect that a locally
> issued BEGIN ... COMMIT executes a transaction, whereas now you don't know
> what you're getting. I think this change would have merit.

Interesting point. My only comment is, "Do it one way or the other,
don't give me a user or a distribution packager a foot gun."

E.g., no GUC parameter. Just change the behavior or don't.

Joshua D. Drake

>
--

=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997
http://www.commandprompt.com/

Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Lukas Kahwe Smith <smith(at)pooteeweet(dot)org> writes:
> Err, I think you misunderstood what I said. My implementation uses
> SAVEPOINTs already. The point is having some API where you do not have
> to care of you are already in a transaction or not.

It's not that hard, is it?

if (PQtransactionStatus(conn) == PQTRANS_IDLE)
PQexec(conn, "BEGIN");
else
PQexec(conn, "SAVEPOINT foo");

regards, tom lane

On 1/2/07, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
>
> E.g., no GUC parameter. Just change the behavior or don't.
>
>
Please refer the conversation beginning at:
http://archives.postgresql.org/pgsql-hackers/2006-05/msg00249.php

That is where this TODO item came from. In the conversation, it was
understood that such a change would break many applications, hence one of
the option was to introduce a GUC var and keep it on by default, and a
couple of releases later, remove the GUC and make the behaviour default (
http://archives.postgresql.org/pgsql-hackers/2006-05/msg00273.php).

This would help the programmers can realize that they are doing something
wrong, and they can make appropriate changes to their code.

The usability of the GUC comes in a production environment, where it is not
possible to change the application. The DBA can buy some time by turning the
GUC var off.

I submitted a patch, which was incorrect and incomplete as I was _very_ new
to PGSQL. I could not follow up on it as I was switching jobs at the time.

Tom objected to the default=ON setting for the GUC.

The TODO has been declared "misconceived", but I guess there's still
interest out here. Would like to finish it once we reach a consensus.

Best regards,

--
gurjeet[(dot)singh](at)EnterpriseDB(dot)com
singh(dot)gurjeet(at){ gmail | hotmail | yahoo }.com

Tom Lane wrote:
> Lukas Kahwe Smith <smith(at)pooteeweet(dot)org> writes:
>> Err, I think you misunderstood what I said. My implementation uses
>> SAVEPOINTs already. The point is having some API where you do not have
>> to care of you are already in a transaction or not.
>
> It's not that hard, is it?
>
> if (PQtransactionStatus(conn) == PQTRANS_IDLE)
> PQexec(conn, "BEGIN");
> else
> PQexec(conn, "SAVEPOINT foo");

Its not exactly convenient either, especially in the case of modular
code that may be developed by different people. Anyways, like I said I
have a solution in my framework to make life of module developers
easier. Obviously proper nested transactions would be the ideal, but so
it goes. I was just throwing this out here when I saw Peter's comment.

regards,
Lukas

On Jan 2, 2007, at 2:01 PM, Lukas Kahwe Smith wrote:
> Tom Lane wrote:
>> Lukas Kahwe Smith <smith(at)pooteeweet(dot)org> writes:
>>> Err, I think you misunderstood what I said. My implementation
>>> uses SAVEPOINTs already. The point is having some API where you
>>> do not have to care of you are already in a transaction or not.
>> It's not that hard, is it?
>> if (PQtransactionStatus(conn) == PQTRANS_IDLE)
>> PQexec(conn, "BEGIN");
>> else
>> PQexec(conn, "SAVEPOINT foo");
>
> Its not exactly convenient either, especially in the case of
> modular code that may be developed by different people. Anyways,
> like I said I have a solution in my framework to make life of
> module developers easier. Obviously proper nested transactions
> would be the ideal, but so it goes. I was just throwing this out
> here when I saw Peter's comment.

+1. Right now it's not as big a deal since we don't allow transaction
control inside functions, but if we ever get that ability it will
become much more of a pain to write modular code that deals with
savepoints/subtransactions.
--
Jim Nasby jim(at)nasby(dot)net
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)

On 1/2/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Lukas Kahwe Smith <smith(at)pooteeweet(dot)org> writes:
> > Err, I think you misunderstood what I said. My implementation uses
> > SAVEPOINTs already. The point is having some API where you do not have
> > to care of you are already in a transaction or not.
>
> It's not that hard, is it?
>
> if (PQtransactionStatus(conn) == PQTRANS_IDLE)
> PQexec(conn, "BEGIN");
> else
> PQexec(conn, "SAVEPOINT foo");
>
> regards, tom lane
>

and how the releases and commit will happen? all at once or one by one?

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