Re: Continue transactions after errors in psql

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Attached is a patch that takes advantage of savepoints to enable
transactions to continue even after errors in psql. The name of it
is \reseterror, and it is off by default. It's backwards compatible,
and allows things like this to work on 8.0 and up servers:

\reseterror
BEGIN;
DELETE FROM foobar;
INSERT INTO foobar(a) VALUES(1);
ISNER INTO foobar(a) VALUES(2);
INSERT INTO foobar(a) VALUES(3);
COMMIT;

Doing a SELECT(a) FROM foobar will show two values, 1 and 3. This
is a great help for those of us that tend to type typos into our
psql session, and end up cursing as we have to restart our current
transaction. :)

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200501252203
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iD8DBQFB9wlpvJuQZxSWSsgRAsAzAKCxQ/JtR6/RXgV39uDTm9FIxCIp8QCeKC6T
2l10ef5DHkmFC2dSMQLNHjg=
=HKv9
-----END PGP SIGNATURE-----

On Tuesday 25 January 2005 22:07, Greg Sabino Mullane wrote:
> Attached is a patch that takes advantage of savepoints to enable
> transactions to continue even after errors in psql. The name of it
> is \reseterror, and it is off by default. It's backwards compatible,
> and allows things like this to work on 8.0 and up servers:
>
> \reseterror
> BEGIN;
> DELETE FROM foobar;
> INSERT INTO foobar(a) VALUES(1);
> ISNER INTO foobar(a) VALUES(2);
> INSERT INTO foobar(a) VALUES(3);
> COMMIT;
>
> Doing a SELECT(a) FROM foobar will show two values, 1 and 3. This
> is a great help for those of us that tend to type typos into our
> psql session, and end up cursing as we have to restart our current
> transaction. :)

I've been testing this patch and found the following bug:
test=# \reseterror
Reset error is on.
test=# begin;
BEGIN
test=# select * from t;
c
---
1
(1 row)
test=# delete from t;
DELETE 1
test=# select * from tt;
ERROR: relation "tt" does not exist
ERROR: relation "tt" does not exist
test=# select * from t;
c
---
(0 rows)
test=# commit;
COMMIT
ERROR: RELEASE SAVEPOINT may only be used in transaction blocks
ERROR: RELEASE SAVEPOINT may only be used in transaction blocks

I've attached a revised patch which fixes the problem, however I'm sure there
is a better way. Thanks to Neil for putting up with me on irc :-)

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

> I've attached a revised patch which fixes the problem, however I'm sure there
> is a better way. Thanks to Neil for putting up with me on irc :-)

How about calling the savepoint pg_psql_savepoint instead, that way it
follows our 'don't begin things with pg_' philosophy.

Chris

Robert Treat wrote:

> I've attached a revised patch which fixes the problem, however I'm sure
> there
> is a better way. Thanks to Neil for putting up with me on irc :-)

In September 2004 I had already sent a patch to implement this behaviour,
the patch, still in the archives, is here:
http://archives.postgresql.org/pgsql-patches/2004-09/bin00040.bin
(savepoints.patch)

There are some issues it addressed:

Assuming you put this option in your .psqlrc file, you will still probably
not want this to be active when you execute commands from a file
(non-interactive). So pset.notty must be checked.
Again, when using \i, resetting errors seems dangerous. Using \i should also
temporarily disable those savepoints.

The real problem with my patch was, that it did not release the savepoints.
Why? Look at this example (with the current patch reseterrors patch):

template1=# \reseterror
Reset error is on.
template1=# BEGIN;
BEGIN
template1=# SAVEPOINT a;
SAVEPOINT
template1=# CREATE TABLE TEST ( a integer);
CREATE TABLE
template1=# ROLLBACK TO a;
ERROR: no such savepoint

So to get this right, you have to track savepoints created by the user and
only release psql savepoints when there is no user savepoint "sitting on top
of" your savepoint.

Two ways come to my mind:
1) Parse SQL for savepoint and rollback to and create a stack of all
savepoints. Then you can always release all savepoints as long as they are
your own.
2) Implement a server-side function to get the savepoints from the server
and query that before every release.

What do you think?

Best Regards,
Michael Paesold

On Fri, 2005-01-28 at 04:46, Christopher Kings-Lynne wrote:
> > I've attached a revised patch which fixes the problem, however I'm sure there
> > is a better way. Thanks to Neil for putting up with me on irc :-)
>
> How about calling the savepoint pg_psql_savepoint instead, that way it
> follows our 'don't begin things with pg_' philosophy.
>

I was actually thinking of calling it something like
pg_<xact-start-time> thinking that would be pretty unique within a
transaction, though having a specific documented name seemed ok too.

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Michael Paesold wrote:
> 2) Implement a server-side function to get the savepoints from the server
> and query that before every release.

I could not find a way to do this. Is there any interface to the list?
</aside>

I looked over the patch from Michael Paesold, and talked extensively with
Robert Treat about this, and here is the solution Robert and I came up with:
(thanks to both for their work)

First, I'm not of the opinion that it should automatically be turned off
when running non-interactively. That's too much assuming of what the user
wants, when this is a settable flag. However, it should be settable via
a script to a definite state. So \reseterror will take an optional argument,
"off" or "on", which sets it rather than toggles it.

The patch Robert provided shold catch the problem of "good command-commit".
The other problem is not stepping on other people's savepoints. The best
solution we came up with was to check for savepoint commands ourselves,
similar to the way psql already checks for transaction affecting commands,
and handle things appropriately. Specifically, if someone issues a savepoint
while in \reseterror mode, it switches off automatically*. Since the
implementation of reseterror is pretty much a lazy shortcut to issuing savepoints
yourself, it should be safe to say that you do not want to mix manual and
automatic ones, and we'll back off (with a message) if you issue your own.
Plus there will be a warning in the docs to be careful about mixing savepoints
and the \reseterror method.

* We could also switch it back on after rollback or release, but this would
entail a little more tracking.

Comments?

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200501282306
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

-----BEGIN PGP SIGNATURE-----

iD8DBQFB+wzovJuQZxSWSsgRAt5eAJ9BVMtYZ9H+A76cNdUuhv4GpXeCwQCdFVsi
+mgg6ZzMylgHgdfiVn4yI5o=
=CpZQ
-----END PGP SIGNATURE-----

Greg Sabino Mullane wrote:

> Michael Paesold wrote:
>> 2) Implement a server-side function to get the savepoints from the server
>> and query that before every release.
>
> I could not find a way to do this. Is there any interface to the list?

Alvaro suggested to implement such a function. It is not there yet. I think
you would have to access the sub xact stack, but I have not looked into that
code for quite some time.
http://archives.postgresql.org/pgsql-general/2004-10/msg00370.php

> First, I'm not of the opinion that it should automatically be turned off
> when running non-interactively. That's too much assuming of what the user
> wants, when this is a settable flag. However, it should be settable via
> a script to a definite state. So \reseterror will take an optional
> argument,
> "off" or "on", which sets it rather than toggles it.

Discussion here last year showed some concern from people that this feature
could bite people and is not really safe. Perhaps the best way would be to
create three states:
\reseterrors (on|off|auto)
where auto means it's only active for interactive queries.
(auto could be named interactive)

> The other problem is not stepping on other people's savepoints. The best
> solution we came up with was to check for savepoint commands ourselves,
> similar to the way psql already checks for transaction affecting commands,
> and handle things appropriately. Specifically, if someone issues a
> savepoint
> while in \reseterror mode, it switches off automatically*. Since the
> implementation of reseterror is pretty much a lazy shortcut to issuing
> savepoints
> yourself, it should be safe to say that you do not want to mix manual and
> automatic ones, and we'll back off (with a message) if you issue your own.
> Plus there will be a warning in the docs to be careful about mixing
> savepoints
> and the \reseterror method.
>
> * We could also switch it back on after rollback or release, but this
> would
> entail a little more tracking.
>
> Comments?

I would prefer a solution, where the feature is not disabled as soon as I
use my own savepoints. I like \reseterror because it prevents making typos
from aborting my transaction. Explicit savepoints I rather use to try a
whole bunch of statements and then decide if I want to commit so far. I can
still make typos.

If you don't want to, I can implement such a savepoint stack. I don't think
it's that hard. The parsing, as you mentioned, should also not be too hard,
because the infrastructure (removing white space) is already there.

Best Regards,
Michael Paesold

On Sat, Jan 29, 2005 at 01:04:36PM +0100, Michael Paesold wrote:
> Greg Sabino Mullane wrote:
>
> >Michael Paesold wrote:
> >>2) Implement a server-side function to get the savepoints from the server
> >>and query that before every release.
> >
> >I could not find a way to do this. Is there any interface to the list?
>
> Alvaro suggested to implement such a function. It is not there yet. I think
> you would have to access the sub xact stack, but I have not looked into
> that code for quite some time.
> http://archives.postgresql.org/pgsql-general/2004-10/msg00370.php

The only problem with this idea is that the function cannot be run when
the transaction is in aborted state. Not sure if that is a problem or
not. What happens if the user does

SAVEPOINT foo; SLECT 1; ROLLBACK TO foo;

all in one command in psql?

I know psql sends that as three commands, so maybe it's not an issue.

--
Alvaro Herrera (<alvherre[(at)]dcc(dot)uchile(dot)cl>)
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem." (Tom Lane)

Alvaro Herrera wrote:

>> >Michael Paesold wrote:
>> Alvaro suggested to implement such a function. It is not there yet. I
>> think
>> you would have to access the sub xact stack, but I have not looked into
>> that code for quite some time.
>> http://archives.postgresql.org/pgsql-general/2004-10/msg00370.php
>
> The only problem with this idea is that the function cannot be run when
> the transaction is in aborted state. Not sure if that is a problem or
> not. What happens if the user does

I don't think there is a problem. If the transaction is in aborted state, we
only care if the last statement has aborted the transaction. Otherwise we
would not issue our savepoint at all. In that case, i.e. if the last
statement aborted the transaction, we can roll it back anyway, can't we?
There can't be a savepoint on top of us, because that would have failed
right now.
Is my logic wrong?

> SAVEPOINT foo; SLECT 1; ROLLBACK TO foo;
>
> all in one command in psql?
>
> I know psql sends that as three commands, so maybe it's not an issue.

As far as I remember psql splits the three commands, so there would be an
implicit savepoint for each individual statement:

* SAVEPOINT pg_psql_savepoint; -- [1]
SAVEPOINT foo;
* SAVEPOINT pg_psql_savepoint; -- [2]
SLECT 1;
* ROLLBACK TO pg_psql_savepoint; -- [2]
* SAVEPOINT pg_psql_savepoint; -- [3]
ROLLBACK TO foo;
* RELEASE pg_psql_savepoint; -- [3]
* RELEASE pg_psql_savepoint; -- [1], because pg_psql_savepoint is on top of
the stack now again

I hope you get the point. ;-)

Do you think it's better to create a server-side function or handle that in
the client? How hard would it be to implement such a function? And what
should it return? Only the name of the current top savepoint?

Best Regards,
Michael Paesold

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


> Do you think it's better to create a server-side function or
> handle that in the client? How hard would it be to implement
> such a function? And what should it return? Only the name of
> the current top savepoint?

I think handled on the client. Otherwise, this will not work
for 8.0 and I'd like to see it able to do so. I tbink the
logic presented so far is good: I'll work on getting a new
patch out as soon as I can.

Still, a server-side function would eventually be nice, perhaps
have it return a setof savepoint names in order, allowing one
to easily grab the whole list or just the "latest/current" one.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200502012248
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iD8DBQFCAE5IvJuQZxSWSsgRAqy7AJ4wo03ir/qRlRUxdC4sXId4OvlsswCgy50l
ldB3hFJaO88sBV1rfbADwwU=
=la3h
-----END PGP SIGNATURE-----

This thread has been saved for the 8.1 release:

http://momjian.postgresql.org/cgi-bin/pgpatches2

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

Greg Sabino Mullane wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> Attached is a patch that takes advantage of savepoints to enable
> transactions to continue even after errors in psql. The name of it
> is \reseterror, and it is off by default. It's backwards compatible,
> and allows things like this to work on 8.0 and up servers:
>
> \reseterror
> BEGIN;
> DELETE FROM foobar;
> INSERT INTO foobar(a) VALUES(1);
> ISNER INTO foobar(a) VALUES(2);
> INSERT INTO foobar(a) VALUES(3);
> COMMIT;
>
> Doing a SELECT(a) FROM foobar will show two values, 1 and 3. This
> is a great help for those of us that tend to type typos into our
> psql session, and end up cursing as we have to restart our current
> transaction. :)
>
> - --
> Greg Sabino Mullane greg(at)turnstep(dot)com
> PGP Key: 0x14964AC8 200501252203
> http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
> -----BEGIN PGP SIGNATURE-----
>
> iD8DBQFB9wlpvJuQZxSWSsgRAsAzAKCxQ/JtR6/RXgV39uDTm9FIxCIp8QCeKC6T
> 2l10ef5DHkmFC2dSMQLNHjg=
> =HKv9
> -----END PGP SIGNATURE-----
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
> joining column's datatypes do not match

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Andrew Dunstan wrote:
> Bruce Momjian said:
> >
> > This thread has been saved for the 8.1 release:
> >
> > http://momjian.postgresql.org/cgi-bin/pgpatches2
> >
> >
>
> There have been many such postings. Are we not in the 8.1 dev cycle right
> now? And some changes have already been committed to the HEAD branch that
> are not or should not be intended for 8.0.x with or without a n ARC
> replacement, unless my memory is mistaken. If thta is so, these posts are
> quite confusing, as this is the message that has usually greeted patches
> destined for a branch later than the one in the current dev cycle. If not,
> what on earth is going on?

Yes. I am putting all the stuff in one place so we can review them as a
group and apply them. They all require some work before being applied.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian said:
>
> This thread has been saved for the 8.1 release:
>
> http://momjian.postgresql.org/cgi-bin/pgpatches2
>
>

There have been many such postings. Are we not in the 8.1 dev cycle right
now? And some changes have already been committed to the HEAD branch that
are not or should not be intended for 8.0.x with or without a n ARC
replacement, unless my memory is mistaken. If thta is so, these posts are
quite confusing, as this is the message that has usually greeted patches
destined for a branch later than the one in the current dev cycle. If not,
what on earth is going on?

cheers

andrew

"Andrew Dunstan" <andrew(at)dunslane(dot)net> writes:
> what on earth is going on?

The particular posts you're noticing are just Bruce's script that acks
messages he's saved into his pending-patches lists:
http://candle.pha.pa.us/cgi-bin/pgpatches
http://candle.pha.pa.us/cgi-bin/pgpatches2
pgpatches is meant as the current-cycle TODO queue, pgpatches2 as the
next-cycle queue.

I'm not sure why he hasn't folded the latter into the former, because
8.0 isn't the development tip anymore. But anyway, those lists exist
only to make sure that proposed patches don't fall through the cracks
--- they aren't meant as hard promises that something will be applied
in a particular development cycle.

The big picture at the moment is what Bruce mentioned earlier today:
core is currently thinking that we should do a quick and dirty ARC-
to-2Q change to eliminate the ARC patent problem in the 8.0.* branch,
and go forward with a normal development cycle for 8.1. The whole
LRU/ARC/2Q approach is looking like it's a dead end in the long run
because of locking considerations, so we're thinking we shouldn't do
any more with it than is needed to ensure we can ship a stable,
patent-free algorithm in 8.0.*.

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Finally had a chance to sit down at look at this afresh, and I'm
pretty sure I've got all the kinks worked out this time. Apologies
for not attaching, but my mail system is not working well enough
at the moment. So, please try to break this patch:

http://www.gtsm.com/pg/psql_error_recovery.diff

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200503060152
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

-----BEGIN PGP SIGNATURE-----

iD8DBQFCKqjXvJuQZxSWSsgRArIsAJ9wR99qOOCrfS8Hly7xNnWHV/RSHwCfQUac
V2Xr4prpz50+nJJe6ci1rLY=
=F6aX
-----END PGP SIGNATURE-----

Greg Sabino Mullane wrote:

> Finally had a chance to sit down at look at this afresh, and I'm
> pretty sure I've got all the kinks worked out this time. Apologies
> for not attaching, but my mail system is not working well enough
> at the moment. So, please try to break this patch:
>
> http://www.gtsm.com/pg/psql_error_recovery.diff

Some suggestions in random order:

* I think you should use PSQLexec instead of using PQexec directly. PSQLexec
is used by all \-commands and prints out queries with -E, which is very
helpful for debugging.

-E display queries that internal commands generate

* You do not check for the server version before activating \reseterror.
-> use PQserverVersion() to check for >= 80000

* Perhaps the name should be \reseterrors (plural)? Just my personal opinion
though.

* If I read the code correctly, you now don't destroy user savepoints
anymore, but on the other hand, you do not release the psql savepoint after
a user-defined savepoint is released. In other words, each time a user
creates a savepoint, one psql savepoint is left on the subxact stack. I
don't know if this is a real problem, though.

* You have not yet implemented a way to savely put \reseterror in .psqlrc. I
previously suggested an AUTO setting (additional to ON/OFF) that disables
\reseterror when reading from a non-tty. So putting \reseterror AUTO in
.psqlrc would be save.

Otherwise, I could not find a way to break it. :-)

Best Regards,
Michael Paesold

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> * I think you should use PSQLexec instead of using PQexec directly. PSQLexec
> is used by all \-commands and prints out queries with -E, which is very
> helpful for debugging.
But these are not backslash commands, but almost directly analogous to the
BEGINs emitted by psql when in AutoCommit mode. On the other hand, it might
be neat to see all the savepoints psql will automagically create for you, so
I could go either way.

> * You do not check for the server version before activating \reseterror.
> -> use PQserverVersion() to check for >= 80000

Thanks, it was in an earlier version, promise. This should be in command.c:

if (pset.sversion < 80000)
{
fprintf(stderr, _("The server version (%d) does not support savepoints.\n"),
pset.sversion);
}

> * Perhaps the name should be \reseterrors (plural)? Just my personal
> opinion though.

Nah, less errors from people typing the wrong thing if we keep it shorter.

> * You have not yet implemented a way to savely put \reseterror in .psqlrc. I
> previously suggested an AUTO setting (additional to ON/OFF) that disables
> \reseterror when reading from a non-tty. So putting \reseterror AUTO in
> ..psqlrc would be save.

Hmm...I suppose we could do that. Do we have anything else that does something
similar? I guess I'm not convinced that we need to change a switch's behavior
based on the tty status.

> * If I read the code correctly, you now don't destroy user savepoints
> anymore, but on the other hand, you do not release the psql savepoint after
> a user-defined savepoint is released. In other words, each time a user
> creates a savepoint, one psql savepoint is left on the subxact stack. I
> don't know if this is a real problem, though.

Correct. More detail: we release our own temporary savepoint, unless the user
has successfully implemented their own savepoint. We need to do this so that we
do not clobber the user's savepoint. The larger problem is that "our" savepoints
and the user's savepoints tend to clobber each other. The normal flow of things
is to issue our savepoint, then the user's command, and then check to see if the
command succcessfully completed, and if we are still in a transaction. If we are
no longer in a transaction, we do nothing, as it means that our savepoint has been
destroyed, so we don't need to worry about it. Otherwise, if the command failed,
we issue a rollback of our savepoint, which is guaranteed to be there because the
user cannot have removed it, because their command did not succeed. Now the tricky
part: If the transaction is still active, and the command succeeded, and the command
was not SAVEPOINT, ROLLBACK TO, or RELEASE, we issue a release of our savepoint,
which is not strictly necessary, but is a good idea so we don't build up a large
chunk of old savepoints. Aside: we check if the command they issued was a savepoint-
manipulating one by not parsing the SQL (yuck) but by simply checking the cmdResult
string. Although there is no way to tell "RELEASE" from "RELEASE TO" from this check,
we know it cannot be the former because we are still in a transaction. :) If it was
one of those three commands, we do not issue a release. If they issued a successful
release or rollback, then it just clobbered our savepoint, which now no longer exists.
If it was a savepoint, we cannot release, or we will clobber their savepoint, which
was created after ours. We could theoretically try and figure out beforehand if
they are issuing a savepoint command, but we must wrap it anyway in case it fails so
we can rollback and not have it end the outer transaction. Thus, we create one extra
savepoint every time the user issues a savepoint. Until they rollback or release, of
course, in which case they also remove an equal number of our savepoints as their
savepoints. So it doubles the number of savepoints a user currently has, but this
is the price we pay for having the feature.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200503070028
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iD8DBQFCK+a6vJuQZxSWSsgRAsGRAJ99vJ0Mlzzl8MWBv262K//h0NasLwCgiBHZ
o2tgPvfwHR8zSJ1TAJ5/x30=
=itOf
-----END PGP SIGNATURE-----

Greg Sabino Mullane wrote:

>> * You have not yet implemented a way to savely put \reseterror
>> in .psqlrc. I previously suggested an AUTO setting (additional
>> to ON/OFF) that disables \reseterror when reading from a non-tty.
>> So putting \reseterror AUTO in ..psqlrc would be save.
>
> Hmm...I suppose we could do that. Do we have anything else that
> does something similar? I guess I'm not convinced that we need
> to change a switch's behavior based on the tty status.

I do think so. In it's current state, would you yourself put \reseterror in
your .psqlrc? Or even an /etc/psqlrc?
It would break all my scripts that must either succeed or fail -- now they
will produce garbage in my databases when something goes wrong! In my
opinion, the behaviour should depend on tty in all settings, but I am o.k.
with an AUTO setting, because so it's at least usable.

I think without tty-detection, the patch just conflicts with PostgreSQL
philosophy that the user should be kept save from unintended
data-destruction.

The SQL-Standard itself says that errors inside transactions should only
rollback the last statement, if possible. So why is that not implemented in
PostgreSQL? What I read from past discussions here, is because it's just
unsave and will lead to data-garbage if you aren't very careful.

>> * If I read the code correctly, you now don't destroy user savepoints
>> anymore, but on the other hand, you do not release the psql savepoint
>> after
>> a user-defined savepoint is released. In other words, each time a user
>> creates a savepoint, one psql savepoint is left on the subxact stack. I
>> don't know if this is a real problem, though.
>
> Correct. More detail: we release our own temporary savepoint, unless
> the user has successfully implemented their own savepoint...

The current way is ok for me at the moment. I still think there is a better
way (parsing statements like it's already done for
no-transaction-allowed-statements), but hey, as soon as your patch will be
applied, I can myself propose another patch to improve this. ;-)

Best Regards,
Michael Paesold

"Michael Paesold" <mpaesold(at)gmx(dot)at> writes:
> I do think so. In it's current state, would you yourself put \reseterror in
> your .psqlrc? Or even an /etc/psqlrc?
> It would break all my scripts that must either succeed or fail -- now they
> will produce garbage in my databases when something goes wrong!

This is sounding a whole lot like the concerns that prompted us to
reject server-side autocommit a little while ago.

The problem with rejiggering error-handling behavior is that you *will*
break existing code, on a rather fundamental level, and it's not even
obvious that it's broken until after things have gone badly wrong.

I don't have a good solution, but I do think that you need to set things
up so that an application or script must invoke the new behavior
explicitly. Hidden defaults that silently change such behavior look
like land mines waiting to be stepped on.

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Tom Lane wrote:
> I don't have a good solution, but I do think that you need to set things
> up so that an application or script must invoke the new behavior
> explicitly. Hidden defaults that silently change such behavior look
> like land mines waiting to be stepped on.

Michael Paesold wrote:
> I think without tty-detection, the patch just conflicts with PostgreSQL
> philosophy that the user should be kept save from unintended
> data-destruction.

I don't know if I would go that far. This is a setting that must be explicitly
enabled, so it's more a case of caveat emptor is you choose to enable it.
I don't like the idea of the behavior changing so radically just depending
on the tty state. Maybe if we call it "ttyonly" or something instead of
"auto"...?

> The SQL-Standard itself says that errors inside transactions should only
> rollback the last statement, if possible. So why is that not implemented in
> PostgreSQL? What I read from past discussions here, is because it's just
> unsave and will lead to data-garbage if you aren't very careful.

That's a good point: if that is indeed what the standard says, we should
probably see about following it. Rolling back to the last savepoint seems
a reasonable behavior to me.

> The current way is ok for me at the moment. I still think there is a better
> way (parsing statements like it's already done for
> no-transaction-allowed-statements), but hey, as soon as your patch will be
> applied, I can myself propose another patch to improve this. ;-)

Parsing the statment will not help: even if the statement is a savepoint, we
need to wrap it in case we need to roll it back. The only other option I
can see to my patch is to, upon a successful user savepoint creation,
roll back their savepoint and immediately reissue it. That seems worse to
me than having N*2 savepoints though.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200503121836
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iD8DBQFCM30WvJuQZxSWSsgRAryZAKCyIDYd36mAaU464AbPkHe9zkYI+QCfU+Fb
7A2WJwLJcOvzJDHjRnr55v4=
=UJ8E
-----END PGP SIGNATURE-----

Greg Sabino Mullane wrote:
[ There is text before PGP section. ]
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> Finally had a chance to sit down at look at this afresh, and I'm
> pretty sure I've got all the kinks worked out this time. Apologies
> for not attaching, but my mail system is not working well enough
> at the moment. So, please try to break this patch:
>
> http://www.gtsm.com/pg/psql_error_recovery.diff

I have modified Greg's patch to fit in better with the existing psql
code. I changed the command to \set ON_ERROR_ROLLBACK, which seems to
fit better. Here is an illustration of its use (patch attached):

test=> BEGIN;
BEGIN
test=> lkjasdf;
ERROR: syntax error at or near "lkjasdf" at character 1
LINE 1: lkjasdf;
^
test=> SELECT 1;
ERROR: current transaction is aborted, commands ignored until end of
transaction block
test=> COMMIT;
ROLLBACK
test=> \set ON_ERROR_ROLLBACK on
test=> BEGIN;
BEGIN
test=> lkjasdf;
ERROR: syntax error at or near "lkjasdf" at character 1
LINE 1: lkjasdf;
^
test=> SELECT 1;
?column?
----------
1
(1 row)

test=> COMMIT;
COMMIT

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Mon, Apr 25, 2005 at 12:34:00PM -0400, Bruce Momjian wrote:

> > Finally had a chance to sit down at look at this afresh, and I'm
> > pretty sure I've got all the kinks worked out this time. Apologies
> > for not attaching, but my mail system is not working well enough
> > at the moment. So, please try to break this patch:

This will only be activated when using interactive input, right?
Seems dangerous to apply the setting to scripts. What if the user
enables the feature in .psqlrc and then forgets?

--
Alvaro Herrera (<alvherre[(at)]dcc(dot)uchile(dot)cl>)
"Y eso te lo doy firmado con mis lágrimas" (Fiebre del Loco)

Michael Paesold wrote:
> Greg Sabino Mullane wrote:
>
> > Finally had a chance to sit down at look at this afresh, and I'm
> > pretty sure I've got all the kinks worked out this time. Apologies
> > for not attaching, but my mail system is not working well enough
> > at the moment. So, please try to break this patch:
> >
> > http://www.gtsm.com/pg/psql_error_recovery.diff
>
>
> Some suggestions in random order:
>
> * I think you should use PSQLexec instead of using PQexec directly. PSQLexec
> is used by all \-commands and prints out queries with -E, which is very
> helpful for debugging.
>
> -E display queries that internal commands generate

It is now \set ON_ERROR_ROLLBACK, and PQexec seems right for that.
Also, this isn't something like \d where anyone would want to see the
queries, I think.

>
> * You do not check for the server version before activating \reseterror.
> -> use PQserverVersion() to check for >= 80000

Added. Patch just posted.

> * Perhaps the name should be \reseterrors (plural)? Just my personal opinion
> though.

Changed, as you see above.

> * If I read the code correctly, you now don't destroy user savepoints
> anymore, but on the other hand, you do not release the psql savepoint after
> a user-defined savepoint is released. In other words, each time a user
> creates a savepoint, one psql savepoint is left on the subxact stack. I
> don't know if this is a real problem, though.

Interesting. I thought this would fail, but it doesn't:

test=> \set ON_ERROR_ROLLBACK on
test=> begin;
BEGIN
test=> savepoint user1;
SAVEPOINT
test=> lkjasdfsadf;
ERROR: syntax error at or near "lkjasdfsadf" at character 1
LINE 1: lkjasdfsadf;
^
test=> rollback to savepoint user1;
ROLLBACK

and I think this is the reason:

test=> BEGIN;
BEGIN
test=> SAVEPOINT psql1;
SAVEPOINT
test=> SAVEPOINT user1;
SAVEPOINT
test=> SAVEPOINT psql1;
SAVEPOINT
test=> lkjasd;
ERROR: syntax error at or near "lkjasd" at character 1
LINE 1: lkjasd;
^
test=> ROLLBACK TO psql1;
ROLLBACK
test=> ROLLBACK TO user1;
ROLLBACK

What Greg's code does, effectively, is to move the savepoint down below
the SAVEPOINt/RELEASE/ROLLBACK so it doesn't discard the user command.
Nice trick:

+ /*
+ * Do nothing if they are messing with savepoints themselves:
+ * doing otherwise would cause us to remove their savepoint,
+ * or have us rollback our savepoint they have just removed
+ */
+ if (strcmp(PQcmdStatus(results), "SAVEPOINT") == 0 ||
+ strcmp(PQcmdStatus(results), "RELEASE") == 0 ||
+ strcmp(PQcmdStatus(results), "ROLLBACK") ==0)
+ results = NULL;

> * You have not yet implemented a way to savely put \reseterror in .psqlrc. I
> previously suggested an AUTO setting (additional to ON/OFF) that disables
> \reseterror when reading from a non-tty. So putting \reseterror AUTO in
> .psqlrc would be save.

Good question, or rather, should ON_ERROR_ROLLBACK have an effect when
commands come from a file? There is no way to test for the error in
psql so it seems you would never want the transaction to continue after
an error. I am inclined to make ON_ERROR_ROLLBACK work only for
interactive sessions, just like ON_ERROR_STOP works only for
non-interactive sessions.

Comments?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Greg Sabino Mullane wrote:
> > * If I read the code correctly, you now don't destroy user savepoints
> > anymore, but on the other hand, you do not release the psql savepoint after
> > a user-defined savepoint is released. In other words, each time a user
> > creates a savepoint, one psql savepoint is left on the subxact stack. I
> > don't know if this is a real problem, though.
>
> Correct. More detail: we release our own temporary savepoint, unless the user
> has successfully implemented their own savepoint. We need to do this so that we
> do not clobber the user's savepoint. The larger problem is that "our" savepoints
> and the user's savepoints tend to clobber each other. The normal flow of things
> is to issue our savepoint, then the user's command, and then check to see if the
> command succcessfully completed, and if we are still in a transaction. If we are
> no longer in a transaction, we do nothing, as it means that our savepoint has been
> destroyed, so we don't need to worry about it. Otherwise, if the command failed,
> we issue a rollback of our savepoint, which is guaranteed to be there because the
> user cannot have removed it, because their command did not succeed. Now the tricky
> part: If the transaction is still active, and the command succeeded, and the command
> was not SAVEPOINT, ROLLBACK TO, or RELEASE, we issue a release of our savepoint,
> which is not strictly necessary, but is a good idea so we don't build up a large
> chunk of old savepoints. Aside: we check if the command they issued was a savepoint-
> manipulating one by not parsing the SQL (yuck) but by simply checking the cmdResult
> string. Although there is no way to tell "RELEASE" from "RELEASE TO" from this check,
> we know it cannot be the former because we are still in a transaction. :) If it was
> one of those three commands, we do not issue a release. If they issued a successful
> release or rollback, then it just clobbered our savepoint, which now no longer exists.
> If it was a savepoint, we cannot release, or we will clobber their savepoint, which
> was created after ours. We could theoretically try and figure out beforehand if
> they are issuing a savepoint command, but we must wrap it anyway in case it fails so
> we can rollback and not have it end the outer transaction. Thus, we create one extra
> savepoint every time the user issues a savepoint. Until they rollback or release, of
> course, in which case they also remove an equal number of our savepoints as their
> savepoints. So it doubles the number of savepoints a user currently has, but this
> is the price we pay for having the feature.

Oh, here's his description. I updated the patch comments:

+ /*
+ * Do nothing if they are messing with savepoints themselves:
+ * If the user did RELEASE or ROLLBACK, our savepoint is gone.
+ * If they issued a SAVEPOINT, releasing ours would remove theirs.
+ */

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Tom Lane wrote:
> "Michael Paesold" <mpaesold(at)gmx(dot)at> writes:
> > I do think so. In it's current state, would you yourself put \reseterror in
> > your .psqlrc? Or even an /etc/psqlrc?
> > It would break all my scripts that must either succeed or fail -- now they
> > will produce garbage in my databases when something goes wrong!
>
> This is sounding a whole lot like the concerns that prompted us to
> reject server-side autocommit a little while ago.
>
> The problem with rejiggering error-handling behavior is that you *will*
> break existing code, on a rather fundamental level, and it's not even
> obvious that it's broken until after things have gone badly wrong.
>
> I don't have a good solution, but I do think that you need to set things
> up so that an application or script must invoke the new behavior
> explicitly. Hidden defaults that silently change such behavior look
> like land mines waiting to be stepped on.

Right, this is off by default. We might be able to make it on by
default if we have it enabled only for interactive psql's. We need to
discuss this.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Greg Sabino Mullane wrote:
> > The current way is ok for me at the moment. I still think there is a better
> > way (parsing statements like it's already done for
> > no-transaction-allowed-statements), but hey, as soon as your patch will be
> > applied, I can myself propose another patch to improve this. ;-)
>
> Parsing the statment will not help: even if the statement is a savepoint, we
> need to wrap it in case we need to roll it back. The only other option I
> can see to my patch is to, upon a successful user savepoint creation,
> roll back their savepoint and immediately reissue it. That seems worse to
> me than having N*2 savepoints though.

Agreed.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Greg Sabino Mullane wrote:
> > The SQL-Standard itself says that errors inside transactions should only
> > rollback the last statement, if possible. So why is that not implemented in
> > PostgreSQL? What I read from past discussions here, is because it's just
> > unsave and will lead to data-garbage if you aren't very careful.
>
> That's a good point: if that is indeed what the standard says, we should
> probably see about following it. Rolling back to the last savepoint seems
> a reasonable behavior to me.

OK, so we need to make a decision. Right now I have Greg's patch that
is enabled by "\set ON_ERROR_ROLLBACK on":

test=> \set ON_ERROR_ROLLBACK on
test=> BEGIN;
BEGIN
test=> lkjasdf;
ERROR: syntax error at or near "lkjasdf" at character 1
LINE 1: lkjasdf;
^
test=> SELECT 1;
?column?
----------
1
(1 row)

test=> COMMIT;
COMMIT

The question is what to make the default:

disable it by default for all sessions (current patch)
enable it by default only for interactive sessions, like AUTOCOMMIT
enable it by default for all sessions (breaks too many apps)
add a third mode called 'ttyonly' and figure out a default

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Alvaro Herrera wrote:
> On Mon, Apr 25, 2005 at 12:34:00PM -0400, Bruce Momjian wrote:
>
> > > Finally had a chance to sit down at look at this afresh, and I'm
> > > pretty sure I've got all the kinks worked out this time. Apologies
> > > for not attaching, but my mail system is not working well enough
> > > at the moment. So, please try to break this patch:
>
> This will only be activated when using interactive input, right?
> Seems dangerous to apply the setting to scripts. What if the user
> enables the feature in .psqlrc and then forgets?

I just posted this question to hacker to get votes.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

> Michael Paesold wrote:
>> Some suggestions in random order:
>>
>> * I think you should use PSQLexec instead of using PQexec directly.
>> PSQLexec
>> is used by all \-commands and prints out queries with -E, which is very
>> helpful for debugging.
>>
>> -E display queries that internal commands generate
>
> It is now \set ON_ERROR_ROLLBACK, and PQexec seems right for that.
> Also, this isn't something like \d where anyone would want to see the
> queries, I think.

I just thought it was nice for debugging. E.g. your example below would be
more easy to analyze if one could see the queries with -E.

>> * You do not check for the server version before activating \reseterror.
>> -> use PQserverVersion() to check for >= 80000
>
> Added. Patch just posted.

Ok, looks good.

>> * Perhaps the name should be \reseterrors (plural)? Just my personal
>> opinion
>> though.
>
> Changed, as you see above.

My first patch for this feature (last year) also used \set. I think this is
more consistent. On the other hand there is no auto-completition for \set.
Perhaps this should be added later.

>> * If I read the code correctly, you now don't destroy user savepoints
>> anymore, but on the other hand, you do not release the psql savepoint
>> after >>
>> a user-defined savepoint is released. In other words, each time a user
>> creates a savepoint, one psql savepoint is left on the subxact stack. I
>> don't know if this is a real problem, though.
>
> Interesting. I thought this would fail, but it doesn't:
>
[example...]
Yeah, I tried that earlier.

> What Greg's code does, effectively, is to move the savepoint down below
> the SAVEPOINt/RELEASE/ROLLBACK so it doesn't discard the user command.
> Nice trick:
[code...]

I think it is quite good. But note: I did not say that the feature broke
user savepoint, I just mentioned that with user savepoints, some (internal)
savepoint could be left on the stack (in the server) until the user defined
savepoints below the interal ones would be released. Nevertheless, I think
this is no problem in the real-world.

>> * You have not yet implemented a way to savely put \reseterror in
>> .psqlrc. I
>> previously suggested an AUTO setting (additional to ON/OFF) that disables
>> \reseterror when reading from a non-tty. So putting \reseterror AUTO in
>> .psqlrc would be save.
>
> Good question, or rather, should ON_ERROR_ROLLBACK have an effect when
> commands come from a file? There is no way to test for the error in
> psql so it seems you would never want the transaction to continue after
> an error. I am inclined to make ON_ERROR_ROLLBACK work only for
> interactive sessions, just like ON_ERROR_STOP works only for
> non-interactive sessions.

+1 for disabling ON_ERROR_ROLLBACK if pset.cur_cmd_interactive is false. Or
provide another switch that can be put in .psqlrc and is only activated for
pset.cur_cmd_interactive.

Btw. thanks Bruce for getting this done.

Best Regards,
Michael Paesold

Bruce Momjian wrote:

> Greg Sabino Mullane wrote:
>> > The SQL-Standard itself says that errors inside transactions should
>> only
>> > rollback the last statement, if possible. So why is that not
>> > implemented in
>> > PostgreSQL? What I read from past discussions here, is because it's
>> > just
>> > unsave and will lead to data-garbage if you aren't very careful.
>>
>> That's a good point: if that is indeed what the standard says, we should
>> probably see about following it. Rolling back to the last savepoint seems
>> a reasonable behavior to me.
>
> OK, so we need to make a decision. Right now I have Greg's patch that
> is enabled by "\set ON_ERROR_ROLLBACK on":
>
> test=> \set ON_ERROR_ROLLBACK on
> test=> BEGIN;
> BEGIN
> test=> lkjasdf;
> ERROR: syntax error at or near "lkjasdf" at character 1
> LINE 1: lkjasdf;
> ^
> test=> SELECT 1;
> ?column?
> ----------
> 1
> (1 row)
>
> test=> COMMIT;
> COMMIT
>
> The question is what to make the default:
>
> disable it by default for all sessions (current patch)
> enable it by default only for interactive sessions, like AUTOCOMMIT
> enable it by default for all sessions (breaks too many apps)
> add a third mode called 'ttyonly' and figure out a default

My vote:
1) disable it by default for all sessions

2) enable it with \set (can be set in .psqlrc), but provide a way so it only
works with interactive commands, either always, or something like
\set ON_ERROR_ROLLBACK interactive

Best Regards,
Michael Paesold

Bruce Momjian wrote:
> Greg Sabino Mullane wrote:
> > > The SQL-Standard itself says that errors inside transactions should only
> > > rollback the last statement, if possible. So why is that not implemented in
> > > PostgreSQL? What I read from past discussions here, is because it's just
> > > unsave and will lead to data-garbage if you aren't very careful.
> >
> > That's a good point: if that is indeed what the standard says, we should
> > probably see about following it. Rolling back to the last savepoint seems
> > a reasonable behavior to me.
>
> The question is what to make the default:
>
> o disable it by default for all sessions (current patch)
> o enable it by default only for interactive sessions, like AUTOCOMMIT
> o enable it by default for all sessions (breaks too many apps)
> o add a third mode called 'ttyonly' and figure out a default

Based on the comments I received, and the mention that ignoring errors
is part of the SQL standard, I chose the second option, patch attached:

$ psql test
Welcome to psql 8.1devel, the PostgreSQL interactive terminal.

Type: \copyright for distribution terms
\h for help with SQL commands
\? for help with psql commands
\g or terminate with semicolon to execute query
\q to quit

test=> BEGIN;
BEGIN
test=> asdf;
ERROR: syntax error at or near "asdf" at character 1
LINE 1: asdf;
^
test=> SELECT 1;
?column?
----------
1
(1 row)

test=> COMMIT;
COMMIT

Can someone confirm that this is the way Oracle works as well? I
checked on IRC and isql does it. I am uncertain how applications
behave.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

I think everyone agrees this should only work in interactive mode. I
think the only unknown is if it should be 'on' by default in interactive
mode? Does it make sense to follow the standard in interactive mode if
we don't follow it in non-interative mode?

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

Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Greg Sabino Mullane wrote:
> > > > The SQL-Standard itself says that errors inside transactions should only
> > > > rollback the last statement, if possible. So why is that not implemented in
> > > > PostgreSQL? What I read from past discussions here, is because it's just
> > > > unsave and will lead to data-garbage if you aren't very careful.
> > >
> > > That's a good point: if that is indeed what the standard says, we should
> > > probably see about following it. Rolling back to the last savepoint seems
> > > a reasonable behavior to me.
> >
> > The question is what to make the default:
> >
> > o disable it by default for all sessions (current patch)
> > o enable it by default only for interactive sessions, like AUTOCOMMIT
> > o enable it by default for all sessions (breaks too many apps)
> > o add a third mode called 'ttyonly' and figure out a default
>
> Based on the comments I received, and the mention that ignoring errors
> is part of the SQL standard, I chose the second option, patch attached:
>
> $ psql test
> Welcome to psql 8.1devel, the PostgreSQL interactive terminal.
>
> Type: \copyright for distribution terms
> \h for help with SQL commands
> \? for help with psql commands
> \g or terminate with semicolon to execute query
> \q to quit
>
> test=> BEGIN;
> BEGIN
> test=> asdf;
> ERROR: syntax error at or near "asdf" at character 1
> LINE 1: asdf;
> ^
> test=> SELECT 1;
> ?column?
> ----------
> 1
> (1 row)
>
> test=> COMMIT;
> COMMIT
>
> Can someone confirm that this is the way Oracle works as well? I
> checked on IRC and isql does it. I am uncertain how applications
> behave.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I think everyone agrees this should only work in interactive mode. I
> think the only unknown is if it should be 'on' by default in interactive
> mode? Does it make sense to follow the standard in interactive mode if
> we don't follow it in non-interative mode?

I doubt it's a good idea to change the default for this at all; in
particular, making the default interactive behavior different from
the noninteractive behavior seems like a recipe for problems.

regards, tom lane

> disable it by default for all sessions (current patch)

That is the most backwards non-surprising solution.

> enable it by default only for interactive sessions, like AUTOCOMMIT

And that is what other dbms' do.

Chris

Christopher Kings-Lynne wrote:
> > disable it by default for all sessions (current patch)
>
> That is the most backwards non-surprising solution.
>
> > enable it by default only for interactive sessions, like AUTOCOMMIT
>
> And that is what other dbms' do.

The current version controls only interactive sessions, and is off by
default. I am willing to change that, but I need to hear from more
people on this to change it.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

>>> enable it by default only for interactive sessions, like AUTOCOMMIT
>>
>>And that is what other dbms' do.
>
>
> The current version controls only interactive sessions, and is off by
> default. I am willing to change that, but I need to hear from more
> people on this to change it.

It's good with me.

Chris

Bruce Momjian wrote:
>
> disable it by default for all sessions (current patch)
> enable it by default only for interactive sessions, like AUTOCOMMIT

These two seem like the best contenders.

The choice comes down to whether we should:

1) Be like most other dbms's (e.g. Oracle, Firebird) + follow the spec.
2) Minimize psql behavior changes for current postgresql users.

Personally I favor 1), so would prefer:

enable it by default only for interactive sessions, like AUTOCOMMIT

Mark

Mark Kirkwood wrote:
> Bruce Momjian wrote:
> >
> > disable it by default for all sessions (current patch)
> > enable it by default only for interactive sessions, like AUTOCOMMIT
>
> These two seem like the best contenders.
>
> The choice comes down to whether we should:
>
> 1) Be like most other dbms's (e.g. Oracle, Firebird) + follow the spec.
> 2) Minimize psql behavior changes for current postgresql users.
>
> Personally I favor 1), so would prefer:
>
> enable it by default only for interactive sessions, like AUTOCOMMIT

We are going for 'off' by default and only interactive. The fact that
interactive and non-interactive behavior is different is something that
suggests that turning it on by default might cause confusion.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:
> Mark Kirkwood wrote:
>
>
>>Personally I favor 1), so would prefer:
>>
>>enable it by default only for interactive sessions, like AUTOCOMMIT
>
>
> We are going for 'off' by default and only interactive.

Sweet.

> The fact that
> interactive and non-interactive behavior is different is something that
> suggests that turning it on by default might cause confusion.
>

True, but reasonably explained by suggesting that non-interactive
scripts should not be expected to keep going when there are typos or
similar. In fact, I seem to recall being burned by that very thing in my
early Oracle days...

Mark

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


To reiterate my opinion, I think the behavior should be the same
for interactive and non-interactive sessions. Not only will it
prevent nasty surprises, but unless we make a third 'setting',
there will be no way to enable this in non-interactive scripts,
which is something that I would want to be able to do.

I don't buy the "but what if I set it in .psqlrc and forget" argument.
That could be applied to a lot of things you could put in there. This
setting defaults to "off" and must be explicitly enabled. I'd be okay
with a "smart" mode that explicitly enables the interactive/non-interactive
split.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200504260737
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iD8DBQFCbilxvJuQZxSWSsgRAgf8AJ9/NcsU/5A0V9isGvQy4sjba/aukgCgoFbp
otSb0vVLfnL7mIt99rA4Piw=
=1vVP
-----END PGP SIGNATURE-----

Greg Sabino Mullane wrote:
> To reiterate my opinion, I think the behavior should be the same
> for interactive and non-interactive sessions. Not only will it
> prevent nasty surprises, but unless we make a third 'setting',
> there will be no way to enable this in non-interactive scripts,
> which is something that I would want to be able to do.
>
> > I don't buy the "but what if I set it in .psqlrc and forget" argument.
> That could be applied to a lot of things you could put in there. This
> setting defaults to "off" and must be explicitly enabled. I'd be okay
> with a "smart" mode that explicitly enables the
> interactive/non-interactive
> split.

But people (like me for example) will want to enable this behaviour by
default. So they (me too) will put the option in .psqlrc. It is then enabled
"by default". But then many of my scripts will destroy data instead of just
erroring out.
I just don't see why non-interactive mode does need such a switch because
there is no way to check if there was an error. So just put two queries
there and hope one will work?

If you really want this for scripts, there must be two options:
* one to put savely into .psqlrc (what some people will want, I have \set
AUTOCOMMIT off in my .psqlrc file, too, and I know I am not the only one)
* another one that will also work in scripts

I hope you understand and accept the issue here.

Best Regards,
Michael Paesold

Michael Paesold wrote:
>
> But people (like me for example) will want to enable this behaviour by
> default. So they (me too) will put the option in .psqlrc. It is then
> enabled "by default". But then many of my scripts will destroy data
> instead of just erroring out.
> I just don't see why non-interactive mode does need such a switch
> because there is no way to check if there was an error. So just put two
> queries there and hope one will work?

DROP TABLE foo;
CREATE TABLE foo...

--
Richard Huxton
Archonet Ltd

"Greg Sabino Mullane" <greg(at)turnstep(dot)com> writes:
> To reiterate my opinion, I think the behavior should be the same
> for interactive and non-interactive sessions. Not only will it
> prevent nasty surprises, but unless we make a third 'setting',
> there will be no way to enable this in non-interactive scripts,
> which is something that I would want to be able to do.

I'm finding it hard to visualize a non-interactive script making
any good use of such a setting. Without a way to test whether
you got an error or not, it would amount to an "ignore errors
within transactions" mode, which seems a pretty bad idea.

Can you show a plausible use-case for such a thing?

regards, tom lane

Richard Huxton wrote:

> Michael Paesold wrote:
>>
>> But people (like me for example) will want to enable this behaviour by
>> default. So they (me too) will put the option in .psqlrc. It is then
>> enabled "by default". But then many of my scripts will destroy data
>> instead of just erroring out.
>> I just don't see why non-interactive mode does need such a switch because
>> there is no way to check if there was an error. So just put two queries
>> there and hope one will work?
>
> DROP TABLE foo;
> CREATE TABLE foo...

This would be:

\set AUTOCOMMIT off
DROP TABLE foo; -- error, rolled back
CREATE TABLE foo ...
COMMIT;

You could as well do:

\set AUTOCOMMIT on -- default
DROP TABLE foo; -- print error message
CREATE TABLE foo ...

There is not much difference, except for locking, ok. I see your point, but
I don't think this makes enabling it by default (even in .psqlrc) any safer.

Best Regards,
Michael Paesold

Richard Huxton <dev(at)archonet(dot)com> writes:
> Michael Paesold wrote:
>> I just don't see why non-interactive mode does need such a switch
>> because there is no way to check if there was an error. So just put two
>> queries there and hope one will work?

> DROP TABLE foo;
> CREATE TABLE foo...

Unconvincing. What if the drop fails for permission reasons, rather
than because the table's not there? Then the CREATE will fail too
... but now the script bulls ahead regardless, with who knows what
bad consequences.

I would far rather see people code explicit markers around statements
whose failure can be ignored. That is, a script that needs this
behavior ought to look like

BEGIN;
\begin_ignore_error
DROP TABLE foo;
\end_ignore_error
CREATE ...
...
COMMIT;

where I'm supposing that we invent psql backslash commands to cue
the sending of SAVEPOINT and RELEASE-or-ROLLBACK commands. (Anyone
got a better idea for the names than that?)

Once you've got such an infrastructure, it makes sense to allow an
interactive mode that automatically puts such things around each
statement. But I can't really see the argument for using such a
behavior in a script. Scripts are too stupid.

regards, tom lane

> I would far rather see people code explicit markers around statements
> whose failure can be ignored. That is, a script that needs this
> behavior ought to look like
>
> BEGIN;
> \begin_ignore_error
> DROP TABLE foo;
> \end_ignore_error
> CREATE ...
> ...
> COMMIT;

That seems awful noisy. Why not just:

BEGIN:
DROP TABLE foo;
ERROR: table foo does not exist;
CONTINUE;
etc....

Sincerely,

Joshua D. Drake
Command Prompt, Inc.

In article <426E4AC1(dot)4070706(at)archonet(dot)com>,
Richard Huxton <dev(at)archonet(dot)com> writes:

>> I just don't see why non-interactive mode does need such a switch
>> because there is no way to check if there was an error. So just put
>> two queries there and hope one will work?

> DROP TABLE foo;
> CREATE TABLE foo...

Ah, my pet peeve! "DROP TABLE IF EXISTS name" is the only thing I
really miss from MySQL.

At 12:28 AM 27/04/2005, Tom Lane wrote:
>Can you show a plausible use-case for such a thing?

A not-uncommon case in other DBs is to handle insert/update code where
insert is the most likely result. Not sure if this is relevant to scripts:

Begin;
...do stuff...
insert into....
<trap duplicate index error and do update instead>
update...
...more stuff...
commit;

Also, the blunder-on-regardless approach is popular in pg_dump, or so I'm
told ;-).

----------------------------------------------------------------
Philip Warner | __---_____
Albatross Consulting Pty. Ltd. |----/ - \
(A.B.N. 75 008 659 498) | /(@) ______---_
Tel: (+61) 0500 83 82 81 | _________ \
Fax: (+61) 03 5330 3172 | ___________ |
Http://www.rhyme.com.au | / \|
| --________--
PGP key available upon request, | /
and from pgp.mit.edu:11371 |/

Philip Warner <pjw(at)rhyme(dot)com(dot)au> writes:
> Also, the blunder-on-regardless approach is popular in pg_dump, or so I'm
> told ;-).

Sure, but pg_dump scripts don't try to execute as a single transaction.
None of this discussion applies to the behavior outside an explicit
transaction block.

regards, tom lane

"Joshua D. Drake" <jd(at)commandprompt(dot)com> writes:
>> BEGIN;
>> \begin_ignore_error
>> DROP TABLE foo;
>> \end_ignore_error
>> CREATE ...
>> ...
>> COMMIT;

> That seems awful noisy. Why not just:

> BEGIN:
> DROP TABLE foo;
> ERROR: table foo does not exist;
> CONTINUE;
> etc....

Well, ignoring questions of how we choose to spell the commands, the
thing I'd not like about the second alternative is that it doesn't
afford any control over the number of statements rolled back upon
error.

regards, tom lane

Tom Lane wrote:

>Richard Huxton <dev(at)archonet(dot)com> writes:
>
>
>>Michael Paesold wrote:
>>
>>
>>>I just don't see why non-interactive mode does need such a switch
>>>because there is no way to check if there was an error. So just put two
>>>queries there and hope one will work?
>>>
>>>
>
>
>
>>DROP TABLE foo;
>>CREATE TABLE foo...
>>
>>
>
>Unconvincing. What if the drop fails for permission reasons, rather
>than because the table's not there? Then the CREATE will fail too
>... but now the script bulls ahead regardless, with who knows what
>bad consequences.
>
>I would far rather see people code explicit markers around statements
>whose failure can be ignored. That is, a script that needs this
>behavior ought to look like
>
> BEGIN;
> \begin_ignore_error
> DROP TABLE foo;
> \end_ignore_error
> CREATE ...
> ...
> COMMIT;
>
>
>
>

That's a lot of work. In this particular case I would actually like to
see us provide "DROP IF EXISTS ..." or some such.

My instinct on this facility is that distinguishing between interactive
and noninteractive use is likely to be highly confusing. So I would
favor behaviour that is consistent and defaults to off.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> I would far rather see people code explicit markers around statements
>> whose failure can be ignored. That is, a script that needs this
>> behavior ought to look like
>>
>> BEGIN;
>> \begin_ignore_error
>> DROP TABLE foo;
>> \end_ignore_error
>> CREATE ...
>> ...
>> COMMIT;

> That's a lot of work.

How so? It's a minuscule extension to the psql patch already coded:
just provide backslash commands to invoke the bits of code already
written.

> In this particular case I would actually like to
> see us provide "DROP IF EXISTS ..." or some such.

That's substantially more work, with substantially less scope of
applicability: it would only solve the issue for DROP.

regards, tom lane

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>Tom Lane wrote:
>>
>>
>>>I would far rather see people code explicit markers around statements
>>>whose failure can be ignored. That is, a script that needs this
>>>behavior ought to look like
>>>
>>>BEGIN;
>>>\begin_ignore_error
>>>DROP TABLE foo;
>>>\end_ignore_error
>>>CREATE ...
>>>...
>>>COMMIT;
>>>
>>>
>
>
>
>>That's a lot of work.
>>
>>
>
>How so? It's a minuscule extension to the psql patch already coded:
>just provide backslash commands to invoke the bits of code already
>written.
>
>

I meant it's a lot to type ;-)

>
>
>>In this particular case I would actually like to
>>see us provide "DROP IF EXISTS ..." or some such.
>>
>>
>
>That's substantially more work, with substantially less scope of
>applicability: it would only solve the issue for DROP.
>
>
>
>

True. I wasn't suggesting it as an alternative in the general case. I
still think it's worth doing, though - I have often seen it requested
and can't think of a compelling reason not to provide it. But maybe
that's off topic ;-)

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>> \begin_ignore_error
>>> DROP TABLE foo;
>>> \end_ignore_error

> I meant it's a lot to type ;-)

Well, that's just a matter of choosing good (ie short) names for the
backslash commands. I was trying to be clear rather than proposing
names I would actually want to use ;-). Any suggestions?

regards, tom lane

On Apr 26, 2005, at 10:35 AM, Tom Lane wrote:

> Once you've got such an infrastructure, it makes sense to allow an
> interactive mode that automatically puts such things around each
> statement. But I can't really see the argument for using such a
> behavior in a script. Scripts are too stupid.

Would it be possible to have a command line switch and/or a psql
variable to control "interactive"? If I recall correctly, the setting
depends on tty and there are possible interactive uses of psql outside
of a terminal session. With so many things depending on this, it would
be nice to be able to override the default.

Thanks,

John DeSoi, Ph.D.
http://pgedit.com/
Power Tools for PostgreSQL

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> I'm finding it hard to visualize a non-interactive script making
> any good use of such a setting. Without a way to test whether
> you got an error or not, it would amount to an "ignore errors
> within transactions" mode, which seems a pretty bad idea.
>
> Can you show a plausible use-case for such a thing?

I could have used this yesterday. I was populating a test table with
a primary key on two columns and needed to add a bunch of random rows.
I generated a 10_000 line file of one insert statement each. Rather than
worrying about collisions, I could simply \rollbackonerror (or whatever
we're calling it today :) and silently discard the handful that happen
to violate the primary key constraint and let the rest insert.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200504270754
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

-----BEGIN PGP SIGNATURE-----

iD8DBQFCb33NvJuQZxSWSsgRAvdfAJwMqysSpVI2BDh9wENT2jxMZnspagCfRlHJ
9ElhNydsz2FsCc1JgI5R+gU=
=h9AW
-----END PGP SIGNATURE-----

On Tue, 2005-04-26 at 10:28, Tom Lane wrote:
> "Greg Sabino Mullane" <greg(at)turnstep(dot)com> writes:
> > To reiterate my opinion, I think the behavior should be the same
> > for interactive and non-interactive sessions. Not only will it
> > prevent nasty surprises, but unless we make a third 'setting',
> > there will be no way to enable this in non-interactive scripts,
> > which is something that I would want to be able to do.
>
> I'm finding it hard to visualize a non-interactive script making
> any good use of such a setting. Without a way to test whether
> you got an error or not, it would amount to an "ignore errors
> within transactions" mode, which seems a pretty bad idea.
>
> Can you show a plausible use-case for such a thing?
>

I plan to use it in scripts that push site meta-data out to our test
servers, where the list of sites are all different so any static data
dump is bound to fail on some foreign key checks (but I don't care which
ones fail as long as some go over).

I'm sure others can come up with different scenarios, but more
importantly is I don't see a good reason to treat this setting different
from all others and explicitly forbid this use from people, especially
when I can imagine people coming from other dbs where this behavior is
more common who might in fact expect it to work this way.

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

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> >>> \begin_ignore_error
> >>> DROP TABLE foo;
> >>> \end_ignore_error
>
> > I meant it's a lot to type ;-)
>
> Well, that's just a matter of choosing good (ie short) names for the
> backslash commands. I was trying to be clear rather than proposing
> names I would actually want to use ;-). Any suggestions?

Well, if we allowed ON_ERROR_ROLLBACK to work in non-interactive
sessions we could just do:

\set ON_ERROR_ROLLBACK on
DROP TABLE foo;
\set ON_ERROR_ROLLBACK off

No new syntax required. Seems this variable is going to need an
'interactive' setting, which means it isn't boolean anymore.

Also, should we allow 'true/false' to work with these seetings? We do
that with boolean columns in SQL.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> Well, that's just a matter of choosing good (ie short) names for the
>> backslash commands. I was trying to be clear rather than proposing
>> names I would actually want to use ;-). Any suggestions?

> Well, if we allowed ON_ERROR_ROLLBACK to work in non-interactive
> sessions we could just do:

> \set ON_ERROR_ROLLBACK on
> DROP TABLE foo;
> \set ON_ERROR_ROLLBACK off

That isn't the same thing at all. The syntax I was proposing allows the
script writer to define a savepoint covering multiple statements,
whereas the above does not.

Maybe what we really need is a "rollback or release savepoint"
operation, defined as "ROLLBACK TO foo if in error state, RELEASE foo
if not in error state". This is essentially the thing that a script
writer has to have and can't do for himself due to the lack of any
conditional ability in psql scripts. We could imagine implementing
that either as a SQL command or as a psql backslash command ... I don't
have a strong feeling either way.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> Well, that's just a matter of choosing good (ie short) names for the
> >> backslash commands. I was trying to be clear rather than proposing
> >> names I would actually want to use ;-). Any suggestions?
>
> > Well, if we allowed ON_ERROR_ROLLBACK to work in non-interactive
> > sessions we could just do:
>
> > \set ON_ERROR_ROLLBACK on
> > DROP TABLE foo;
> > \set ON_ERROR_ROLLBACK off
>
> That isn't the same thing at all. The syntax I was proposing allows the
> script writer to define a savepoint covering multiple statements,
> whereas the above does not.

Well, it fits the use case posted, that is to conditionally roll back a
_single_ failed query. I don't see the need to add a new
infrastructure/command unless people have a use case for rolling back a
group of statements on failure. I have no seen such a description yet.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073