Thread: 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@turnstep.com PGP Key: 0x14964AC8 200501252203 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -----BEGIN PGP SIGNATURE----- iD8DBQFB9wlpvJuQZxSWSsgRAsAzAKCxQ/JtR6/RXgV39uDTm9FIxCIp8QCeKC6T 2l10ef5DHkmFC2dSMQLNHjg= =HKv9 -----END PGP SIGNATURE-----
Attachment
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
Attachment
> 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@turnstep.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[@]dcc.uchile.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@turnstep.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@turnstep.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@candle.pha.pa.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 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@candle.pha.pa.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" <andrew@dunslane.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@turnstep.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@turnstep.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@gmx.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@turnstep.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@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: doc/src/sgml/ref/psql-ref.sgml =================================================================== RCS file: /cvsroot/pgsql/doc/src/sgml/ref/psql-ref.sgml,v retrieving revision 1.134 diff -c -c -r1.134 psql-ref.sgml *** doc/src/sgml/ref/psql-ref.sgml 14 Mar 2005 06:19:01 -0000 1.134 --- doc/src/sgml/ref/psql-ref.sgml 25 Apr 2005 16:26:03 -0000 *************** *** 2050,2055 **** --- 2050,2070 ---- </varlistentry> <varlistentry> + <indexterm> + <primary>rollback</primary> + <secondary>psql</secondary> + </indexterm> + <term><varname>ON_ERROR_ROLLBACK</varname></term> + <listitem> + <para> + When in a transaction, wrap every command in a savepoint that is + rolled back on error. Thus, errors will not abort an open + transaction. + </para> + </listitem> + </varlistentry> + + <varlistentry> <term><varname>ON_ERROR_STOP</varname></term> <listitem> <para> Index: src/bin/psql/common.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/common.c,v retrieving revision 1.96 diff -c -c -r1.96 common.c *** src/bin/psql/common.c 22 Feb 2005 04:40:52 -0000 1.96 --- src/bin/psql/common.c 25 Apr 2005 16:26:05 -0000 *************** *** 941,950 **** bool SendQuery(const char *query) { ! PGresult *results; ! TimevalStruct before, ! after; ! bool OK; if (!pset.db) { --- 941,950 ---- bool SendQuery(const char *query) { ! PGresult *results; ! TimevalStruct before, after; ! bool OK, on_error_rollback_savepoint = false; ! PGTransactionStatusType transaction_status; if (!pset.db) { *************** *** 973,979 **** SetCancelConn(); ! if (PQtransactionStatus(pset.db) == PQTRANS_IDLE && !GetVariableBool(pset.vars, "AUTOCOMMIT") && !command_no_begin(query)) { --- 973,981 ---- SetCancelConn(); ! transaction_status = PQtransactionStatus(pset.db); ! ! if (transaction_status == PQTRANS_IDLE && !GetVariableBool(pset.vars, "AUTOCOMMIT") && !command_no_begin(query)) { *************** *** 987,992 **** --- 989,1016 ---- } PQclear(results); } + else if (transaction_status == PQTRANS_INTRANS && + GetVariableBool(pset.vars, "ON_ERROR_ROLLBACK")) + { + if (pset.sversion < 80000) + { + fprintf(stderr, _("The server version (%d) does not support savepoints for ON_ERROR_ROLLBACK.\n"), + pset.sversion); + } + else + { + results = PQexec(pset.db, "SAVEPOINT pg_psql_temporary_savepoint"); + if (PQresultStatus(results) != PGRES_COMMAND_OK) + { + psql_error("%s", PQerrorMessage(pset.db)); + PQclear(results); + ResetCancelConn(); + return false; + } + PQclear(results); + on_error_rollback_savepoint = true; + } + } if (pset.timing) GETTIMEOFDAY(&before); *************** *** 1005,1010 **** --- 1029,1069 ---- PQclear(results); + /* If we made a temporary savepoint, possibly release/rollback */ + if (on_error_rollback_savepoint) + { + transaction_status = PQtransactionStatus(pset.db); + + /* We always rollback on an error */ + if (transaction_status == PQTRANS_INERROR) + results = PQexec(pset.db, "ROLLBACK TO pg_psql_temporary_savepoint"); + /* If they are no longer in a transaction, then do nothing */ + else if (transaction_status != PQTRANS_INTRANS) + results = NULL; + else + { + /* + * 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; + else + results = PQexec(pset.db, "RELEASE pg_psql_temporary_savepoint"); + } + if (PQresultStatus(results) != PGRES_COMMAND_OK) + { + psql_error("%s", PQerrorMessage(pset.db)); + PQclear(results); + ResetCancelConn(); + return false; + } + PQclear(results); + } + /* Possible microtiming output */ if (OK && pset.timing) printf(_("Time: %.3f ms\n"), DIFF_MSEC(&after, &before));
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[@]dcc.uchile.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@candle.pha.pa.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@candle.pha.pa.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@gmx.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@candle.pha.pa.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@candle.pha.pa.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@candle.pha.pa.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