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
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:
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.
---------------------------(end of broadcast)--------------------------- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])