Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
On Fri, May 10, 2013 at 9:50 AM, Peter Eisentraut pete...@gmx.net wrote: On 8/9/12 9:08 AM, Robert Haas wrote: On Wed, Aug 8, 2012 at 6:50 PM, David Fetter da...@fetter.org wrote: I'm wondering if perhaps -- in addition to what you've done here -- we should make psql -1 error out if reading from a terminal. +1 for this. OK, done. I had to revise the original patch pretty heavily before committing; My first use of 9.3beta1 in development failed because of changes introduced by this patch, specifically because of the newly introduced error psql: -1 is incompatible with -c and -l I'm not convinced this is correct. -c and -l are single-transaction actions almost by definition. This particular aspect of the change wasn't really brought up in the original thread. What was your thinking? Well, I think my main thinking was to prevent it in interactive mode, since it doesn't work in interactive mode, and then it also seemed to make sense to prevent it in the other cases to which it does not apply. I think there are cases where you can detect the fact that -1 -c wasn't actually wrapping the command in a BEGIN and an END, but I agree it might be a bit pedantic to worry about them. There have been previous proposals to allow multiple -c and -f options, and to allow those to be intermingled; if we did that, then this would surely matter. I agree that's hypothetical though since there's no patch to do any such thing currently on the table. Generally, I think we're too lax about detecting and complaining about conflicting combinations of options. But I'm not going to stand here and hold my breath if someone else feels that this particular combination doesn't merit a complaint. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
On 8/9/12 9:08 AM, Robert Haas wrote: On Wed, Aug 8, 2012 at 6:50 PM, David Fetter da...@fetter.org wrote: I'm wondering if perhaps -- in addition to what you've done here -- we should make psql -1 error out if reading from a terminal. +1 for this. OK, done. I had to revise the original patch pretty heavily before committing; My first use of 9.3beta1 in development failed because of changes introduced by this patch, specifically because of the newly introduced error psql: -1 is incompatible with -c and -l I'm not convinced this is correct. -c and -l are single-transaction actions almost by definition. This particular aspect of the change wasn't really brought up in the original thread. What was your thinking? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
On Fri, May 10, 2013 at 9:50 AM, Peter Eisentraut pete...@gmx.net wrote: On 8/9/12 9:08 AM, Robert Haas wrote: On Wed, Aug 8, 2012 at 6:50 PM, David Fetter da...@fetter.org wrote: I'm wondering if perhaps -- in addition to what you've done here -- we should make psql -1 error out if reading from a terminal. +1 for this. OK, done. I had to revise the original patch pretty heavily before committing; My first use of 9.3beta1 in development failed because of changes introduced by this patch, specifically because of the newly introduced error psql: -1 is incompatible with -c and -l I'm not convinced this is correct. -c and -l are single-transaction actions almost by definition. This particular aspect of the change wasn't really brought up in the original thread. What was your thinking? FYI, I noticed this issue when building one of our applications against HEAD; I'm not sure I agree with you vis-a-vis the -c option, as it is certainly plausible/meaningful to do: psql -c begin; update [something]; insert [something]; delete [something]; commit; and for that to be different from psql -c update [something]; insert [something]; delete [something]; Consider it stipulated that it's pretty plausible to expect things to break down if, in that series of requests, the UPDATE fails, and it isn't nearly safe to assume that the INSERT and/or DELETE statements would succeed after all that. I'd be pretty happy for psql -1 -c update [something]; insert [something]; delete [something]; to implicitly augment the query to: psql -c begin; update [something]; insert [something]; delete [something]; commit; It's a bit annoying (it bit me, giving me a complication, without any evident benefit) for psql -1 -c to refuse to run. I'd rather that it behave similarly to psql -1 -f, and wrap the queries in a transaction. For it to behave badly if I try to induce transaction control (e.g. - embedding BEGIN/END inside the queries) would not come as a surprise; that would be much the same as how psql -1 -f works, where the extra BEGIN is warned as redundant and the extra COMMIT is considered an error. As for psql -1 -l, it seems like a regression for that to fail. Listing the databases is pretty much already a single transaction; adding -1 is perhaps overspecifying things, but it doesn't seem wrong. -- When confronted by a difficult problem, solve it by reducing it to the question, How would the Lone Ranger handle this?
Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
Christopher Browne cbbro...@gmail.com writes: On Fri, May 10, 2013 at 9:50 AM, Peter Eisentraut pete...@gmx.net wrote: My first use of 9.3beta1 in development failed because of changes introduced by this patch, specifically because of the newly introduced error psql: -1 is incompatible with -c and -l I'm not convinced this is correct. -c and -l are single-transaction actions almost by definition. This particular aspect of the change wasn't really brought up in the original thread. What was your thinking? I'm not sure I agree with you vis-a-vis the -c option, as it is certainly plausible/meaningful to do: psql -c begin; update [something]; insert [something]; delete [something]; commit; and for that to be different from psql -c update [something]; insert [something]; delete [something]; While it might be *plausible* for those to be different, that's not actually how -c works in practice, because it sends the string as a single PQexec, which has the effect of making the string a single transaction even if the string does not contain begin/end explicitly. I think Peter is right and this error is bogus. Whatever redeeming social value it might have for sticklers is not worth breaking existing apps for. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
My first use of 9.3beta1 in development failed because of changes introduced by this patch, specifically because of the newly introduced error psql: -1 is incompatible with -c and -l I'm not convinced this is correct. -c and -l are single-transaction actions almost by definition. This particular aspect of the change wasn't really brought up in the original thread. What was your thinking? AFAICR, the 3 lines patch I submitted did not include such a check. Comments by Robert in the source suggest that the -1 option is ignored under -c and -l. This is because the transaction is handled by process_file which is not called in these cases. However, if they are single transaction nevertheless, the guard may just be removed, even if the option does nothing? ISTM that option -l is readonly, it does not matter much. For -c, I'm not that sure. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
On Wed, Aug 8, 2012 at 6:50 PM, David Fetter da...@fetter.org wrote: I'm wondering if perhaps -- in addition to what you've done here -- we should make psql -1 error out if reading from a terminal. +1 for this. OK, done. I had to revise the original patch pretty heavily before committing; the original patch assumed that it was OK to make psql -1 file go through process_file() while having psql -1 file still go through MainLoop() directly. This isn't a good idea, because that means that any other behavioral differences between process_file() and MainLoop() will be contingent on whether -1 is used, which is not what we want. And there is at least one such difference that matters: whether or not the file and line number get prepended when emitting error messages. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
OK, done. I had to revise the original patch pretty heavily before committing; the original patch assumed that it was OK to make psql -1 file go through process_file() while having psql -1 file still go through MainLoop() directly. This isn't a good idea, because that means that any other behavioral differences between process_file() and MainLoop() will be contingent on whether -1 is used, which is not what we want. Yep. I did that with a smallest and simplest change in mind, and beside when doing psql -1 file, you're hardly going to do anything else anyway, so I felt that it was not a big issue. And there is at least one such difference that matters: whether or not the file and line number get prepended when emitting error messages. Thanks for the improvements and for the push! -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
On Wed, Aug 1, 2012 at 4:28 AM, Fabien COELHO coe...@cri.ensmp.fr wrote: Dear PostgreSQL developers, Plese find attached a patch so that: Make psql -1 file.sql work as with -f Make psql --single-transaction option work on a non-interactive standard input as well, so that psql -1 input.sql behaves as psql -1 -f input.sql. This saner/less error-prone behavior was discussed in this thread back in June: http://archives.postgresql.org/pgsql-hackers/2012-06/msg00785.php I have tested it manually and it works for me. I'm not sure this is the best possible implementation, but it is a small diff one. I haven't found a place in the regression tests where psql could be tested with different options. Did I miss something? I'm wondering if perhaps -- in addition to what you've done here -- we should make psql -1 error out if reading from a terminal. Because accepting options that are intended to cause important behavior changes and then ignoring those options is Bad. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
On Wed, Aug 08, 2012 at 04:55:43PM -0400, Robert Haas wrote: On Wed, Aug 1, 2012 at 4:28 AM, Fabien COELHO coe...@cri.ensmp.fr wrote: Dear PostgreSQL developers, Plese find attached a patch so that: Make psql -1 file.sql work as with -f Make psql --single-transaction option work on a non-interactive standard input as well, so that psql -1 input.sql behaves as psql -1 -f input.sql. This saner/less error-prone behavior was discussed in this thread back in June: http://archives.postgresql.org/pgsql-hackers/2012-06/msg00785.php I have tested it manually and it works for me. I'm not sure this is the best possible implementation, but it is a small diff one. I haven't found a place in the regression tests where psql could be tested with different options. Did I miss something? I'm wondering if perhaps -- in addition to what you've done here -- we should make psql -1 error out if reading from a terminal. +1 for this. Because accepting options that are intended to cause important behavior changes and then ignoring those options is Bad. Yes, It Is. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make psql -1 file.sql work as with -f
Here is a new submission with the expected context diff format. Dear PostgreSQL developers, Plese find attached a patch so that: Make psql -1 file.sql work as with -f Make psql --single-transaction option work on a non-interactive standard input as well, so that psql -1 input.sql behaves as psql -1 -f input.sql. This saner/less error-prone behavior was discussed in this thread back in June: http://archives.postgresql.org/pgsql-hackers/2012-06/msg00785.php I have tested it manually and it works for me. I'm not sure this is the best possible implementation, but it is a small diff one. I haven't found a place in the regression tests where psql could be tested with different options. Did I miss something? -- Fabien.*** a/doc/src/sgml/ref/psql-ref.sgml --- b/doc/src/sgml/ref/psql-ref.sgml *** *** 512,521 PostgreSQL documentation listitem para When applicationpsql/application executes a script with the ! option-f/ option, adding this option wraps ! commandBEGIN//commandCOMMIT/ around the script to execute it ! as a single transaction. This ensures that either all the commands ! complete successfully, or no changes are applied. /para para --- 512,521 listitem para When applicationpsql/application executes a script with the ! option-f/ option or from a non-interactive standard input, adding ! this option wraps commandBEGIN//commandCOMMIT/ around the script ! to execute it as a single transaction. This ensures that either all ! the commands complete successfully, or no changes are applied. /para para *** a/src/bin/psql/help.c --- b/src/bin/psql/help.c *** *** 97,103 usage(void) printf(_( -V, --versionoutput version information, then exit\n)); printf(_( -X, --no-psqlrc do not read startup file (~/.psqlrc)\n)); printf(_( -1 (\one\), --single-transaction\n ! execute command file as a single transaction\n)); printf(_( -?, --help show this help, then exit\n)); printf(_(\nInput and output options:\n)); --- 97,104 printf(_( -V, --versionoutput version information, then exit\n)); printf(_( -X, --no-psqlrc do not read startup file (~/.psqlrc)\n)); printf(_( -1 (\one\), --single-transaction\n !execute command file or non-interactive stdin\n !as a single transaction\n)); printf(_( -?, --help show this help, then exit\n)); printf(_(\nInput and output options:\n)); *** a/src/bin/psql/settings.h --- b/src/bin/psql/settings.h *** *** 76,81 typedef struct _psqlSettings --- 76,82 bool notty; /* stdin or stdout is not a tty (as determined * on startup) */ + boolstdin_notty;/* stdin is not a tty (on startup) */ enum trivalue getPassword; /* prompt the user for a username and password */ FILE *cur_cmd_source; /* describe the status of the current main * loop */ *** a/src/bin/psql/startup.c --- b/src/bin/psql/startup.c *** *** 133,139 main(int argc, char *argv[]) /* We must get COLUMNS here before readline() sets it */ pset.popt.topt.env_columns = getenv(COLUMNS) ? atoi(getenv(COLUMNS)) : 0; ! pset.notty = (!isatty(fileno(stdin)) || !isatty(fileno(stdout))); pset.getPassword = TRI_DEFAULT; --- 133,140 /* We must get COLUMNS here before readline() sets it */ pset.popt.topt.env_columns = getenv(COLUMNS) ? atoi(getenv(COLUMNS)) : 0; ! pset.stdin_notty = !isatty(fileno(stdin)); ! pset.notty = (pset.stdin_notty || !isatty(fileno(stdout))); pset.getPassword = TRI_DEFAULT; *** *** 314,320 main(int argc, char *argv[]) if (!pset.notty) initializeInput(options.no_readline ? 0 : 1); ! successResult = MainLoop(stdin); } /* clean up */ --- 315,326 if (!pset.notty) initializeInput(options.no_readline ? 0 : 1); ! if (options.single_txn pset.stdin_notty) ! /* stdin is NOT a tty and -1, process as a file */ ! successResult = process_file(-, true, false); ! else ! /* no single transation, process as if interactive */ ! successResult = MainLoop(stdin); } /* clean up */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Make psql -1 file.sql work as with -f
Dear PostgreSQL developers, Plese find attached a patch so that: Make psql -1 file.sql work as with -f Make psql --single-transaction option work on a non-interactive standard input as well, so that psql -1 input.sql behaves as psql -1 -f input.sql. This saner/less error-prone behavior was discussed in this thread back in June: http://archives.postgresql.org/pgsql-hackers/2012-06/msg00785.php I have tested it manually and it works for me. I'm not sure this is the best possible implementation, but it is a small diff one. I haven't found a place in the regression tests where psql could be tested with different options. Did I miss something? -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index b6bf6a3..dc3c97e 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -512,10 +512,10 @@ PostgreSQL documentation listitem para When applicationpsql/application executes a script with the -option-f/ option, adding this option wraps -commandBEGIN//commandCOMMIT/ around the script to execute it -as a single transaction. This ensures that either all the commands -complete successfully, or no changes are applied. +option-f/ option or from a non-interactive standard input, adding +this option wraps commandBEGIN//commandCOMMIT/ around the script +to execute it as a single transaction. This ensures that either all +the commands complete successfully, or no changes are applied. /para para diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index 3ebf7cc..e56fd39 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -97,7 +97,8 @@ usage(void) printf(_( -V, --versionoutput version information, then exit\n)); printf(_( -X, --no-psqlrc do not read startup file (~/.psqlrc)\n)); printf(_( -1 (\one\), --single-transaction\n - execute command file as a single transaction\n)); + execute command file or non-interactive stdin\n + as a single transaction\n)); printf(_( -?, --help show this help, then exit\n)); printf(_(\nInput and output options:\n)); diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h index c907fa0..06d2407 100644 --- a/src/bin/psql/settings.h +++ b/src/bin/psql/settings.h @@ -76,6 +76,7 @@ typedef struct _psqlSettings bool notty; /* stdin or stdout is not a tty (as determined * on startup) */ + boolstdin_notty;/* stdin is not a tty (on startup) */ enum trivalue getPassword; /* prompt the user for a username and password */ FILE *cur_cmd_source; /* describe the status of the current main * loop */ diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c index 9a6306b..fb417ce 100644 --- a/src/bin/psql/startup.c +++ b/src/bin/psql/startup.c @@ -133,7 +133,8 @@ main(int argc, char *argv[]) /* We must get COLUMNS here before readline() sets it */ pset.popt.topt.env_columns = getenv(COLUMNS) ? atoi(getenv(COLUMNS)) : 0; - pset.notty = (!isatty(fileno(stdin)) || !isatty(fileno(stdout))); + pset.stdin_notty = !isatty(fileno(stdin)); + pset.notty = (pset.stdin_notty || !isatty(fileno(stdout))); pset.getPassword = TRI_DEFAULT; @@ -314,7 +315,12 @@ main(int argc, char *argv[]) if (!pset.notty) initializeInput(options.no_readline ? 0 : 1); - successResult = MainLoop(stdin); + if (options.single_txn pset.stdin_notty) + /* stdin is NOT a tty and -1, process as a file */ + successResult = process_file(-, true, false); + else + /* no single transation, process as if interative */ + successResult = MainLoop(stdin); } /* clean up */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
