Re: [HACKERS] psql \d sequence display
On 9/25/17 13:53, Fabien COELHO wrote: > \d+ does not show more. > > Maybe Type, Min, Max, Inc & Cycles are enough for \d? That seems kind of arbitrary. Start and Cache are just as relevant. > The next/future or last/previous value is not shown. If one could be > available it would be nice to have? You can get those from the sequence. Running \d on a table doesn't show the table data either. So I think this is a valid distinction. > Maybe some names are a little large, eg "Increment" could be "Inc.". > The value is nearly always 1? Yeah, but then that would look weird if only one heading were abbreviated. The total display width is good, so we don't need to squeeze it too hard. > Not sure why it is "Cycles" (plural) instead of "Cycle". This is meant to be a verb, as in "does it cycle?". I have changed it to "Cycles?" to match other similar headings. > I do not understand why some queries have "... \n" "... \n" and others > have "\n ..." "\n ...". I would suggest to homogeneize around the former, > because "\nFROM ..." is less readable. Yeah that is a bit ugly, but I have just moved existing code around. I have now committed my patch with minor adjustments, so we have something working for PG10. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] psql \d sequence display
Hello, This should be fixed for PG10, so if you have any feedback on the design, please let me know soon. Works for me on head against a 9.6 server, which is good. My 0.02 €: \d+ does not show more. Maybe Type, Min, Max, Inc & Cycles are enough for \d? The next/future or last/previous value is not shown. If one could be available it would be nice to have? Maybe some names are a little large, eg "Increment" could be "Inc.". The value is nearly always 1? Not sure why it is "Cycles" (plural) instead of "Cycle". The non regression test could also show a more esoteric sequence (cyclic, ...). There is no documentation. Maybe none is needed. I do not understand why some queries have "... \n" "... \n" and others have "\n ..." "\n ...". I would suggest to homogeneize around the former, because "\nFROM ..." is less readable. -- 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] psql - add ability to test whether a variable exists
Correct Fabien. I have already removed myself as a reviewer. Thanks. As you wish! Thanks for the feedback, which I understood as "works for me". -- 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] psql - add ability to test whether a variable exists
Correct Fabien. I have already removed myself as a reviewer. Thanks. - robins | mobile On 20 Sep. 2017 5:13 pm, "Fabien COELHO"wrote: > > Hello Robins, > > I was able to test the functionality (which seemed to work fine) and fed in >> my comment to assist anyone else reviewing this patch (and intentionally >> let it's state as 'Needs Review'). >> >> While trying to provide my feedback, on hindsight I should have been more >> detailed about what I didn't test. Being my first review, I didn't >> understand that not checking a box meant 'failure'. For e.g. I read the >> sgml changes, which felt okay but didn't click 'Passed' because my env >> wasn't setup properly. >> > > Hmmm, ISTM that it was enough. The feature is psql specific, so the fact > that it works against a 9.6 server is both expected and fine. So ISTM that > your test "passed". > > Just running "make check" would run the non regression test, which is > basically what you tested online, against the compiled version. > > Probably you should have a little look at the source code and doc as well. > > I've set this back to 'Needs Review' because clearly needs it. >> > > Hmmm. > > If you do a review, which I think you have done, then you have done it:-) > > If you consider that your test was not a review and you do not intend to > provide one, then thanks for the feedback anyway, and maybe you should > consider removing yourself from the "Reviewer" column, otherwise nobody > will provide a review. > > -- > Fabien. >
Re: [HACKERS] psql - add ability to test whether a variable exists
Hello Robins, I was able to test the functionality (which seemed to work fine) and fed in my comment to assist anyone else reviewing this patch (and intentionally let it's state as 'Needs Review'). While trying to provide my feedback, on hindsight I should have been more detailed about what I didn't test. Being my first review, I didn't understand that not checking a box meant 'failure'. For e.g. I read the sgml changes, which felt okay but didn't click 'Passed' because my env wasn't setup properly. Hmmm, ISTM that it was enough. The feature is psql specific, so the fact that it works against a 9.6 server is both expected and fine. So ISTM that your test "passed". Just running "make check" would run the non regression test, which is basically what you tested online, against the compiled version. Probably you should have a little look at the source code and doc as well. I've set this back to 'Needs Review' because clearly needs it. Hmmm. If you do a review, which I think you have done, then you have done it:-) If you consider that your test was not a review and you do not intend to provide one, then thanks for the feedback anyway, and maybe you should consider removing yourself from the "Reviewer" column, otherwise nobody will provide a review. -- 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] psql - add ability to test whether a variable exists
I was able to test the functionality (which seemed to work fine) and fed in my comment to assist anyone else reviewing this patch (and intentionally let it's state as 'Needs Review'). While trying to provide my feedback, on hindsight I should have been more detailed about what I didn't test. Being my first review, I didn't understand that not checking a box meant 'failure'. For e.g. I read the sgml changes, which felt okay but didn't click 'Passed' because my env wasn't setup properly. I've set this back to 'Needs Review' because clearly needs it. Apologies for the noise here. The new status of this patch is: Needs review -- 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] psql - add ability to test whether a variable exists
Hi Fabien, I was able to test the functionality (which seemed to work fine) and fed in my comment to assist anyone else reviewing this patch (and intentionally let it's state as 'Needs Review'). While trying to provide my feedback, on hindsight I should have been more detailed about what I didn't test. Being my first review, I didn't understand that not checking a box meant 'failure'. For e.g. I read the sgml changes, which felt okay but didn't click 'Passed' because my env wasn't setup properly. I've set this back to 'Needs Review' because clearly needs it. Apologies for the noise here. - robins On 20 September 2017 at 16:18, Fabien COELHOwrote: > > Hello Robins, > > Thanks for the review. > > The following review has been posted through the commitfest application: >> make installcheck-world: not tested >> Implements feature: tested, failed >> > > Where ? > > Spec compliant: not tested >> Documentation:tested, failed >> > > Where ? I just regenerated the html doc on the patch without a glitch. > > The patch applies cleanly and compiles + installs fine (although am unable >> to do installcheck-world on my Cygwin setup). This is how the patch works >> on my setup. >> >> $ /opt/postgres/reviewpatch/bin/psql -U postgres -h localhost >> psql (11devel, server 9.6.1) >> Type "help" for help. >> >> postgres=# \set i 1 >> postgres=# \if :{?i} >> postgres=# \echo 'testing' >> testing >> postgres=# \endif >> postgres=# \if :{?id} >> postgres@# \echo 'testing' >> \echo command ignored; use \endif or Ctrl-C to exit current \if block >> postgres@# \endif >> postgres=# >> > > ISTM that this is the expected behavior. > > In the first case, "i" is defined, so the test is true and the echo echoes. > > In the second case, "id" is undefined, the test is false and the echo is > skipped. > > I do not understand why you conclude that the feature "failed". > > -- > Fabien. >
Re: [HACKERS] psql - add ability to test whether a variable exists
Hello Robins, Thanks for the review. The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, failed Where ? Spec compliant: not tested Documentation:tested, failed Where ? I just regenerated the html doc on the patch without a glitch. The patch applies cleanly and compiles + installs fine (although am unable to do installcheck-world on my Cygwin setup). This is how the patch works on my setup. $ /opt/postgres/reviewpatch/bin/psql -U postgres -h localhost psql (11devel, server 9.6.1) Type "help" for help. postgres=# \set i 1 postgres=# \if :{?i} postgres=# \echo 'testing' testing postgres=# \endif postgres=# \if :{?id} postgres@# \echo 'testing' \echo command ignored; use \endif or Ctrl-C to exit current \if block postgres@# \endif postgres=# ISTM that this is the expected behavior. In the first case, "i" is defined, so the test is true and the echo echoes. In the second case, "id" is undefined, the test is false and the echo is skipped. I do not understand why you conclude that the feature "failed". -- 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] psql - add ability to test whether a variable exists
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, failed Spec compliant: not tested Documentation:tested, failed The patch applies cleanly and compiles + installs fine (although am unable to do installcheck-world on my Cygwin setup). This is how the patch works on my setup. $ /opt/postgres/reviewpatch/bin/psql -U postgres -h localhost psql (11devel, server 9.6.1) Type "help" for help. postgres=# \set i 1 postgres=# \if :{?i} postgres=# \echo 'testing' testing postgres=# \endif postgres=# \if :{?id} postgres@# \echo 'testing' \echo command ignored; use \endif or Ctrl-C to exit current \if block postgres@# \endif postgres=# -- 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] psql: new help related to variables are not too readable
2017-09-18 11:44 GMT+02:00 Alvaro Herrera: > Pavel Stehule wrote: > > > I am looking on man pagers - and there are very well readable > > > > The rules are simply - when some variables are short - less than 6 chars, > > then it description and label are on same line. Between items are empty > > line > > I was having a similar idea. I'm not sure how many lines we would save > that way, but it seems to me we could use a slightly longer threshold -- > say 8 chars before we cause a newline and wrap. > > Maybe we need to come up with guidelines for translators, also. As > somebody noted, I've always gone to some trouble to produce translate > output that preserves the original's constraints; I don't see any reason > forother translations to behave differently. > > (By all means, if we add entry-separating newlines, let's not put them > as part of the translatable string.) > +1 Pavel > -- > Álvaro Herrerahttps://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
Re: [HACKERS] psql: new help related to variables are not too readable
Pavel Stehule wrote: > I am looking on man pagers - and there are very well readable > > The rules are simply - when some variables are short - less than 6 chars, > then it description and label are on same line. Between items are empty > line I was having a similar idea. I'm not sure how many lines we would save that way, but it seems to me we could use a slightly longer threshold -- say 8 chars before we cause a newline and wrap. Maybe we need to come up with guidelines for translators, also. As somebody noted, I've always gone to some trouble to produce translate output that preserves the original's constraints; I don't see any reason forother translations to behave differently. (By all means, if we add entry-separating newlines, let's not put them as part of the translatable string.) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] psql: new help related to variables are not too readable
2017-09-14 16:35 GMT+02:00 Pavel Stehule: > > > 2017-09-14 15:17 GMT+02:00 Alvaro Herrera : > >> Tom Lane wrote: >> > "David G. Johnston" writes: >> > >If I was going to try and read it like a book I'd want the extra >> > > white-space to make doing so easier (white-space gives the eye a >> breather >> > > when done with a particular concept) - and the length wouldn't really >> > > matter since I'd just make a single pass and be done with it. But the >> > > planned usage is for quick lookup of options that you know (or at >> least >> > > suspect) exist and which you probably have an approximate idea of how >> they >> > > are spelled. The all-caps and left-justified block headers are >> distinct >> > > enough to scan down - though I'd consider indenting 4 spaces instead >> of 2 >> > > to make that even easier (less effort to ignore the indented lines >> since >> > > ignoring nothing is easier than ignoring something). Having more >> fit on >> > > one screen makes that vertical skimming considerably easier as well >> (no >> > > break and re-acquire when scrolling in a new page). >> > >> > Hmm, indenting the descriptions a couple more spaces might be a workable >> > compromise. Anyone want to try that and see what it looks like? >> > Preferably somebody who's not happy with the current layout ;-) >> >> I have to admit that adding two spaces makes it look a lot more >> acceptable to me. >> > > I did some tests now, and when the name of variable is a upper case > string, then are acceptable (although with empty line space it is better). > > for pset variables (is lower case), then reading is not too friendly still. > > Sure - four spaces is better than two - but readability is not good. > > There can be another reason of feeling vertical spaces - the size of > chars. I am using probably small fonts - I am using X windows and my > typical terminal windows is half of screen (I have T520 Lenovo) about 60 > rows and 120 columns. > > Please, look to document https://github.com/darold/ora2pg README and try > to remove empty lines. > I am looking on man pagers - and there are very well readable The rules are simply - when some variables are short - less than 6 chars, then it description and label are on same line. Between items are empty line - see "man less" ESC-} or ^RIGHTARROW Scroll horizontally right to show the end of the longest displayed line. ESC-{ or ^LEFTARROW Scroll horizontally left back to the first column. r or ^R or ^L Repaint the screen. R Repaint the screen, discarding any buffered input. Useful if the file is changing while it is being viewed. F Scroll forward, and keep trying to read when the end of file is reached. Normally this command would be used when already at the end of the file. It is a way to moni‐ tor the tail of a file which is growing while it is being viewed. (The behavior is similar to the "tail -f" command.) ESC-F Like F, but as soon as a line is found which matches the last search pattern, the terminal bell is rung and forward scrolling stops. > Regards > > Pavel > > >> >> (I'd tweak the description of PSQL_EDITOR_LINENUMBER_ARG by replacing >> "how to" with "mechanism to" while at it, by the way. It took me a >> while to understand what it was and I first thought the description was >> completely bogus.) >> >> -- >> Álvaro Herrerahttps://www.2ndQuadrant.com/ >> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >> > >
Re: [HACKERS] psql: new help related to variables are not too readable
2017-09-14 15:17 GMT+02:00 Alvaro Herrera: > Tom Lane wrote: > > "David G. Johnston" writes: > > >If I was going to try and read it like a book I'd want the extra > > > white-space to make doing so easier (white-space gives the eye a > breather > > > when done with a particular concept) - and the length wouldn't really > > > matter since I'd just make a single pass and be done with it. But the > > > planned usage is for quick lookup of options that you know (or at least > > > suspect) exist and which you probably have an approximate idea of how > they > > > are spelled. The all-caps and left-justified block headers are > distinct > > > enough to scan down - though I'd consider indenting 4 spaces instead > of 2 > > > to make that even easier (less effort to ignore the indented lines > since > > > ignoring nothing is easier than ignoring something). Having more fit > on > > > one screen makes that vertical skimming considerably easier as well (no > > > break and re-acquire when scrolling in a new page). > > > > Hmm, indenting the descriptions a couple more spaces might be a workable > > compromise. Anyone want to try that and see what it looks like? > > Preferably somebody who's not happy with the current layout ;-) > > I have to admit that adding two spaces makes it look a lot more > acceptable to me. > I did some tests now, and when the name of variable is a upper case string, then are acceptable (although with empty line space it is better). for pset variables (is lower case), then reading is not too friendly still. Sure - four spaces is better than two - but readability is not good. There can be another reason of feeling vertical spaces - the size of chars. I am using probably small fonts - I am using X windows and my typical terminal windows is half of screen (I have T520 Lenovo) about 60 rows and 120 columns. Please, look to document https://github.com/darold/ora2pg README and try to remove empty lines. Regards Pavel > > (I'd tweak the description of PSQL_EDITOR_LINENUMBER_ARG by replacing > "how to" with "mechanism to" while at it, by the way. It took me a > while to understand what it was and I first thought the description was > completely bogus.) > > -- > Álvaro Herrerahttps://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
Re: [HACKERS] psql: new help related to variables are not too readable
Tom Lane wrote: > "David G. Johnston"writes: > >If I was going to try and read it like a book I'd want the extra > > white-space to make doing so easier (white-space gives the eye a breather > > when done with a particular concept) - and the length wouldn't really > > matter since I'd just make a single pass and be done with it. But the > > planned usage is for quick lookup of options that you know (or at least > > suspect) exist and which you probably have an approximate idea of how they > > are spelled. The all-caps and left-justified block headers are distinct > > enough to scan down - though I'd consider indenting 4 spaces instead of 2 > > to make that even easier (less effort to ignore the indented lines since > > ignoring nothing is easier than ignoring something). Having more fit on > > one screen makes that vertical skimming considerably easier as well (no > > break and re-acquire when scrolling in a new page). > > Hmm, indenting the descriptions a couple more spaces might be a workable > compromise. Anyone want to try that and see what it looks like? > Preferably somebody who's not happy with the current layout ;-) I have to admit that adding two spaces makes it look a lot more acceptable to me. (I'd tweak the description of PSQL_EDITOR_LINENUMBER_ARG by replacing "how to" with "mechanism to" while at it, by the way. It took me a while to understand what it was and I first thought the description was completely bogus.) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] psql: new help related to variables are not too readable
Hello, Personnally I'm fine with a pager, so vertical spacing is fine. I just do not like paging horizontally. -1 [...] If I was going to try and read it like a book I'd want the extra white-space to make doing so easier (white-space gives the eye a breather when done with a particular concept) - and the length wouldn't really matter since I'd just make a single pass and be done with it. But the planned usage is for quick lookup of options that you know (or at least suspect) exist and which you probably have an approximate idea of how they are spelled. The all-caps and left-justified block headers are distinct enough to scan down - though I'd consider indenting 4 spaces instead of 2 to make that even easier (less effort to ignore the indented lines since ignoring nothing is easier than ignoring something). Having more fit on one screen makes that vertical skimming considerably easier as well (no break and re-acquire when scrolling in a new page). Interesting and fine arguments! So I'll agree that in an absolute sense reading the whole of the content in its condensed form is more difficult than if there were blank lines in between each block, but usability for the intended purpose is better in the current form. As far as usability is concerned, I most often use the "/" pager search feature, or page down to scan everything. Both uses are not really hampered by skipping lines, but I can leave with that as well. Help formatting could be an option, but that would require more coding and I'm not sure of the i18n aspect. -- 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] psql: new help related to variables are not too readable
"David G. Johnston"writes: >If I was going to try and read it like a book I'd want the extra > white-space to make doing so easier (white-space gives the eye a breather > when done with a particular concept) - and the length wouldn't really > matter since I'd just make a single pass and be done with it. But the > planned usage is for quick lookup of options that you know (or at least > suspect) exist and which you probably have an approximate idea of how they > are spelled. The all-caps and left-justified block headers are distinct > enough to scan down - though I'd consider indenting 4 spaces instead of 2 > to make that even easier (less effort to ignore the indented lines since > ignoring nothing is easier than ignoring something). Having more fit on > one screen makes that vertical skimming considerably easier as well (no > break and re-acquire when scrolling in a new page). Hmm, indenting the descriptions a couple more spaces might be a workable compromise. Anyone want to try that and see what it looks like? Preferably somebody who's not happy with the current layout ;-) 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] psql: new help related to variables are not too readable
On Wed, Sep 13, 2017 at 12:46 PM, Fabien COELHOwrote: > > Hello Tom, > > Probably it needs some rebase after Tom committed result status variables. >>> >> >> As it is a style thing, ISTM that the patch is ready if most people agree >>> that it is better this way and there is no strong veto against. >>> >> >> FWIW, I think it's a bad idea. We already nearly-doubled the vertical >> space required for this variable list. That was a heavy cost --- and we >> already got at least one complaint about it --- but it seemed warranted >> to avoid having to deal with very constrained variable descriptions. >> This proposes to make the vertical space nearly triple what it was in v10. >> In a typical-size window that's going to have a pretty severe impact on >> how much of the list you can see at once. And the readability gain is >> (at least to my eyes) very marginal. >> > > Ok, you do not like it. As Pavel said, it is subjective. When it is a > matter of taste, people tend to differ, someone will always complain, one > way or another, and they are neither right nor wrong. > > So, is it a -1 or a veto? > > If it is the later, the patch can be marked as "Rejected" and everybody > will get more time for other things:-) > > If it is a not a veto, people can continue to give their opinions. > Personnally I'm fine with a pager, so vertical spacing is fine. I just do > not like paging horizontally. -1 I don't fully by the "it is subjective" argument - I'm by no means an expert but there are many people out there who have done considerable research on human perception that there is at least room for non-subjective evaluation. Below is my attempt. If I was going to try and read it like a book I'd want the extra white-space to make doing so easier (white-space gives the eye a breather when done with a particular concept) - and the length wouldn't really matter since I'd just make a single pass and be done with it. But the planned usage is for quick lookup of options that you know (or at least suspect) exist and which you probably have an approximate idea of how they are spelled. The all-caps and left-justified block headers are distinct enough to scan down - though I'd consider indenting 4 spaces instead of 2 to make that even easier (less effort to ignore the indented lines since ignoring nothing is easier than ignoring something). Having more fit on one screen makes that vertical skimming considerably easier as well (no break and re-acquire when scrolling in a new page). So I'll agree that in an absolute sense reading the whole of the content in its condensed form is more difficult than if there were blank lines in between each block, but usability for the intended purpose is better in the current form. David J.
Re: [HACKERS] psql: new help related to variables are not too readable
Hello Tom, Probably it needs some rebase after Tom committed result status variables. As it is a style thing, ISTM that the patch is ready if most people agree that it is better this way and there is no strong veto against. FWIW, I think it's a bad idea. We already nearly-doubled the vertical space required for this variable list. That was a heavy cost --- and we already got at least one complaint about it --- but it seemed warranted to avoid having to deal with very constrained variable descriptions. This proposes to make the vertical space nearly triple what it was in v10. In a typical-size window that's going to have a pretty severe impact on how much of the list you can see at once. And the readability gain is (at least to my eyes) very marginal. Ok, you do not like it. As Pavel said, it is subjective. When it is a matter of taste, people tend to differ, someone will always complain, one way or another, and they are neither right nor wrong. So, is it a -1 or a veto? If it is the later, the patch can be marked as "Rejected" and everybody will get more time for other things:-) If it is a not a veto, people can continue to give their opinions. Personnally I'm fine with a pager, so vertical spacing is fine. I just do not like paging horizontally. -- 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] psql: new help related to variables are not too readable
Alvaro Herrerawrites: > Why is it that we're not opening the pager automatically when this help > is invoked via psql --help=variables? "\? variables" already does that. Hm, given that output from a -c option does get paginated (I just checked), maybe that should happen. 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] psql: new help related to variables are not too readable
Most of the time I suppose you'd search (using the pager's search function) whatever you're looking for, rather than read the whole page from top to bottom. Why is it that we're not opening the pager automatically when this help is invoked via psql --help=variables? "\? variables" already does that. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] psql: new help related to variables are not too readable
2017-09-13 16:11 GMT+02:00 Tom Lane: > Fabien COELHO writes: > >> I'll assign this patch to next commitfest > > > Probably it needs some rebase after Tom committed result status > variables. > > > As it is a style thing, ISTM that the patch is ready if most people agree > > that it is better this way and there is no strong veto against. > > FWIW, I think it's a bad idea. We already nearly-doubled the vertical > space required for this variable list. That was a heavy cost --- and we > already got at least one complaint about it --- but it seemed warranted > to avoid having to deal with very constrained variable descriptions. > This proposes to make the vertical space nearly triple what it was in v10. > In a typical-size window that's going to have a pretty severe impact on > how much of the list you can see at once. And the readability gain is > (at least to my eyes) very marginal. > unfortunately - readability is subjective. Now it is really hard to read to me. I understand so text is long enough already, and with lot of empty lines, it will be longer. because we cannot to use colors or some effects (bold, ..) I am not feeling to well when I read it. I proposed it because readability of current design is not too good - but it is not major topic for me - so should not to push this topic too much. Somebody can read it better, some not - it is subjective. Regards Pavel > > regards, tom lane >
Re: [HACKERS] psql: new help related to variables are not too readable
Fabien COELHOwrites: >> I'll assign this patch to next commitfest > Probably it needs some rebase after Tom committed result status variables. > As it is a style thing, ISTM that the patch is ready if most people agree > that it is better this way and there is no strong veto against. FWIW, I think it's a bad idea. We already nearly-doubled the vertical space required for this variable list. That was a heavy cost --- and we already got at least one complaint about it --- but it seemed warranted to avoid having to deal with very constrained variable descriptions. This proposes to make the vertical space nearly triple what it was in v10. In a typical-size window that's going to have a pretty severe impact on how much of the list you can see at once. And the readability gain is (at least to my eyes) very marginal. 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] psql - add special variable to reflect the last query status
One thing we could think about if this seems too high is to drop ROW_COUNT. I'm unconvinced that it has a real use-case, and it seems to be taking more than its share of the work in non-error cases, because it turns out that PQcmdTuples() is not an amazingly cheap function. I do think that a small overhead on a contrived case is worth removing the feature, as it is really insignificant on any realistic case. Please read: I do NOT think that... -- 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] psql - add special variable to reflect the last query status
Hello Tom, I put back SetResultVariables function which is called twice, for SQL queries and the new descriptions. It worked out of the box with DECLARE which is just another SQL statement, so maybe I did not understood the cursor issue you were signaling... No, I was concerned about ExecQueryUsingCursor(), which is used when FETCH_COUNT is enabled. It's sort of a pain because you have to accumulate the row count across multiple PGresults. If you don't, then FETCH_COUNT mode isn't transparent, which it's supposed to be. Please allow me to disagree a little with this semantics. ISTM that the semantics of the simple previous implementation was fine, "number of rows returned by previous statement". If you do "FETCH 3 ...", then you get between 0 and 3 rows... Good. If you do it again, same... I'm not sure having an accumulation semantics helps a lot, because it creates an exception, and moreover I can think of legitimate use case where counting only last statement rows would be useful, eg to check that we are done with a cursor and it can be closed. If someone really wants to accumulate, it can be done by hand quite simply, currently as: SELECT :ROW_COUNT + :accum AS accum \gset or client side: \set accum `expr :ROW_COUNT + :accum` and maybe some day something like: \let accum :ROW_COUNT + :accum I did some performance testing of my own, based on this possibly-silly test case: [...] The idea was to run a trivial query and minimize all other psql overhead, particularly results-printing. With this, "perf" told me that SetResultVariables and its called functions accounted for 1.5% of total CPU (including the server processes). That's kind of high, but it's probably tolerable considering that any real application would involve both far more server work per query and far more psql work (at least for SELECTs). This seems pretty reasonable to me, and is consistent with my 1% elapsed time measure on a silent "SELECT;". One thing we could think about if this seems too high is to drop ROW_COUNT. I'm unconvinced that it has a real use-case, and it seems to be taking more than its share of the work in non-error cases, because it turns out that PQcmdTuples() is not an amazingly cheap function. I do think that a small overhead on a contrived case is worth removing the feature, as it is really insignificant on any realistic case. -- 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] psql: new help related to variables are not too readable
Finally, as vertical scrolling is mandatory, I would be fine with skipping lines with entries for readability, but it is just a matter of taste and I expect there should be half a dozen different opinions on the matter of formatting. FWIW, +1 to extra lines from me - I find it way more readable, as it clearly separates the items. +1 As already said above +1 for me for having more space. I'll assign this patch to next commitfest Probably it needs some rebase after Tom committed result status variables. As it is a style thing, ISTM that the patch is ready if most people agree that it is better this way and there is no strong veto against. -- 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] psql: new help related to variables are not too readable
2017-09-09 1:30 GMT+02:00 Alvaro Herrera: > Tomas Vondra wrote: > > > > Finally, as vertical scrolling is mandatory, I would be fine with > > > skipping lines with entries for readability, but it is just a matter of > > > taste and I expect there should be half a dozen different opinions on > > > the matter of formatting. > > > > FWIW, +1 to extra lines from me - I find it way more readable, as it > > clearly separates the items. > > +1 > I'll assign this patch to next commitfest > > -- > Álvaro Herrerahttps://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
Re: [HACKERS] psql - add special variable to reflect the last query status
Fabien COELHOwrites: > See v9 attached. I've pushed this with some editorialization. > I put back SetResultVariables function which is called twice, for SQL > queries and the new descriptions. It worked out of the box with DECLARE > which is just another SQL statement, so maybe I did not understood the > cursor issue you were signaling... No, I was concerned about ExecQueryUsingCursor(), which is used when FETCH_COUNT is enabled. It's sort of a pain because you have to accumulate the row count across multiple PGresults. If you don't, then FETCH_COUNT mode isn't transparent, which it's supposed to be. I did some performance testing of my own, based on this possibly-silly test case: perl -e 'for($i=0;$i<999;$i++) {print "set enable_seqscan=0;\n";}' | psql -q The idea was to run a trivial query and minimize all other psql overhead, particularly results-printing. With this, "perf" told me that SetResultVariables and its called functions accounted for 1.5% of total CPU (including the server processes). That's kind of high, but it's probably tolerable considering that any real application would involve both far more server work per query and far more psql work (at least for SELECTs). One thing we could think about if this seems too high is to drop ROW_COUNT. I'm unconvinced that it has a real use-case, and it seems to be taking more than its share of the work in non-error cases, because it turns out that PQcmdTuples() is not an amazingly cheap function. 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] psql - add special variable to reflect the last query status
Well, if we provided a different SQLSTATE for each qualitatively different type of libpq error, that might well be useful enough to justify some risk of application breakage. But replacing a constant string that we've had for ~15 years with a different constraint string isn't doing anything about the lack-of-information problem you're complaining about. True. Well, the original point here was whether psql ought to be doing something to mask libpq's (mis) behavior. I'm inclined to think not: if it doesn't get a SQLSTATE from the PGresult, it should just set the sqlstate variables to empty strings. See v9 attached. -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index a74caf8..b994fcd 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3518,6 +3518,16 @@ bar + ERROR + + + Whether the last query failed, as a boolean. + See also SQLSTATE. + + + + + FETCH_COUNT @@ -3654,6 +3664,18 @@ bar + LAST_ERROR_SQLSTATE + LAST_ERROR_MESSAGE + + + The error code and associated error message of the last + error, or "0" and empty strings if no error occured + since the beginning of the script. + + + + + ON_ERROR_ROLLBACK @@ -3722,6 +3744,25 @@ bar + ROW_COUNT + + + How many rows were returned or affected by the last query. + + + + + + SQLSTATE + + + The error code associated to the last query, or + 0 if no error occured. + + + + + QUIET diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index b997058..cc7e3aa 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -493,7 +493,6 @@ ResetCancelConn(void) #endif } - /* * AcceptResult * @@ -971,6 +970,45 @@ loop_exit: return success; } +/* + * Set special variables + * - ERROR: true/false, whether an error occurred + * - SQLSTATE: code of error, or "0", or "" + * - LAST_ERROR_SQLSTATE: same for last error + * - LAST_ERROR_MESSAGE: message of last error + * - ROW_COUNT: how many rows were returned or affected, or "0" + */ +static void +SetResultVariables(PGresult *results, bool success) +{ + if (success) + { + char *ntuples = PQcmdTuples(results); + SetVariable(pset.vars, "ERROR", "false"); + SetVariable(pset.vars, "SQLSTATE", "0"); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); + } + else + { + char *code = PQresultErrorField(results, PG_DIAG_SQLSTATE); + char *mesg = PQresultErrorField(results, PG_DIAG_MESSAGE_PRIMARY); + + SetVariable(pset.vars, "ERROR", "true"); + + /* + * if there is no code, use an empty string? + * libpq may return such thing on internal errors + * (lost connection, EOM). + */ + if (code == NULL) + code = "" ; + + SetVariable(pset.vars, "SQLSTATE", code); + SetVariable(pset.vars, "LAST_ERROR_SQLSTATE", code); + SetVariable(pset.vars, "LAST_ERROR_MESSAGE", mesg ? mesg : ""); + SetVariable(pset.vars, "ROW_COUNT", "0"); + } +} /* * ProcessResult: utility function for use by SendQuery() only @@ -1107,6 +1145,8 @@ ProcessResult(PGresult **results) first_cycle = false; } + SetResultVariables(*results, success); + /* may need this to recover from conn loss during COPY */ if (!first_cycle && !CheckConnection()) return false; @@ -1214,7 +1254,6 @@ PrintQueryResults(PGresult *results) return success; } - /* * SendQuery: send the query string to the backend * (and print out results) @@ -1523,7 +1562,11 @@ DescribeQuery(const char *query, double *elapsed_msec) * good thing because libpq provides no easy way to do that.) */ results = PQprepare(pset.db, "", query, 0, NULL); - if (PQresultStatus(results) != PGRES_COMMAND_OK) + OK = PQresultStatus(results) == PGRES_COMMAND_OK; + + SetResultVariables(results, OK); + + if (!OK) { psql_error("%s", PQerrorMessage(pset.db)); ClearOrSaveResult(results); diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index 4d1c0ec..ae951f5 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -337,7 +337,7 @@ helpVariables(unsigned short int pager) * Windows builds currently print one more line than non-Windows builds. * Using the larger number is fine. */ - output = PageOutput(147, pager ? &(pset.popt.topt) : NULL); + output = PageOutput(155, pager ? &(pset.popt.topt) : NULL); fprintf(output, _("List of specially treated variables\n\n")); @@ -360,6 +360,8 @@ helpVariables(unsigned short int pager) "if set to \"noexec\", just show them without execution\n")); fprintf(output, _(" ENCODING\n" "current client character set encoding\n")); + fprintf(output, _(" ERROR\n" +
Re: [HACKERS] psql - add special variable to reflect the last query status
Robert Haaswrites: > Well, if we provided a different SQLSTATE for each qualitatively > different type of libpq error, that might well be useful enough to > justify some risk of application breakage. But replacing a constant > string that we've had for ~15 years with a different constraint string > isn't doing anything about the lack-of-information problem you're > complaining about. True. Well, the original point here was whether psql ought to be doing something to mask libpq's (mis) behavior. I'm inclined to think not: if it doesn't get a SQLSTATE from the PGresult, it should just set the sqlstate variables to empty strings. 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] psql - add special variable to reflect the last query status
On Tue, Sep 12, 2017 at 3:12 PM, Tom Lanewrote: >> I think this is a bad plan. Right now, libpq sets no SQLSTATE for >> internally generated errors; it is almost certain that there are >> applications testing for an empty SQLSTATE to notice when they're >> getting an error from libpq. EnterpriseDB had a support ticket quite >> recently where this precise behavior was at issue. Changing it will >> break stuff, so we shouldn't do it unless there's a really compelling >> benefit. Universally returning PQ000 is not a sufficient improvement >> over universally returning the empty string to justify the risk of >> application breakage. > > I don't think I want to buy this argument, because the logical conclusion > of it is that we can never fix libpq to offer proper SQLSTATEs for > client-side errors. Admittedly, the fact that nobody's bothered to do so > in ~15 years may indicate that nobody cares ... but I would think that > at least it'd be useful to distinguish, say, ENOMEM from connection loss. > Saying we can't do it for compatibility reasons doesn't sound great > to me. Especially when you've not provided any hard evidence as to why > the current lack-of-information is useful. Well, if we provided a different SQLSTATE for each qualitatively different type of libpq error, that might well be useful enough to justify some risk of application breakage. But replacing a constant string that we've had for ~15 years with a different constraint string isn't doing anything about the lack-of-information problem you're complaining about. -- 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] psql - add special variable to reflect the last query status
Robert Haaswrites: > On Tue, Sep 12, 2017 at 1:23 PM, Fabien COELHO wrote: >> I added two error codes, which is debatable. One is used hardcoded by libpq >> if no diagnostic is found, and the other by psql if libpq returned something >> empty, which might happen if psql is linked with an older libpq, maybe. I do >> not know how to trigger such errors anyway, so this is rather academic. > I think this is a bad plan. Right now, libpq sets no SQLSTATE for > internally generated errors; it is almost certain that there are > applications testing for an empty SQLSTATE to notice when they're > getting an error from libpq. EnterpriseDB had a support ticket quite > recently where this precise behavior was at issue. Changing it will > break stuff, so we shouldn't do it unless there's a really compelling > benefit. Universally returning PQ000 is not a sufficient improvement > over universally returning the empty string to justify the risk of > application breakage. I don't think I want to buy this argument, because the logical conclusion of it is that we can never fix libpq to offer proper SQLSTATEs for client-side errors. Admittedly, the fact that nobody's bothered to do so in ~15 years may indicate that nobody cares ... but I would think that at least it'd be useful to distinguish, say, ENOMEM from connection loss. Saying we can't do it for compatibility reasons doesn't sound great to me. Especially when you've not provided any hard evidence as to why the current lack-of-information is useful. 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] psql - add special variable to reflect the last query status
2017-09-12 20:43 GMT+02:00 Robert Haas: > On Tue, Sep 12, 2017 at 1:23 PM, Fabien COELHO > wrote: > > I added two error codes, which is debatable. One is used hardcoded by > libpq > > if no diagnostic is found, and the other by psql if libpq returned > something > > empty, which might happen if psql is linked with an older libpq, maybe. > I do > > not know how to trigger such errors anyway, so this is rather academic. > > I think this is a bad plan. Right now, libpq sets no SQLSTATE for > internally generated errors; it is almost certain that there are > applications testing for an empty SQLSTATE to notice when they're > getting an error from libpq. EnterpriseDB had a support ticket quite > recently where this precise behavior was at issue. Changing it will > break stuff, so we shouldn't do it unless there's a really compelling > benefit. Universally returning PQ000 is not a sufficient improvement > over universally returning the empty string to justify the risk of > application breakage. > +1 Pavel > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
Re: [HACKERS] psql - add special variable to reflect the last query status
On Tue, Sep 12, 2017 at 1:23 PM, Fabien COELHOwrote: > I added two error codes, which is debatable. One is used hardcoded by libpq > if no diagnostic is found, and the other by psql if libpq returned something > empty, which might happen if psql is linked with an older libpq, maybe. I do > not know how to trigger such errors anyway, so this is rather academic. I think this is a bad plan. Right now, libpq sets no SQLSTATE for internally generated errors; it is almost certain that there are applications testing for an empty SQLSTATE to notice when they're getting an error from libpq. EnterpriseDB had a support ticket quite recently where this precise behavior was at issue. Changing it will break stuff, so we shouldn't do it unless there's a really compelling benefit. Universally returning PQ000 is not a sufficient improvement over universally returning the empty string to justify the risk of application breakage. -- 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] psql - add special variable to reflect the last query status
Hello Tom, Yep, I thought I was optimistic:-) Can I add a special SQLSTATE for that situation where libpq did not report an error? Meh. If we're going to do that I think it might be better to hack libpq itself to do so, ie, force PQresultErrorField(..., PG_DIAG_SQLSTATE) to always return something. But it seems like a hack either way. I would not have took the liberty to hack into libpq internals for such a small front-end feature. However I agree that having libpq always return some diagnostic, even if it means "something unclear happened, sorry not to be very precise", would be better. Here is an attempt at implementing your suggestions. I added two error codes, which is debatable. One is used hardcoded by libpq if no diagnostic is found, and the other by psql if libpq returned something empty, which might happen if psql is linked with an older libpq, maybe. I do not know how to trigger such errors anyway, so this is rather academic. I put back SetResultVariables function which is called twice, for SQL queries and the new descriptions. It worked out of the box with DECLARE which is just another SQL statement, so maybe I did not understood the cursor issue you were signaling... -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index a74caf8..b994fcd 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3518,6 +3518,16 @@ bar + ERROR + + + Whether the last query failed, as a boolean. + See also SQLSTATE. + + + + + FETCH_COUNT @@ -3654,6 +3664,18 @@ bar + LAST_ERROR_SQLSTATE + LAST_ERROR_MESSAGE + + + The error code and associated error message of the last + error, or "0" and empty strings if no error occured + since the beginning of the script. + + + + + ON_ERROR_ROLLBACK @@ -3722,6 +3744,25 @@ bar + ROW_COUNT + + + How many rows were returned or affected by the last query. + + + + + + SQLSTATE + + + The error code associated to the last query, or + 0 if no error occured. + + + + + QUIET diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index b997058..bbffcac 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -493,7 +493,6 @@ ResetCancelConn(void) #endif } - /* * AcceptResult * @@ -971,6 +970,44 @@ loop_exit: return success; } +/* + * Set special variables + * - ERROR: true/false, whether an error occurred + * - SQLSTATE: code of error, or "0" + * - LAST_ERROR_SQLSTATE: same for last error + * - LAST_ERROR_MESSAGE: message of last error + * - ROW_COUNT: how many rows were returned or affected, or "0" + */ +static void +SetResultVariables(PGresult *results, bool success) +{ + if (success) + { + char *ntuples = PQcmdTuples(results); + SetVariable(pset.vars, "ERROR", "false"); + SetVariable(pset.vars, "SQLSTATE", "0"); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); + } + else + { + char *code = PQresultErrorField(results, PG_DIAG_SQLSTATE); + char *mesg = PQresultErrorField(results, PG_DIAG_MESSAGE_PRIMARY); + + SetVariable(pset.vars, "ERROR", "true"); + + /* + * Ensure that something sensible is shown, + * without assumption about libpq implementation + */ + if (code == NULL || *code == '\0') + code = "PQ001" /* ERROR_LIBPQ_EMPTY_SQLSTATE */ ; + + SetVariable(pset.vars, "SQLSTATE", code); + SetVariable(pset.vars, "LAST_ERROR_SQLSTATE", code); + SetVariable(pset.vars, "LAST_ERROR_MESSAGE", mesg ? mesg : ""); + SetVariable(pset.vars, "ROW_COUNT", "0"); + } +} /* * ProcessResult: utility function for use by SendQuery() only @@ -1107,6 +1144,8 @@ ProcessResult(PGresult **results) first_cycle = false; } + SetResultVariables(*results, success); + /* may need this to recover from conn loss during COPY */ if (!first_cycle && !CheckConnection()) return false; @@ -1214,7 +1253,6 @@ PrintQueryResults(PGresult *results) return success; } - /* * SendQuery: send the query string to the backend * (and print out results) @@ -1523,7 +1561,11 @@ DescribeQuery(const char *query, double *elapsed_msec) * good thing because libpq provides no easy way to do that.) */ results = PQprepare(pset.db, "", query, 0, NULL); - if (PQresultStatus(results) != PGRES_COMMAND_OK) + OK = PQresultStatus(results) == PGRES_COMMAND_OK; + + SetResultVariables(results, OK); + + if (!OK) { psql_error("%s", PQerrorMessage(pset.db)); ClearOrSaveResult(results); diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index 4d1c0ec..ae951f5 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -337,7 +337,7 @@
Re: [HACKERS] psql - add special variable to reflect the last query status
2017-09-11 20:46 GMT+02:00 Fabien COELHO: > > I think you're overly optimistic to believe that every failure will have a SQLSTATE; I don't think that's true for libpq-reported errors, such as connection loss. >>> >> Yep, I thought I was optimistic:-) Can I add a special SQLSTATE for that >>> situation where libpq did not report an error? >>> >> >> Meh. If we're going to do that I think it might be better to hack >> libpq itself to do so, ie, force PQresultErrorField(..., PG_DIAG_SQLSTATE) >> to always return something. But it seems like a hack either way. >> > > I would not have took the liberty to hack into libpq internals for such a > small front-end feature. However I agree that having libpq always return > some diagnostic, even if it means "something unclear happened, sorry not to > be very precise", would be better. probably better don't do it before somebody implements this are correctly .. some temporary solution can introduce possible compatibility issues. If SQLSTATE has not know value, then it should be NULL or maybe empty string. Pavel > > > -- > Fabien. >
Re: [HACKERS] psql - add special variable to reflect the last query status
I think you're overly optimistic to believe that every failure will have a SQLSTATE; I don't think that's true for libpq-reported errors, such as connection loss. Yep, I thought I was optimistic:-) Can I add a special SQLSTATE for that situation where libpq did not report an error? Meh. If we're going to do that I think it might be better to hack libpq itself to do so, ie, force PQresultErrorField(..., PG_DIAG_SQLSTATE) to always return something. But it seems like a hack either way. I would not have took the liberty to hack into libpq internals for such a small front-end feature. However I agree that having libpq always return some diagnostic, even if it means "something unclear happened, sorry not to be very precise", would be better. -- 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] psql - add special variable to reflect the last query status
Fabien COELHOwrites: >> I think you're overly optimistic to believe that every failure will >> have a SQLSTATE; I don't think that's true for libpq-reported errors, >> such as connection loss. > Yep, I thought I was optimistic:-) Can I add a special SQLSTATE for that > situation where libpq did not report an error? Meh. If we're going to do that I think it might be better to hack libpq itself to do so, ie, force PQresultErrorField(..., PG_DIAG_SQLSTATE) to always return something. But it seems like a hack either way. 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] psql - add special variable to reflect the last query status
Hello Tom, Hm. Looking closer at this, I see that it doesn't work so well after all to put the variable-setting code in ProcessResult: that fails to cover the ExecQueryUsingCursor code path. Ok, I'll investigate this path. And it also fails to cover DescribeQuery, which arguably should set these variables as well And this one. -- certainly so if it gets a failure. Maybe you could create a small subroutine along the lines of SetResultVariables(PGresult *result, bool success) for all three places to call. (ProcessResult certainly has already decided whether it's got a success, and I think the other paths would know that as well, so no need to re-extract it from the PGresult.) Ok. I think you're overly optimistic to believe that every failure will have a SQLSTATE; I don't think that's true for libpq-reported errors, such as connection loss. Yep, I thought I was optimistic:-) Can I add a special SQLSTATE for that situation where libpq did not report an error? Using upper-case TRUE/FALSE for the values of ERROR seems a bit ugly to me; we generally use lower case for other variable values, so I'd go with true/false. Ok. The choice is not aesthetic but systematic: I use upper-case for all SQL keywords, and lower-case or capitalized for anything user land. I can put lower-case if you want. -- 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] psql - add special variable to reflect the last query status
Fabien COELHOwrites: > Small v7 update, sorry for the noise. Hm. Looking closer at this, I see that it doesn't work so well after all to put the variable-setting code in ProcessResult: that fails to cover the ExecQueryUsingCursor code path. And it also fails to cover DescribeQuery, which arguably should set these variables as well -- certainly so if it gets a failure. Maybe you could create a small subroutine along the lines of SetResultVariables(PGresult *result, bool success) for all three places to call. (ProcessResult certainly has already decided whether it's got a success, and I think the other paths would know that as well, so no need to re-extract it from the PGresult.) I think you're overly optimistic to believe that every failure will have a SQLSTATE; I don't think that's true for libpq-reported errors, such as connection loss. Using upper-case TRUE/FALSE for the values of ERROR seems a bit ugly to me; we generally use lower case for other variable values, so I'd go with true/false. 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] psql: new help related to variables are not too readable
Tomas Vondra wrote: > > Finally, as vertical scrolling is mandatory, I would be fine with > > skipping lines with entries for readability, but it is just a matter of > > taste and I expect there should be half a dozen different opinions on > > the matter of formatting. > > FWIW, +1 to extra lines from me - I find it way more readable, as it > clearly separates the items. +1 -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] psql: new help related to variables are not too readable
Tomas Vondra wrote: > That's not what I meant. I've never felt a strong urge to avoid wrapping > at 80 chars when translating strings - simply translate in the most > meaningful way, if it gets longer than 80 chars and can't be easily > shortened, just wrap. If there are 60 or 80 characters does not change > this much - 80 chars may allow more unwrapped strings, of course, but > it's a minor change for the translators. Or at least me, others may > disagree, of course. The difficulty varies across languages since some are more compact than others, and the choice of wrapping or not is not consistent across translations. As an example, in the previous version, the first variable comes out as: en AUTOCOMMIT if set, successful SQL commands are automatically committed de AUTOCOMMIT wenn gesetzt werden alle erfolgreichen SQL-Befehle automatisch committet es AUTOCOMMIT si está definida, órdenes SQL exitosas se comprometen automáticamente fr AUTOCOMMIT si activé, les commandes SQL réussies sont automatiquement validées he AUTOCOMMITאם הופעל, פקודות SQL מחויבות באופן אוטומטי it AUTOCOMMIT se impostato, i comandi SQL riusciti sono salvati automaticamente ko AUTOCOMMIT 지정 하면 SQL 명령이 성공하면 자동으로 커밋 pl AUTOCOMMIT gdy ustawiony, polecenia SQL zakończone powodzeniem są automatycznie zatwierdzone pt_BR AUTOCOMMIT se definido, comandos SQL bem sucedidos são automaticamente efetivados ru AUTOCOMMIT если установлен, успешные SQL-команды фиксируются автоматически sv AUTOCOMMIT om satt, efterföljande SQL-kommandon commit:as automatiskt zh_CN AUTOCOMMIT 如果被设置,成功的SQL命令将会被自动提交 The original line in english has exactly 80 characters. When translated in other latin languages, it tends to be a bit longer. German and spanish translators have taken the trouble to break the message into two lines of less than 80 chars with the second one nicely indented, also aligned in the .po file: msgid " AUTOCOMMIT if set, successful SQL commands are automatically committed\n" msgstr "" " AUTOCOMMIT si está definida, órdenes SQL exitosas se comprometen\n" " automáticamente\n" But other translations are not necessarily done this way, or at least not consistently. If only for that, having more characters in the description before screen wrap should help a bit. In the above example, I think with the new presentation only the polish version wouldn't fit in 80 chars without wrapping. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- 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] psql: new help related to variables are not too readable
On 09/09/2017 01:24 AM, Tom Lane wrote: > Tomas Vondrawrites: >> The translator has exactly the same context in both cases, and the >> struggle to wrap it at 80 characters will be pretty much the same too. > > Really? With the old way, you had something under 60 characters to > work in, now it's nearly 80. I don't buy that that's not a significant > difference. It's also much less ugly if you decide you need one more > line than the English version uses. > That's not what I meant. I've never felt a strong urge to avoid wrapping at 80 chars when translating strings - simply translate in the most meaningful way, if it gets longer than 80 chars and can't be easily shortened, just wrap. If there are 60 or 80 characters does not change this much - 80 chars may allow more unwrapped strings, of course, but it's a minor change for the translators. Or at least me, others may disagree, of course. What I find way more annoying are strings where it's not clear where to wrap, because it gets prefixed by something, we insert a value while formatting the error message, etc. But that's not the case here, as both _(" " " ") and _(" ") give you the whole string. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] psql: new help related to variables are not too readable
Hi 2017-09-09 1:24 GMT+02:00 Tom Lane: > Tomas Vondra writes: > > The translator has exactly the same context in both cases, and the > > struggle to wrap it at 80 characters will be pretty much the same too. > > Really? With the old way, you had something under 60 characters to > work in, now it's nearly 80. I don't buy that that's not a significant > difference. It's also much less ugly if you decide you need one more > line than the English version uses. > > here is patch - anybody can check if with this change will be result better or not. Regards Pavel > regards, tom lane > diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index 4d1c0ec3c6..9877cc5f55 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -337,7 +337,7 @@ helpVariables(unsigned short int pager) * Windows builds currently print one more line than non-Windows builds. * Using the larger number is fine. */ - output = PageOutput(147, pager ? &(pset.popt.topt) : NULL); + output = PageOutput(206, pager ? &(pset.popt.topt) : NULL); fprintf(output, _("List of specially treated variables\n\n")); @@ -346,108 +346,108 @@ helpVariables(unsigned short int pager) fprintf(output, _(" psql --set=NAME=VALUE\n or \\set NAME VALUE inside psql\n\n")); fprintf(output, _(" AUTOCOMMIT\n" - "if set, successful SQL commands are automatically committed\n")); + "if set, successful SQL commands are automatically committed\n\n")); fprintf(output, _(" COMP_KEYWORD_CASE\n" "determines the case used to complete SQL key words\n" - "[lower, upper, preserve-lower, preserve-upper]\n")); + "[lower, upper, preserve-lower, preserve-upper]\n\n")); fprintf(output, _(" DBNAME\n" - "the currently connected database name\n")); + "the currently connected database name\n\n")); fprintf(output, _(" ECHO\n" "controls what input is written to standard output\n" - "[all, errors, none, queries]\n")); + "[all, errors, none, queries]\n\n")); fprintf(output, _(" ECHO_HIDDEN\n" "if set, display internal queries executed by backslash commands;\n" - "if set to \"noexec\", just show them without execution\n")); + "if set to \"noexec\", just show them without execution\n\n")); fprintf(output, _(" ENCODING\n" - "current client character set encoding\n")); + "current client character set encoding\n\n")); fprintf(output, _(" FETCH_COUNT\n" - "the number of result rows to fetch and display at a time (0 = unlimited)\n")); + "the number of result rows to fetch and display at a time (0 = unlimited)\n\n")); fprintf(output, _(" HISTCONTROL\n" - "controls command history [ignorespace, ignoredups, ignoreboth]\n")); + "controls command history [ignorespace, ignoredups, ignoreboth]\n\n")); fprintf(output, _(" HISTFILE\n" - "file name used to store the command history\n")); + "file name used to store the command history\n\n")); fprintf(output, _(" HISTSIZE\n" - "max number of commands to store in the command history\n")); + "max number of commands to store in the command history\n\n")); fprintf(output, _(" HOST\n" - "the currently connected database server host\n")); + "the currently connected database server host\n\n")); fprintf(output, _(" IGNOREEOF\n" - "number of EOFs needed to terminate an interactive session\n")); + "number of EOFs needed to terminate an interactive session\n\n")); fprintf(output, _(" LASTOID\n" - "value of the last affected OID\n")); + "value of the last affected OID\n\n")); fprintf(output, _(" ON_ERROR_ROLLBACK\n" - "if set, an error doesn't stop a transaction (uses implicit savepoints)\n")); + "if set, an error doesn't stop a transaction (uses implicit savepoints)\n\n")); fprintf(output, _(" ON_ERROR_STOP\n" - "stop batch execution after error\n")); + "stop batch execution after error\n\n")); fprintf(output, _(" PORT\n" - "server port of the current connection\n")); + "server port of the current connection\n\n")); fprintf(output, _(" PROMPT1\n" - "specifies the standard psql prompt\n")); + "specifies the standard psql prompt\n\n")); fprintf(output, _(" PROMPT2\n" - "specifies the prompt used when a statement continues from a previous line\n")); + "specifies the prompt used when a statement continues from a previous line\n\n")); fprintf(output, _(" PROMPT3\n" - "specifies the prompt used during COPY ... FROM STDIN\n")); + "specifies the prompt used during COPY ... FROM STDIN\n\n")); fprintf(output, _(" QUIET\n" - "run quietly (same as -q option)\n")); + "run quietly (same as -q
Re: [HACKERS] psql: new help related to variables are not too readable
Tomas Vondrawrites: > The translator has exactly the same context in both cases, and the > struggle to wrap it at 80 characters will be pretty much the same too. Really? With the old way, you had something under 60 characters to work in, now it's nearly 80. I don't buy that that's not a significant difference. It's also much less ugly if you decide you need one more line than the English version uses. 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] psql: new help related to variables are not too readable
Hi, On 09/08/2017 07:25 AM, Fabien COELHO wrote: > > Hello, > >>> PSQL_HISTORY alternative location for the command history file >>> >>> I would prefer to revert to that more compact 9.6-formatting. >> >> There was a problem with line width .. its hard to respect 80 chars > > Yes. > > Scrolling in two dimensions because it does not fit either way is not > too practical, so the idea was the it should at least fit a reasonable > terminal in the horizontal dimension, the vertical one having been > unfittable anyway for a long time. > > Once you want to do strict 80 columns, a lot of/most descriptions do not > fit and need to be split somehow on two lines, one way or another. It > seemed that > > XXX > xxx xx xxx xxx > > Is a good way to do that systematically and with giving more space and > chance for a description to fit in its line. ISTM that it was already > done like for environment variables, so it is also for homogeneity. > FWIW I also find the new formatting hard to read, as it does not clearly separate the items. The old formatting was not ideal either, though. > > It also simplify work for translators that can just follow the same > formatting and know what to do if a one line English explanation does > not fit once translated. > As someone responsible for a significant part of the Czech translation, I find this argument somewhat dubious. I don't see how this _(" " " ") simplifies the work for translators, compared to this _(" ") The translator has exactly the same context in both cases, and the struggle to wrap it at 80 characters will be pretty much the same too. The thing that would make a difference is automatic wrapping, i.e. split on spaces, then re-build into shorter than 80 characters ... > Finally, as vertical scrolling is mandatory, I would be fine with > skipping lines with entries for readability, but it is just a matter of > taste and I expect there should be half a dozen different opinions on > the matter of formatting. > FWIW, +1 to extra lines from me - I find it way more readable, as it clearly separates the items. You're right this is also a matter of taste to some degree, so I understand Erik's point about vertical scrolling. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] psql: new help related to variables are not too readable
Hello, PSQL_HISTORY alternative location for the command history file I would prefer to revert to that more compact 9.6-formatting. There was a problem with line width .. its hard to respect 80 chars Yes. Scrolling in two dimensions because it does not fit either way is not too practical, so the idea was the it should at least fit a reasonable terminal in the horizontal dimension, the vertical one having been unfittable anyway for a long time. Once you want to do strict 80 columns, a lot of/most descriptions do not fit and need to be split somehow on two lines, one way or another. It seemed that XXX xxx xx xxx xxx Is a good way to do that systematically and with giving more space and chance for a description to fit in its line. ISTM that it was already done like for environment variables, so it is also for homogeneity. It also simplify work for translators that can just follow the same formatting and know what to do if a one line English explanation does not fit once translated. Finally, as vertical scrolling is mandatory, I would be fine with skipping lines with entries for readability, but it is just a matter of taste and I expect there should be half a dozen different opinions on the matter of formatting. -- 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] psql: new help related to variables are not too readable
2017-09-08 6:36 GMT+02:00 Erik Rijkers: > On 2017-09-08 06:09, Pavel Stehule wrote: > >> Hi >> >> Now the output looks like: >> >> AUTOCOMMIT >> if set, successful SQL commands are automatically committed >> COMP_KEYWORD_CASE >> determines the case used to complete SQL key words >> [lower, upper, preserve-lower, preserve-upper] >> DBNAME >> the currently connected database name >> > [...] > >> What do you think about using new line between entries in this format? >> >> AUTOCOMMIT >> if set, successful SQL commands are automatically committed >> >> COMP_KEYWORD_CASE >> determines the case used to complete SQL key words >> [lower, upper, preserve-lower, preserve-upper] >> >> DBNAME >> the currently connected database name >> >> > I dislike it, it takes more screen space and leads to unneccessary > scroll-need. > > The 9.6.5 formatting is/was: > > AUTOCOMMIT if set, successful SQL commands are automatically > committed > COMP_KEYWORD_CASE determines the case used to complete SQL key words > [lower, upper, preserve-lower, preserve-upper] > DBNAME the currently connected database name > [...] > PGPASSWORD connection password (not recommended) > PGPASSFILE password file name > PSQL_EDITOR, EDITOR, VISUAL > editor used by the \e, \ef, and \ev commands > PSQL_EDITOR_LINENUMBER_ARG > how to specify a line number when invoking the editor > PSQL_HISTORY alternative location for the command history file > > I would prefer to revert to that more compact 9.6-formatting. There was a problem with line width .. its hard to respect 80 chars > > > > Erik Rijkers >
Re: [HACKERS] psql: new help related to variables are not too readable
On 2017-09-08 06:09, Pavel Stehule wrote: Hi Now the output looks like: AUTOCOMMIT if set, successful SQL commands are automatically committed COMP_KEYWORD_CASE determines the case used to complete SQL key words [lower, upper, preserve-lower, preserve-upper] DBNAME the currently connected database name [...] What do you think about using new line between entries in this format? AUTOCOMMIT if set, successful SQL commands are automatically committed COMP_KEYWORD_CASE determines the case used to complete SQL key words [lower, upper, preserve-lower, preserve-upper] DBNAME the currently connected database name I dislike it, it takes more screen space and leads to unneccessary scroll-need. The 9.6.5 formatting is/was: AUTOCOMMIT if set, successful SQL commands are automatically committed COMP_KEYWORD_CASE determines the case used to complete SQL key words [lower, upper, preserve-lower, preserve-upper] DBNAME the currently connected database name [...] PGPASSWORD connection password (not recommended) PGPASSFILE password file name PSQL_EDITOR, EDITOR, VISUAL editor used by the \e, \ef, and \ev commands PSQL_EDITOR_LINENUMBER_ARG how to specify a line number when invoking the editor PSQL_HISTORY alternative location for the command history file I would prefer to revert to that more compact 9.6-formatting. Erik Rijkers -- 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] psql - add special variable to reflect the last query status
Hello Pavel, I checked performance - the most fast queries are execution of simple prepared statement prepare x as select 1; -- 100 x execute x; execute x; execute x; execute x; ## patched [pavel@nemesis postgresql]$ time psql -At -1 postgres -f ~/xxx.sql > /dev/null real 0m44,887s user 0m11,703s sys 0m6,942s This is probably the most worst case, what is possible and see some slowdown - in this case there is about 10% slowdown - but it is pretty untypical - the one query was less than 50 microsec. When there will be any IO activity or network usage, than this patch doesn't create any significant overhead. Interesting. Thanks for the test. I tried to replicate with a variant without any output: "SELECT;" SELECT NOW() AS start \gset BEGIN; SELECT; -- 2^19 times END; SELECT NOW() - :'start'; The run time is about 19 µs per SELECT on my laptop. Over 33 runs each alternating master with and without the patch, I got the following stats on the measured time in seconds (average, stddev [min Q1 median Q3 max]): - with : 9.898 ± 0.158 [9.564, 9.762, 9.936, 10.037, 10.108] - without: 9.419 ± 0.294 [8.670, 9.226, 9.533, 9.625, 9.845] This seems consistent and significant. It suggests a 0.40-0.50 s difference, that is about 5%, i.e. about (under) 1 µs overhead per statement in pretty defavorable circumstances. -- 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] psql - add special variable to reflect the last query status
Hi 2017-09-06 11:14 GMT+02:00 Fabien COELHO: > > Here is a version 6. >> > > Small v7 update, sorry for the noise. > > Add testing the initial state of all variables. > > Fix typos in a comment in tests. > > Fix the documentation wrt the current implementation behavior. I rechecked last patch There is not any issue with patching. All regress tests passed I checked performance - the most fast queries are execution of simple prepared statement prepare x as select 1; -- 100 x execute x; execute x; execute x; execute x; ## patched [pavel@nemesis postgresql]$ time psql -At -1 postgres -f ~/xxx.sql > /dev/null real 0m44,887s user 0m11,703s sys 0m6,942s This is probably the most worst case, what is possible and see some slowdown - in this case there is about 10% slowdown - but it is pretty untypical - the one query was less than 50 microsec. When there will be any IO activity or network usage, than this patch doesn't create any significant overhead. I'll mark this patch as ready for commiter Regards Pavel > > > -- > Fabien.
Re: [HACKERS] psql - add special variable to reflect the last query status
Here is a version 6. Small v7 update, sorry for the noise. Add testing the initial state of all variables. Fix typos in a comment in tests. Fix the documentation wrt the current implementation behavior. -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 5bdbc1e..97962d4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3475,6 +3475,16 @@ bar + ERROR + + + Whether the last query failed, as a boolean. + See also SQLSTATE. + + + + + FETCH_COUNT @@ -3611,6 +3621,18 @@ bar + LAST_ERROR_SQLSTATE + LAST_ERROR_MESSAGE + + + The error code and associated error message of the last + error, or "0" and empty strings if no error occured + since the beginning of the script. + + + + + ON_ERROR_ROLLBACK @@ -3679,6 +3701,25 @@ bar + ROW_COUNT + + + How many rows were returned or affected by the last query. + + + + + + SQLSTATE + + + The error code associated to the last query, or + 0 if no error occured. + + + + + QUIET diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index b997058..93950fd 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -493,7 +493,6 @@ ResetCancelConn(void) #endif } - /* * AcceptResult * @@ -1107,6 +1106,35 @@ ProcessResult(PGresult **results) first_cycle = false; } + /* + * Set special variables + * - ERROR: TRUE/FALSE, whether an error occurred + * - SQLSTATE: code of error, or "0" + * - LAST_ERROR_SQLSTATE: same for last error + * - LAST_ERROR_MESSAGE: message of last error + * - ROW_COUNT: how many rows were returned or affected, or "0" + */ + if (success) + { + char *ntuples = PQcmdTuples(*results); + SetVariable(pset.vars, "ERROR", "FALSE"); + SetVariable(pset.vars, "SQLSTATE", "0"); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); + } + else + { + char *code = PQresultErrorField(*results, PG_DIAG_SQLSTATE); + char *mesg = PQresultErrorField(*results, PG_DIAG_MESSAGE_PRIMARY); + + SetVariable(pset.vars, "ERROR", "TRUE"); + /* if an error was detected, it must have a code! */ + Assert(code != NULL); + SetVariable(pset.vars, "SQLSTATE", code); + SetVariable(pset.vars, "LAST_ERROR_SQLSTATE", code); + SetVariable(pset.vars, "LAST_ERROR_MESSAGE", mesg ? mesg : ""); + SetVariable(pset.vars, "ROW_COUNT", "0"); + } + /* may need this to recover from conn loss during COPY */ if (!first_cycle && !CheckConnection()) return false; @@ -1214,7 +1242,6 @@ PrintQueryResults(PGresult *results) return success; } - /* * SendQuery: send the query string to the backend * (and print out results) diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index 4d1c0ec..ae951f5 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -337,7 +337,7 @@ helpVariables(unsigned short int pager) * Windows builds currently print one more line than non-Windows builds. * Using the larger number is fine. */ - output = PageOutput(147, pager ? &(pset.popt.topt) : NULL); + output = PageOutput(155, pager ? &(pset.popt.topt) : NULL); fprintf(output, _("List of specially treated variables\n\n")); @@ -360,6 +360,8 @@ helpVariables(unsigned short int pager) "if set to \"noexec\", just show them without execution\n")); fprintf(output, _(" ENCODING\n" "current client character set encoding\n")); + fprintf(output, _(" ERROR\n" + "whether the last query failed\n")); fprintf(output, _(" FETCH_COUNT\n" "the number of result rows to fetch and display at a time (0 = unlimited)\n")); fprintf(output, _(" HISTCONTROL\n" @@ -374,6 +376,9 @@ helpVariables(unsigned short int pager) "number of EOFs needed to terminate an interactive session\n")); fprintf(output, _(" LASTOID\n" "value of the last affected OID\n")); + fprintf(output, _(" LAST_ERROR_SQLSTATE\n" + " LAST_ERROR_MESSAGE\n" + "error code and message of last error, or \"0\" and empty if none\n")); fprintf(output, _(" ON_ERROR_ROLLBACK\n" "if set, an error doesn't stop a transaction (uses implicit savepoints)\n")); fprintf(output, _(" ON_ERROR_STOP\n" @@ -388,6 +393,8 @@ helpVariables(unsigned short int pager) "specifies the prompt used during COPY ... FROM STDIN\n")); fprintf(output, _(" QUIET\n" "run quietly (same as -q option)\n")); + fprintf(output, _(" ROW_COUNT\n" + "number of rows of last query, or 0\n")); fprintf(output, _(" SERVER_VERSION_NAME\n" " SERVER_VERSION_NUM\n" "server's version (in
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Tom, Here is a version 6. A few thoughts about this patch: * I think the ERROR_CODE variable should instead be named SQLSTATE. That is what the SQL standard calls this string, and it's also what just about all our documentation calls it; see PG_DIAG_SQLSTATE in libpq, or the SQLSTATE 'x' construct in pl/pgsql, or the sqlstate attribute of an exception object in plpython, etc etc. ERROR_CODE -> SQLSTATE. * I'm not exactly convinced that there's a use-case for STATUS Removed, but I think it was nice to have, it is easier to interpret than error codes and their classes that I have not memorized yet:-) * It might be better if SQLSTATE and ERROR_MESSAGE were left unchanged by a non-error query. That would reduce the need to copy them into other variables just because you needed to do something else before printing them. It'd save a few cycles too. Added LAST_ERROR_SQLSTATE & MESSAGE, only reset when an error occured. * Speaking of which, has anyone tried to quantify the performance impact of this patch? It might well be negligible, but I do not think we should assume that without evidence. My guess is negligible. Not sure how to measure this negligible, as many very fast query should be executed to have something significant. Maybe 100,000 "SELECT 1;" in a script? * I wonder why you didn't merge this processing into ProcessResult, instead of inventing an extra function (whose call site seems rather poorly chosen anyhow --- what's the justification for not counting this overhead as part of the query runtime?). You could probably eliminate the refactoring you did, since it wouldn't be necessary to recompute AcceptResult's result that way. Variable setting moved at then end of ProcessResult, no new functions, result is clean, so I should have done it like that in the beginning. Forgotten help stuff added. -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 5bdbc1e..0bbe1c9 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3475,6 +3475,16 @@ bar + ERROR + + + Whether the last query failed, as a boolean. + See also SQLSTATE. + + + + + FETCH_COUNT @@ -3611,6 +3621,18 @@ bar + LAST_ERROR_SQLSTATE + LAST_ERROR_MESSAGE + + + The error code and associated error message of the last + error, or empty strings if no error occured since the + beginning of the script. + + + + + ON_ERROR_ROLLBACK @@ -3679,6 +3701,25 @@ bar + ROW_COUNT + + + How many rows were returned or affected by the last query. + + + + + + SQLSTATE + + + The error code associated to the last query, or + 0 if no error occured. + + + + + QUIET diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index b997058..93950fd 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -493,7 +493,6 @@ ResetCancelConn(void) #endif } - /* * AcceptResult * @@ -1107,6 +1106,35 @@ ProcessResult(PGresult **results) first_cycle = false; } + /* + * Set special variables + * - ERROR: TRUE/FALSE, whether an error occurred + * - SQLSTATE: code of error, or "0" + * - LAST_ERROR_SQLSTATE: same for last error + * - LAST_ERROR_MESSAGE: message of last error + * - ROW_COUNT: how many rows were returned or affected, or "0" + */ + if (success) + { + char *ntuples = PQcmdTuples(*results); + SetVariable(pset.vars, "ERROR", "FALSE"); + SetVariable(pset.vars, "SQLSTATE", "0"); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); + } + else + { + char *code = PQresultErrorField(*results, PG_DIAG_SQLSTATE); + char *mesg = PQresultErrorField(*results, PG_DIAG_MESSAGE_PRIMARY); + + SetVariable(pset.vars, "ERROR", "TRUE"); + /* if an error was detected, it must have a code! */ + Assert(code != NULL); + SetVariable(pset.vars, "SQLSTATE", code); + SetVariable(pset.vars, "LAST_ERROR_SQLSTATE", code); + SetVariable(pset.vars, "LAST_ERROR_MESSAGE", mesg ? mesg : ""); + SetVariable(pset.vars, "ROW_COUNT", "0"); + } + /* may need this to recover from conn loss during COPY */ if (!first_cycle && !CheckConnection()) return false; @@ -1214,7 +1242,6 @@ PrintQueryResults(PGresult *results) return success; } - /* * SendQuery: send the query string to the backend * (and print out results) diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index 4d1c0ec..ae951f5 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -337,7 +337,7 @@ helpVariables(unsigned short int pager) * Windows builds currently print one more line than non-Windows builds. * Using the larger number is fine.
Re: [HACKERS] psql - add special variable to reflect the last query status
* It might be better if SQLSTATE and ERROR_MESSAGE were left unchanged by a non-error query. Hmmm. I'm not sure. If so, ERROR_MESSAGE should be LAST_ERROR_MESSAGE maybe? Then what about LAST_ERROR_SQLSTATE to go with it, and let SQLSTATE & ERROR_MESSAGE reflect the last command, and ERROR is just the boolean to test if it occured? That would reduce the need to copy them into other variables just because you needed to do something else before printing them. It'd save a few cycles too. Well, my suggestion would mean that they would be copied when an error occurs, but only when it occurs, which would not be often. Uh ... what? Sorry if my sentence was not very clear. Time to go do bed:-) I just mean that a LAST_ERROR_* would be set when an error occurs. When there is no error, it is expected to remain the same, and it does not cost anything to let it as is. If an error occured then you had a problem, a transaction aborted, paying to set a few variables when it occurs does not look like a big performance issue. Script usually expect to run without error, errors are rare events. -- 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] psql - add special variable to reflect the last query status
Fabien COELHOwrites: >> * It might be better if SQLSTATE and ERROR_MESSAGE were left >> unchanged by a non-error query. > Hmmm. I'm not sure. If so, ERROR_MESSAGE should be LAST_ERROR_MESSAGE > maybe? Then what about LAST_ERROR_SQLSTATE to go with it, and let SQLSTATE > & ERROR_MESSAGE reflect the last command, and ERROR is just the boolean to > test if it occured? >> That would reduce the need to copy them into other variables just >> because you needed to do something else before printing them. It'd save >> a few cycles too. > Well, my suggestion would mean that they would be copied when an error > occurs, but only when it occurs, which would not be often. Uh ... what? 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] psql - add special variable to reflect the last query status
Hello Tom, * I think the ERROR_CODE variable should instead be named SQLSTATE. That is what the SQL standard calls this string, and it's also what just about all our documentation calls it; see PG_DIAG_SQLSTATE in libpq, or the SQLSTATE 'x' construct in pl/pgsql, or the sqlstate attribute of an exception object in plpython, etc etc. I choose ERROR_CODE because it matched the ERROR boolean. But is may be a misnomer if the status is that all is well. I'm okay with SQLSTATE. * I'm not exactly convinced that there's a use-case for STATUS that's not covered as well or better by ERROR. Client code that looks at PQresStatus for anything beyond error/not-error is usually doing that because it's library code that doesn't know what kind of query it's working on. It seems like a stretch that a psql script would not know that. Also, PQresultStatus memorializes some legacy distinctions, like "fatal" vs "nonfatal" error, that I think we'd be better off not exposing in psql scripting. Ok. * It might be better if SQLSTATE and ERROR_MESSAGE were left unchanged by a non-error query. Hmmm. I'm not sure. If so, ERROR_MESSAGE should be LAST_ERROR_MESSAGE maybe? Then what about LAST_ERROR_SQLSTATE to go with it, and let SQLSTATE & ERROR_MESSAGE reflect the last command, and ERROR is just the boolean to test if it occured? That would reduce the need to copy them into other variables just because you needed to do something else before printing them. It'd save a few cycles too. Well, my suggestion would mean that they would be copied when an error occurs, but only when it occurs, which would not be often. If you would like them, I'm not sure how these variable should be initialized. Undefined? Empty? * Speaking of which, has anyone tried to quantify the performance impact of this patch? It might well be negligible, but I do not think we should assume that without evidence. I think it should be negligible compared to network connections, aborting an ongoing transaction, reading the script... But I do not know libpq internals so I may be quite naive. * I wonder why you didn't merge this processing into ProcessResult, instead of inventing an extra function (whose call site seems rather poorly chosen anyhow --- what's the justification for not counting this overhead as part of the query runtime?). You could probably eliminate the refactoring you did, since it wouldn't be necessary to recompute AcceptResult's result that way. Hmmm. I assume that you are unhappy about ResultIsSuccess. The refactoring is because the function is used twice. I choose to do that because the functionality is clear and could be made as a function which improved readability. Ok, PQresultStatus is thus called twice, I assumed that it is just reading a field in a struct... it could be returned to the caller with an additional pointer to avoid that. The SendResult & ProcessResult functions are already quite heavy to my taste, I did not want to add significantly to that. The ProcessResult switch does not test all states cleanly, it is really about checking about copy, and not so clear, so I do not think that it would mix well to add the variable stuff in the middle of that. SendQuery is also pretty complex, including gotos everywhere. So I did want to add to these two functions beyond the minimum. Now, I can also inline everything coldly in ProcessResult, no problem. -- 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] psql - add special variable to reflect the last query status
A few thoughts about this patch: * I think the ERROR_CODE variable should instead be named SQLSTATE. That is what the SQL standard calls this string, and it's also what just about all our documentation calls it; see PG_DIAG_SQLSTATE in libpq, or the SQLSTATE 'x' construct in pl/pgsql, or the sqlstate attribute of an exception object in plpython, etc etc. * I'm not exactly convinced that there's a use-case for STATUS that's not covered as well or better by ERROR. Client code that looks at PQresStatus for anything beyond error/not-error is usually doing that because it's library code that doesn't know what kind of query it's working on. It seems like a stretch that a psql script would not know that. Also, PQresultStatus memorializes some legacy distinctions, like "fatal" vs "nonfatal" error, that I think we'd be better off not exposing in psql scripting. * It might be better if SQLSTATE and ERROR_MESSAGE were left unchanged by a non-error query. That would reduce the need to copy them into other variables just because you needed to do something else before printing them. It'd save a few cycles too. * Speaking of which, has anyone tried to quantify the performance impact of this patch? It might well be negligible, but I do not think we should assume that without evidence. * I wonder why you didn't merge this processing into ProcessResult, instead of inventing an extra function (whose call site seems rather poorly chosen anyhow --- what's the justification for not counting this overhead as part of the query runtime?). You could probably eliminate the refactoring you did, since it wouldn't be necessary to recompute AcceptResult's result that way. 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] psql --batch
I don't doubt about a sense of this configuration - but this specific combination depends on usage - so I don't think so using special option is good idea. Although I agree with you that detailed settings are definitely debatable, I'd say that at least it would be a more reasonable starting point for scripting than the default configuration which is more interactive usage oriented. So even if unperfect, one is free to update defaults to suit more closely their needs, eg "psql -B -F ':' ...", at least most of the scripting conviniencies are already there. I think that such a scripting mode should also imply --no-readline. -- 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] psql --batch
2017-08-28 11:05 GMT+02:00 Craig Ringer: > On 28 August 2017 at 16:23, Fabien COELHO wrote: > >> >> This doesn't really address the original issue though, that it's far from >>> obvious how to easily and correctly script psql. >>> >> >> That is another interesting argument. I understood that the issue was >> having to type these options, but now it is also to remember which one are >> relevant and wanted, which is a little different and more justifiable as an >> option. >> >> On that account, ISTM that '|' as a field separator is debatable, that >> pager should be turned off... and maybe a few other things. >> > > > Good point re pager, though it's turned off automatically when stdout > isn't a terminal, so in practice that'll only matter if you're using > something like 'expect' that uses a pty. Still worth doing. > > I agree that | is a bit iffy, but so's anything really. null bytes aren't > usable for all scripts, and nothing else cannot also be output in the data > its self. No easy answers there. In cases where I expect that to be an > issue I sometimes use \COPY ... TO STDOUT WITH (FORMAT CSV) though. > I don't doubt about a sense of this configuration - but this specific combination depends on usage - so I don't think so using special option is good idea. Regards Pavel > > -- > Craig Ringer http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services >
Re: [HACKERS] psql --batch
On 28 August 2017 at 16:23, Fabien COELHOwrote: > > This doesn't really address the original issue though, that it's far from >> obvious how to easily and correctly script psql. >> > > That is another interesting argument. I understood that the issue was > having to type these options, but now it is also to remember which one are > relevant and wanted, which is a little different and more justifiable as an > option. > > On that account, ISTM that '|' as a field separator is debatable, that > pager should be turned off... and maybe a few other things. > Good point re pager, though it's turned off automatically when stdout isn't a terminal, so in practice that'll only matter if you're using something like 'expect' that uses a pty. Still worth doing. I agree that | is a bit iffy, but so's anything really. null bytes aren't usable for all scripts, and nothing else cannot also be output in the data its self. No easy answers there. In cases where I expect that to be an issue I sometimes use \COPY ... TO STDOUT WITH (FORMAT CSV) though. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] psql --batch
This doesn't really address the original issue though, that it's far from obvious how to easily and correctly script psql. That is another interesting argument. I understood that the issue was having to type these options, but now it is also to remember which one are relevant and wanted, which is a little different and more justifiable as an option. On that account, ISTM that '|' as a field separator is debatable, that pager should be turned off... and maybe a few other things. -- 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] psql --batch
On 28 August 2017 at 15:34, Pavel Stehulewrote: > > > 2017-08-28 9:33 GMT+02:00 Fabien COELHO : > >> >> ISTM that the real pain is the "-v ON_ERRORS_STOP=1" which I occasionally encountered as well, the other one letter ones are not too bad. Maybe it would be enough to have a shortcut for this one, say "-B"? >>> >>> I agree with last sentence. I don't think so -qAtX are expected always, >>> but >>> "-v ON_ERRORS_STOP=1" is pretty often. >>> >> >> Yep. I often "\set" that in the script. >> >> What do you think about long option "--on-errors-stop" ? >>> >> >> It does not really relieve the typing pain Craig is complaining about, >> but it would be ok as the long option version if -B is added, and it is >> auto-completion friendly. > > > This doesn't really address the original issue though, that it's far from obvious how to easily and correctly script psql. I guess there's always the option of a docs patch for that. *shrug* I'll see what others have to say. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] psql --batch
2017-08-28 9:33 GMT+02:00 Fabien COELHO: > > ISTM that the real pain is the "-v ON_ERRORS_STOP=1" which I occasionally >>> encountered as well, the other one letter ones are not too bad. Maybe it >>> would be enough to have a shortcut for this one, say "-B"? >>> >> >> I agree with last sentence. I don't think so -qAtX are expected always, >> but >> "-v ON_ERRORS_STOP=1" is pretty often. >> > > Yep. I often "\set" that in the script. > > What do you think about long option "--on-errors-stop" ? >> > > It does not really relieve the typing pain Craig is complaining about, but > it would be ok as the long option version if -B is added, and it is > auto-completion friendly. ok Pavel > > > -- > Fabien. >
Re: [HACKERS] psql --batch
ISTM that the real pain is the "-v ON_ERRORS_STOP=1" which I occasionally encountered as well, the other one letter ones are not too bad. Maybe it would be enough to have a shortcut for this one, say "-B"? I agree with last sentence. I don't think so -qAtX are expected always, but "-v ON_ERRORS_STOP=1" is pretty often. Yep. I often "\set" that in the script. What do you think about long option "--on-errors-stop" ? It does not really relieve the typing pain Craig is complaining about, but it would be ok as the long option version if -B is added, and it is auto-completion friendly. -- 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] psql --batch
2017-08-28 8:56 GMT+02:00 Fabien COELHO: > > I find myself regurgitating the incantation >> >> psql -qAtX -v ON_ERRORS_STOP=1 >> >> quite a bit. It's not ... super friendly. >> >> It strikes me that we could possibly benefit from a 'psql --batch' option. >> >> Thoughts? >> > > The link between -qAtX and "batch" is not that fully obvious, especially > the unaligned tuples-only part. If so, why not some -F as well? > > ISTM that the real pain is the "-v ON_ERRORS_STOP=1" which I occasionally > encountered as well, the other one letter ones are not too bad. Maybe it > would be enough to have a shortcut for this one, say "-B"? > I agree with last sentence. I don't think so -qAtX are expected always, but "-v ON_ERROS_STOP=1" is pretty often. What do you think about long option "---on-errors-stop" ? Regards Pavel > alias? > > -- > 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] psql --batch
On 28 August 2017 at 14:56, Fabien COELHOwrote: > > I find myself regurgitating the incantation >> >> psql -qAtX -v ON_ERRORS_STOP=1 >> >> quite a bit. It's not ... super friendly. >> >> It strikes me that we could possibly benefit from a 'psql --batch' option. >> >> Thoughts? >> > > The link between -qAtX and "batch" is not that fully obvious, especially > the unaligned tuples-only part. If so, why not some -F as well? > > q: quiet Pretty much always wanted for a batch mode run of anything. A: unaligned tuples Because alignment is pretty much never useful when you're parsing result sets with scripting (splitting, cut, etc) and just makes everything harder. The alignment psql uses isn't fixed, after all. t: tuples-only Headers just make everything more annoying to parse, and force you to do extra work to skip them. If you're running batch code you know the headers because you used a column-list form SELECT, or should've. You're unlikely to be ingesting them and using them to split up the tuple anyway. I think this one is a bit more arguable than the first two, though, as I can at least think of some cases where you might want it. X: skip .psqlrc Reliable, portable scripted psql shouldn't be using the local .psqlrc IMO. It's likely to just break things in exciting ways. But I can see it being reasonable to require this option to be supplied separately and just document it as "recommended" with --batch. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] psql --batch
I find myself regurgitating the incantation psql -qAtX -v ON_ERRORS_STOP=1 quite a bit. It's not ... super friendly. It strikes me that we could possibly benefit from a 'psql --batch' option. Thoughts? The link between -qAtX and "batch" is not that fully obvious, especially the unaligned tuples-only part. If so, why not some -F as well? ISTM that the real pain is the "-v ON_ERRORS_STOP=1" which I occasionally encountered as well, the other one letter ones are not too bad. Maybe it would be enough to have a shortcut for this one, say "-B"? alias? -- 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] psql - add ability to test whether a variable exists
Here is a version with the :{?varname} syntax. It looks much better for me. I'll admit that it looks better to me as well:-) -- 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] psql - add ability to test whether a variable exists
On Sat, Aug 26, 2017 at 2:09 PM, Pavel Stehulewrote: > > > 2017-08-26 19:55 GMT+02:00 Fabien COELHO : > >> >> Any colon prefixed syntax can be made to work because it is enough for >>> the lexer to detect and handle... so >>> >>> :{defined varname} >>> >>> may be an option, although I do not like the space much because it adds >>> some fuzzyness in the lexer which has to process it. It is probably doable, >>> though. I like having a "?" because there is a question. Other >>> suggestions somehow in line with your proposal could be >>> :{?varname} >>> :{varname?} >>> what do you think? >>> >> >> Here is a version with the :{?varname} syntax. > > > It looks much better for me. > > Regards > > Pavel > +1. Glad to have this feature. Any of the proposed syntaxes look good to me, with a slight preference for {?var}.
Re: [HACKERS] psql - add ability to test whether a variable exists
2017-08-26 19:55 GMT+02:00 Fabien COELHO: > > Any colon prefixed syntax can be made to work because it is enough for the >> lexer to detect and handle... so >> >> :{defined varname} >> >> may be an option, although I do not like the space much because it adds >> some fuzzyness in the lexer which has to process it. It is probably doable, >> though. I like having a "?" because there is a question. Other >> suggestions somehow in line with your proposal could be >> :{?varname} >> :{varname?} >> what do you think? >> > > Here is a version with the :{?varname} syntax. It looks much better for me. Regards Pavel > > > -- > Fabien.
Re: [HACKERS] psql - add ability to test whether a variable exists
Any colon prefixed syntax can be made to work because it is enough for the lexer to detect and handle... so :{defined varname} may be an option, although I do not like the space much because it adds some fuzzyness in the lexer which has to process it. It is probably doable, though. I like having a "?" because there is a question. Other suggestions somehow in line with your proposal could be :{?varname} :{varname?} what do you think? Here is a version with the :{?varname} syntax. -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index c592eda..58f8e9a 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -781,6 +781,10 @@ testdb= The forms :'variable_name' and :"variable_name" described there work as well. +The :{?variable_name} syntax allows +to test whether a variable is defined. It is substituted by +TRUE or FALSE. +Escaping the colon with a backslash protects it from substitution. @@ -3813,6 +3817,12 @@ testdb= INSERT INTO my_table VALUES (:'content'); +The :{?name} special syntax returns TRUE +or FALSE depending on whether the variable exists or not, thus is +always substituted, unless the colon is backslash-escaped. + + + The colon syntax for variables is standard SQL for embedded query languages, such as ECPG. The colon syntaxes for array slices and type casts are diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l index db7a1b9..9a53cb3 100644 --- a/src/bin/psql/psqlscanslash.l +++ b/src/bin/psql/psqlscanslash.l @@ -281,6 +281,10 @@ other . unquoted_option_chars = 0; } +:\{\?{variable_char}+\} { + psqlscan_test_variable(cur_state, yytext, yyleng); +} + :'{variable_char}* { /* Throw back everything but the colon */ yyless(1); @@ -295,6 +299,20 @@ other . ECHO; } +:\{\?{variable_char}* { + /* Throw back everything but the colon */ + yyless(1); + unquoted_option_chars++; + ECHO; +} + +:\{ { + /* Throw back everything but the colon */ + yyless(1); + unquoted_option_chars++; + ECHO; +} + {other} { unquoted_option_chars++; ECHO; diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l index 27689d7..4375142a 100644 --- a/src/fe_utils/psqlscan.l +++ b/src/fe_utils/psqlscan.l @@ -745,9 +745,13 @@ other . PQUOTE_SQL_IDENT); } +:\{\?{variable_char}+\} { + psqlscan_test_variable(cur_state, yytext, yyleng); +} + /* * These rules just avoid the need for scanner backup if one of the - * two rules above fails to match completely. + * three rules above fails to match completely. */ :'{variable_char}* { @@ -762,6 +766,17 @@ other . ECHO; } +:\{\?{variable_char}* { + /* Throw back everything but the colon */ + yyless(1); + ECHO; +} +:\{ { + /* Throw back everything but the colon */ + yyless(1); + ECHO; +} + /* * Back to backend-compatible rules. */ @@ -1442,3 +1457,28 @@ psqlscan_escape_variable(PsqlScanState state, const char *txt, int len, psqlscan_emit(state, txt, len); } } + +void +psqlscan_test_variable(PsqlScanState state, const char *txt, int len) +{ + char *varname; + char *value; + + varname = psqlscan_extract_substring(state, txt + 3, len - 4); + if (state->callbacks->get_variable) + value = state->callbacks->get_variable(varname, PQUOTE_PLAIN, + state->cb_passthrough); + else + value = NULL; + free(varname); + + if (value != NULL) + { + psqlscan_emit(state, "TRUE", 4); + free(value); + } + else + { + psqlscan_emit(state, "FALSE", 5); + } +} diff --git a/src/include/fe_utils/psqlscan_int.h b/src/include/fe_utils/psqlscan_int.h index c70ff29..e9b3517 100644 --- a/src/include/fe_utils/psqlscan_int.h +++ b/src/include/fe_utils/psqlscan_int.h @@ -142,5 +142,7 @@ extern char *psqlscan_extract_substring(PsqlScanState state, extern void psqlscan_escape_variable(PsqlScanState state, const char *txt, int len, PsqlScanQuoteType quote); +extern void psqlscan_test_variable(PsqlScanState state, + const char *txt, int len); #endif /* PSQLSCAN_INT_H */ diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out index 4aaf4c1..2b2f23b 100644 --- a/src/test/regress/expected/psql.out +++ b/src/test/regress/expected/psql.out @@ -2929,6 +2929,32 @@ bar 'bar' "bar" \echo 'should print #8-1' should print #8-1 \endif +-- :{?...} defined variable test +\set i 1 +\if :{?i} + \echo '#9-1 ok, variable i is defined' +#9-1 ok, variable i is defined +\else + \echo 'should not print #9-2' +\endif +\if :{?no_such_variable} + \echo 'should not print #10-1' +\else + \echo '#10-2 ok, variable no_such_variable is not defined' +#10-2 ok, variable no_such_variable is not defined +\endif +SELECT :{?i} AS i_is_defined; + i_is_defined +-- + t
Re: [HACKERS] psql - add ability to test whether a variable exists
Hello Pavel, As a follow-up to the \if patch by Corey Huinker, here is a proposal to allow testing whether a client-side variable exists in psql. The syntax is as ugly as the current :'var' and :"var" things, but ISTM that this is the only simple option to have a working SQL-compatible syntax with both client-side substitution and server side execution. See the second example below. It is really ugly Yes, I agree:-) - the ? symbol is not used in pair usually - so it is much more visible - it is bad readable. Yep. Maybe some other syntax: :{fx xxx} .. where fx can be one from more possible operators ? ! ... Any colon prefixed syntax can be made to work because it is enough for the lexer to detect and handle... so :{defined varname} may be an option, although I do not like the space much because it adds some fuzzyness in the lexer which has to process it. It is probably doable, though. I like having a "?" because there is a question. Other suggestions somehow in line with your proposal could be :{?varname} :{varname?} what do you think? The other option would be to have some special keyword syntax, say "defined var", but then it would have to be parsed client side, and how to do that in an SQL expression is unclear, and moreover it would not look right in an SQL expression. If it would look like a function call, say "defined('var')", it would potentially interact with existing server-side user-defined functions, which is pretty undesirable. Hence the :?...? proposal above which is limited to variable subsitution syntax. should not be solved by introduction \ifdef ? This would be a client side only non extendable option: you can only test one variable at a time, you cannot say "NOT" or have to do \ifndef... CPP started like that and ended with #if bool-expr-with-defined in the end. The idea is to extend the newly added \if with client-side SQL-compatible expression syntax, and that the same syntax could be used server side with select, eg: -- client side \let i :j + 1 -- server side SELECT :j + 1 AS i \gset -- client-side conditional \if NOT :j > 1 OR ... The colon prefixed substitution syntax already works for server side, but there is no way to check whether a variable exists which is compatible with that, hence this patch. Pgbench has expressions with patches to improve it, especially adding boolean operators. I think that the simplest plan is, when stabalized, to move the parser & executir to fe_utils and to use it from both psql et pgbench. Also I plan to add \if to pgbench, possibly by factoring some of the code from psql in fe_utils as well because it would help with benchmarks. Now given the patch queue length and speed I'm not even thinking of starting doing all this. -- 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] psql - add ability to test whether a variable exists
2017-08-26 8:54 GMT+02:00 Fabien COELHO: > > Hello, > > As a follow-up to the \if patch by Corey Huinker, here is a proposal to > allow testing whether a client-side variable exists in psql. > > The syntax is as ugly as the current :'var' and :"var" things, but ISTM > that this is the only simple option to have a working SQL-compatible syntax > with both client-side substitution and server side execution. See the > second example below. > It is really ugly - the ? symbol is not used in pair usually - so it is much more visible - it is bad readable. Maybe some other syntax: :{fx xxx} .. where fx can be one from more possible operators ? ! ... > > -- client side use > psql> \set i 1 > psql> \if :?i? > psql> \echo 'i is defined' > psql> \endif > -- use server-side in an SQL expression > psql> SELECT NOT :?VERSION_NUM? OR :'VERSION' <> VERSTION() AS bad_conf > \gset > > psql> \if :bad_conf \echo 'too bad...' \quit \endif > > The other option would be to have some special keyword syntax, say > "defined var", but then it would have to be parsed client side, and how to > do that in an SQL expression is unclear, and moreover it would not look > right in an SQL expression. If it would look like a function call, say > "defined('var')", it would potentially interact with existing server-side > user-defined functions, which is pretty undesirable. Hence the :?...? > proposal above which is limited to variable subsitution syntax. should not be solved by introduction \ifdef ? > > -- > 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] psql - add special variable to reflect the last query status
2017-06-28 10:04 GMT+02:00 Fabien COELHO: > > Hello Pavel, > > + if (success) >> + { >> + char *ntuples = PQcmdTuples(results); >> + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); >> + SetVariable(pset.vars, "ERROR", "FALSE"); >> + } >> + else >> + { >> + SetVariable(pset.vars, "ROW_COUNT", "0"); >> + SetVariable(pset.vars, "ERROR", "TRUE"); >> + } >> +} >> >> It can be simplified >> >> SetVariable(pset.vars, "ROW_COUNT", success ? PQcmdTuples(results) : 0); >> > > According to the documentation, PQcmdTuples returns "" in some cases and > ISTM we want "0" instead for consistency, so that it is always a number. I > rejected calling PQcmdTuples twice: > > ..., success && *PQcmdTuples(results) ? PQcmdTuples(results) : "0") > > Thus it makes the "if (success)" necessary for ROW_COUNT, and then it > looked simpler to handle ERROR the same way. > > Now if the semantics is changed to put as row count whatever comes out of > the function, even if not a count, then the code could indeed be simplified > as you suggest. Understand Pavel > > > -- > Fabien. >
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Pavel, + if (success) + { + char *ntuples = PQcmdTuples(results); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); + SetVariable(pset.vars, "ERROR", "FALSE"); + } + else + { + SetVariable(pset.vars, "ROW_COUNT", "0"); + SetVariable(pset.vars, "ERROR", "TRUE"); + } +} It can be simplified SetVariable(pset.vars, "ROW_COUNT", success ? PQcmdTuples(results) : 0); According to the documentation, PQcmdTuples returns "" in some cases and ISTM we want "0" instead for consistency, so that it is always a number. I rejected calling PQcmdTuples twice: ..., success && *PQcmdTuples(results) ? PQcmdTuples(results) : "0") Thus it makes the "if (success)" necessary for ROW_COUNT, and then it looked simpler to handle ERROR the same way. Now if the semantics is changed to put as row count whatever comes out of the function, even if not a count, then the code could indeed be simplified as you suggest. -- 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] psql - add special variable to reflect the last query status
2017-06-28 9:25 GMT+02:00 Fabien COELHO: > > Hello Pavel, > > I agree that the existing "SetVariableBool" function is a misnommer, it >>> should be "SetVariableOn" given what it does, and it is not what we need. >>> >> >> switching default setting from ON to TRUE requires wider discussion - >> > > Yep. > > in this moment I like to have special function "SetVariableON". >> > > I'm fine with this, but this make it a change totally unrelated to this > patch as it would not use the function... Moreover, this function would not > use an hypothetical "set var bool" function because of the debatable on/off > vs true/false change. > > Also, a "set var bool" function would be called only twice, which is not > very beneficial for a oneliner, so I left it out. > > I agree that there is some common structure, but ISTM that the >>> AcceptResult function is called in a variety of situation where variables >>> are not to be set (eg "internal" queries, not user provided queries), so >>> I >>> thought it best to keep the two apart. >>> >> >> I understand, but It is not nice, really - maybe only switch can be moved >> to some inlining function like IsSuccess() - more .. with this function, >> the SetResultVariables function will be more cleaner >> > > Indeed. Attached v5 does that. juju - something like this + if (success) + { + char *ntuples = PQcmdTuples(results); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); + SetVariable(pset.vars, "ERROR", "FALSE"); + } + else + { + SetVariable(pset.vars, "ROW_COUNT", "0"); + SetVariable(pset.vars, "ERROR", "TRUE"); + } +} It can be simplified SetVariable(pset.vars, "ROW_COUNT", success ? PQcmdTuples(results) : 0); SetVariable(pset.vars, "ERROR", success ? "FALSE" : "TRUE"); Regards Pavel > > > -- > Fabien.
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Pavel, I agree that the existing "SetVariableBool" function is a misnommer, it should be "SetVariableOn" given what it does, and it is not what we need. switching default setting from ON to TRUE requires wider discussion - Yep. in this moment I like to have special function "SetVariableON". I'm fine with this, but this make it a change totally unrelated to this patch as it would not use the function... Moreover, this function would not use an hypothetical "set var bool" function because of the debatable on/off vs true/false change. Also, a "set var bool" function would be called only twice, which is not very beneficial for a oneliner, so I left it out. I agree that there is some common structure, but ISTM that the AcceptResult function is called in a variety of situation where variables are not to be set (eg "internal" queries, not user provided queries), so I thought it best to keep the two apart. I understand, but It is not nice, really - maybe only switch can be moved to some inlining function like IsSuccess() - more .. with this function, the SetResultVariables function will be more cleaner Indeed. Attached v5 does that. -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 9faa365..bc9a2e4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3452,6 +3452,35 @@ bar + ERROR + + + Whether the last query failed, as a boolean. + + + + + + ERROR_CODE + + + The error code associated to the last query, or + 0 if no error occured. + + + + + + ERROR_MESSAGE + + + If the last query failed, the associated error message, + otherwise an empty string. + + + + + FETCH_COUNT @@ -3656,6 +3685,24 @@ bar + STATUS + + + Text status of the last query. + + + + + + ROW_COUNT + + + How many rows were returned or affected by the last query. + + + + + QUIET diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 044cdb8..02fa89a 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -492,6 +492,48 @@ ResetCancelConn(void) #endif } +/* + * ResultIsSuccess + * + * Tell whether query result is a success. + */ +static bool +ResultIsSuccess(const PGresult *result) +{ + bool success; + if (!result) + success = false; + else + { + ExecStatusType restat = PQresultStatus(result); + + /* check all possible PGRES_ */ + switch (restat) + { + case PGRES_EMPTY_QUERY: + case PGRES_TUPLES_OK: + case PGRES_COMMAND_OK: + case PGRES_COPY_OUT: + case PGRES_COPY_IN: + case PGRES_COPY_BOTH: + case PGRES_SINGLE_TUPLE: +success = true; +break; + case PGRES_BAD_RESPONSE: + case PGRES_NONFATAL_ERROR: + case PGRES_FATAL_ERROR: +success = false; +break; + default: +/* dead code */ +success = false; +psql_error("unexpected PQresultStatus: %d\n", restat); +break; + } + } + + return success; +} /* * AcceptResult @@ -504,34 +546,7 @@ ResetCancelConn(void) static bool AcceptResult(const PGresult *result) { - bool OK; - - if (!result) - OK = false; - else - switch (PQresultStatus(result)) - { - case PGRES_COMMAND_OK: - case PGRES_TUPLES_OK: - case PGRES_EMPTY_QUERY: - case PGRES_COPY_IN: - case PGRES_COPY_OUT: -/* Fine, do nothing */ -OK = true; -break; - - case PGRES_BAD_RESPONSE: - case PGRES_NONFATAL_ERROR: - case PGRES_FATAL_ERROR: -OK = false; -break; - - default: -OK = false; -psql_error("unexpected PQresultStatus: %d\n", - PQresultStatus(result)); -break; - } + bool OK = ResultIsSuccess(result); if (!OK) { @@ -1213,6 +1228,38 @@ PrintQueryResults(PGresult *results) return success; } +/* + * Set special variables for "front door" queries + * - STATUS: last query status + * - ERROR: TRUE/FALSE, whether an error occurred + * - ERROR_CODE: code if an error occured, or "0" + * - ERROR_MESSAGE: message if an error occured, or "" + * - ROW_COUNT: how many rows were returned or affected, or "0" + */ +static void +SetResultVariables(PGresult *results) +{ + bool success = ResultIsSuccess(results); + ExecStatusType restat = PQresultStatus(results); + char *code = PQresultErrorField(results, PG_DIAG_SQLSTATE); + char *mesg = PQresultErrorField(results, PG_DIAG_MESSAGE_PRIMARY); + + SetVariable(pset.vars, "STATUS", PQresStatus(restat) + strlen("PGRES_")); + SetVariable(pset.vars, "ERROR_CODE", code ? code : "0"); + SetVariable(pset.vars, "ERROR_MESSAGE", mesg ? mesg : ""); + + if (success) + { + char *ntuples = PQcmdTuples(results); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples
Re: [HACKERS] psql - add special variable to reflect the last query status
2017-06-27 17:30 GMT+02:00 Fabien COELHO: > > Hello Pavel, > > We can introduce macro SetVariableBool(vars, varname, bool) instead >>> >>> SetVariable(pset.vars, "ERROR", "FALSE"); >>> >> >> I checked source code, and it requires little bit more harder refactoring >> because now we have SetVariableBool - what is unhappy name, because it >> initialize variable to ON value. It is question what is better name? >> > > The boolean values (on/off 1/0 true/false...) accepted for pg settings is > probably convenient but also somehow fuzzy. > > From a programming point of view, I like booleans to have either true or > false values, and nothing else. > > I agree that the existing "SetVariableBool" function is a misnommer, it > should be "SetVariableOn" given what it does, and it is not what we need. > switching default setting from ON to TRUE requires wider discussion - in this moment I like to have special function "SetVariableON". > > Here is a v4 which attempts to extend & reuse the function. People might > be surprised that TRUE is used where ON was used before, so I'm not sure. > > I found more interesting issue - the code of SetResultVariables is >> partially redundant with AcceptResult - maybe the switch there can be >> shared. >> > > I agree that there is some common structure, but ISTM that the > AcceptResult function is called in a variety of situation where variables > are not to be set (eg "internal" queries, not user provided queries), so I > thought it best to keep the two apart. I understand, but It is not nice, really - maybe only switch can be moved to some inlining function like IsSuccess() - more .. with this function, the SetResultVariables function will be more cleaner Regards Pavel > > > -- > Fabien.
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Pavel, We can introduce macro SetVariableBool(vars, varname, bool) instead SetVariable(pset.vars, "ERROR", "FALSE"); I checked source code, and it requires little bit more harder refactoring because now we have SetVariableBool - what is unhappy name, because it initialize variable to ON value. It is question what is better name? The boolean values (on/off 1/0 true/false...) accepted for pg settings is probably convenient but also somehow fuzzy. From a programming point of view, I like booleans to have either true or false values, and nothing else. I agree that the existing "SetVariableBool" function is a misnommer, it should be "SetVariableOn" given what it does, and it is not what we need. Here is a v4 which attempts to extend & reuse the function. People might be surprised that TRUE is used where ON was used before, so I'm not sure. I found more interesting issue - the code of SetResultVariables is partially redundant with AcceptResult - maybe the switch there can be shared. I agree that there is some common structure, but ISTM that the AcceptResult function is called in a variety of situation where variables are not to be set (eg "internal" queries, not user provided queries), so I thought it best to keep the two apart. -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 9faa365..bc9a2e4 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3452,6 +3452,35 @@ bar + ERROR + + + Whether the last query failed, as a boolean. + + + + + + ERROR_CODE + + + The error code associated to the last query, or + 0 if no error occured. + + + + + + ERROR_MESSAGE + + + If the last query failed, the associated error message, + otherwise an empty string. + + + + + FETCH_COUNT @@ -3656,6 +3685,24 @@ bar + STATUS + + + Text status of the last query. + + + + + + ROW_COUNT + + + How many rows were returned or affected by the last query. + + + + + QUIET diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 044cdb8..3abb3e7 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -1213,6 +1213,62 @@ PrintQueryResults(PGresult *results) return success; } +/* + * Set special variables for "front door" queries + * - STATUS: last query status + * - ERROR: TRUE/FALSE, whether an error occurred + * - ERROR_CODE: code if an error occured, or "0" + * - ERROR_MESSAGE: message if an error occured, or "" + * - ROW_COUNT: how many rows were returned or affected, or "0" + */ +static void +SetResultVariables(PGresult *results) +{ + bool success; + ExecStatusType restat = PQresultStatus(results); + char *code = PQresultErrorField(results, PG_DIAG_SQLSTATE); + char *mesg = PQresultErrorField(results, PG_DIAG_MESSAGE_PRIMARY); + + SetVariable(pset.vars, "STATUS", PQresStatus(restat) + strlen("PGRES_")); + SetVariable(pset.vars, "ERROR_CODE", code ? code : "0"); + SetVariable(pset.vars, "ERROR_MESSAGE", mesg ? mesg : ""); + + /* check all possible PGRES_ */ + switch (restat) + { + case PGRES_EMPTY_QUERY: + case PGRES_TUPLES_OK: + case PGRES_COMMAND_OK: + case PGRES_COPY_OUT: + case PGRES_COPY_IN: + case PGRES_COPY_BOTH: + case PGRES_SINGLE_TUPLE: + success = true; + break; + case PGRES_BAD_RESPONSE: + case PGRES_NONFATAL_ERROR: + case PGRES_FATAL_ERROR: + success = false; + break; + default: + /* dead code */ + success = false; + psql_error("unexpected PQresultStatus: %d\n", restat); + break; + } + + if (success) + { + char *ntuples = PQcmdTuples(results); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); + } + else + { + SetVariable(pset.vars, "ROW_COUNT", "0"); + } + + SetVariableBool(pset.vars, "ERROR", !success); +} /* * SendQuery: send the query string to the backend @@ -1346,6 +1402,9 @@ SendQuery(const char *query) elapsed_msec = INSTR_TIME_GET_MILLISEC(after); } + /* set special variables to reflect the result status */ + SetResultVariables(results); + /* but printing results isn't: */ if (OK && results) OK = PrintQueryResults(results); diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c index 7f76797..4ee2855 100644 --- a/src/bin/psql/startup.c +++ b/src/bin/psql/startup.c @@ -163,7 +163,7 @@ main(int argc, char *argv[]) SetVariable(pset.vars, "VERSION", PG_VERSION_STR); /* Default values for variables (that don't match the result of \unset) */ - SetVariableBool(pset.vars, "AUTOCOMMIT"); + SetVariableBool(pset.vars, "AUTOCOMMIT", true); SetVariable(pset.vars, "PROMPT1", DEFAULT_PROMPT1);
Re: [HACKERS] psql - add special variable to reflect the last query status
Hi 2017-06-19 5:55 GMT+02:00 Pavel Stehule: > > > 2017-06-17 7:58 GMT+02:00 Fabien COELHO : > >> >> I have not any other comments. The implementation is trivial. I rerun all >>> tests and tests passed. >>> >>> I'll mark this patch as ready for commiters. >>> >> >> Oops, I just noticed a stupid confusion on my part which got through, I >> was setting "ERROR" as "success", inverting the expected boolean value. >> >> Here is a fixed version. > > > I missed it too. > > We can introduce macro SetVariableBool(vars, varname, bool) instead > > SetVariable(pset.vars, "ERROR", "FALSE"); > I checked source code, and it requires little bit more harder refactoring because now we have SetVariableBool - what is unhappy name, because it initialize variable to ON value. It is question what is better name? I found more interesting issue - the code of SetResultVariables is partially redundant with AcceptResult - maybe the switch there can be shared. Regards Pavel > > Regards > > Pavel > >> >> >> -- >> Fabien. > > >
Re: [HACKERS] psql: Activate pager only for height, not width
On Tue, 30 May 2017 at 05:30 Christoph Bergwrote: > Oh interesting, I didn't know about pager_min_lines. That sounds > useful as well. +1 on the analogous pager_min_cols option. > On closer inspection, I note that psql already has a 'columns' \pset option, which does control the width for triggering the pager, but also controls the width for wrapping and auto-expanded mode purposes. So, I can set 'columns' to get the pager behaviour that I want, but I also get unwanted effects where I'd rather let the default (terminal width) apply. So if we were to add a 'pager_min_cols', we'd have to decide how it interacts with 'columns'. For example, to determine whether to trigger the pager, we look for 'pager_min_cols' first, and if that is not set, then fall back to 'columns'. For all other width purposes, 'columns' would continue to apply as present. However, my feeling is that this is becoming a bit too fiddly. If 'columns' did not already exist then 'pager_min_cols' would make more sense, but as it does already exist, my preference is to leave 'columns' as-is, and go for a height-only option to 'pager' instead. Thoughts? Cheers, BJ
Re: [HACKERS] [psql] patch to fix ordering in words_after_create array
On Fri, Mar 24, 2017 at 7:10 AM, Rushabh Lathiawrote: > While looking at the code around tab-complete.c, I > found the ordering in words_after_create array is not > correct for DEFAULT PRIVILEGES, which been added > under below commit: > > commit d7d77f3825122bde55be9e06f6c4851028b99795 > Author: Peter Eisentraut > Date: Thu Mar 16 18:54:28 2017 -0400 > > psql: Add completion for \help DROP|ALTER > > PFA patch to fix the same. Committed. -- 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] psql - add special variable to reflect the last query status
2017-06-17 7:58 GMT+02:00 Fabien COELHO: > > I have not any other comments. The implementation is trivial. I rerun all >> tests and tests passed. >> >> I'll mark this patch as ready for commiters. >> > > Oops, I just noticed a stupid confusion on my part which got through, I > was setting "ERROR" as "success", inverting the expected boolean value. > > Here is a fixed version. I missed it too. We can introduce macro SetVariableBool(vars, varname, bool) instead SetVariable(pset.vars, "ERROR", "FALSE"); Regards Pavel > > > -- > Fabien.
Re: [HACKERS] psql - add special variable to reflect the last query status
I have not any other comments. The implementation is trivial. I rerun all tests and tests passed. I'll mark this patch as ready for commiters. Oops, I just noticed a stupid confusion on my part which got through, I was setting "ERROR" as "success", inverting the expected boolean value. Here is a fixed version. -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index e6eba21..18c3e7e 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3450,6 +3450,35 @@ bar + ERROR + + + Whether the last query failed, as a boolean. + + + + + + ERROR_CODE + + + The error code associated to the last query, or + 0 if no error occured. + + + + + + ERROR_MESSAGE + + + If the last query failed, the associated error message, + otherwise an empty string. + + + + + FETCH_COUNT @@ -3654,6 +3683,24 @@ bar + STATUS + + + Text status of the last query. + + + + + + ROW_COUNT + + + How many rows were returned or affected by the last query. + + + + + QUIET diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index a2f1259..e717159 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -1213,6 +1213,62 @@ PrintQueryResults(PGresult *results) return success; } +/* + * Set special variables for "front door" queries + * - STATUS: last query status + * - ERROR: TRUE/FALSE, whether an error occurred + * - ERROR_CODE: code if an error occured, or "0" + * - ERROR_MESSAGE: message if an error occured, or "" + * - ROW_COUNT: how many rows were returned or affected, or "0" + */ +static void +SetResultVariables(PGresult *results) +{ + bool success; + ExecStatusType restat = PQresultStatus(results); + char *code = PQresultErrorField(results, PG_DIAG_SQLSTATE); + char *mesg = PQresultErrorField(results, PG_DIAG_MESSAGE_PRIMARY); + + SetVariable(pset.vars, "STATUS", PQresStatus(restat) + strlen("PGRES_")); + SetVariable(pset.vars, "ERROR_CODE", code ? code : "0"); + SetVariable(pset.vars, "ERROR_MESSAGE", mesg ? mesg : ""); + + /* check all possible PGRES_ */ + switch (restat) + { + case PGRES_EMPTY_QUERY: + case PGRES_TUPLES_OK: + case PGRES_COMMAND_OK: + case PGRES_COPY_OUT: + case PGRES_COPY_IN: + case PGRES_COPY_BOTH: + case PGRES_SINGLE_TUPLE: + success = true; + break; + case PGRES_BAD_RESPONSE: + case PGRES_NONFATAL_ERROR: + case PGRES_FATAL_ERROR: + success = false; + break; + default: + /* dead code */ + success = false; + psql_error("unexpected PQresultStatus: %d\n", restat); + break; + } + + if (success) + { + char *ntuples = PQcmdTuples(results); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); + SetVariable(pset.vars, "ERROR", "FALSE"); + } + else + { + SetVariable(pset.vars, "ROW_COUNT", "0"); + SetVariable(pset.vars, "ERROR", "TRUE"); + } +} /* * SendQuery: send the query string to the backend @@ -1346,6 +1402,9 @@ SendQuery(const char *query) elapsed_msec = INSTR_TIME_GET_MILLISEC(after); } + /* set special variables to reflect the result status */ + SetResultVariables(results); + /* but printing results isn't: */ if (OK && results) OK = PrintQueryResults(results); diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out index d602aee..c3972a6 100644 --- a/src/test/regress/expected/psql.out +++ b/src/test/regress/expected/psql.out @@ -2904,6 +2904,69 @@ bar 'bar' "bar" \echo 'should print #8-1' should print #8-1 \endif +-- special result variables +-- 2 rows select +SELECT 1 AS stuff UNION SELECT 2; + stuff +--- + 1 + 2 +(2 rows) + +\if :ERROR + \echo 'MUST NOT SHOW' +\else + \echo 'ERROR is FALSE as expected' +ERROR is FALSE as expected +\endif +\echo 'status:' :STATUS +status: TUPLES_OK +\echo 'error code:' :ERROR_CODE +error code: 0 +\echo 'error message:' :ERROR_MESSAGE +error message: +\echo 'number of rows:' :ROW_COUNT +number of rows: 2 +-- syntax error +SELECT 1 UNION; +ERROR: syntax error at or near ";" +LINE 1: SELECT 1 UNION; + ^ +\echo 'status:' :STATUS +status: FATAL_ERROR +\if :ERROR + \echo 'ERROR is TRUE as expected' +ERROR is TRUE as expected +\else + \echo 'MUST NOT SHOW' +\endif +\echo 'error code:' :ERROR_CODE +error code: 42601 +\echo 'error message:' :ERROR_MESSAGE +error message: syntax error at or near ";" +\echo 'number of rows:' :ROW_COUNT +number of rows: 0 +-- empty query +; +\echo 'status:' :STATUS +status: EMPTY_QUERY +\echo 'error code:' :ERROR_CODE +error code: 0 +\echo 'error message:' :ERROR_MESSAGE +error message: +\echo 'number of rows:'
Re: [HACKERS] psql: Activate pager only for height, not width
Re: Jeff Janes 2017-05-29
Re: [HACKERS] psql: Activate pager only for height, not width
On Sun, May 28, 2017 at 10:09 PM, Brendan Jurdwrote: > Hello hackers, > > I am often frustrated by the default behaviour of the psql pager, which > will activate a pager if the output is deemed to be "too wide" for the > terminal, regardless of the number of lines output, and of the > pager_min_lines setting. > > This behaviour is sometimes desirable, but in my use patterns it is more > often the case that I want the pager to activate for output longer than > terminal height, whereas for output a little wider than the terminal, I am > happy for there to be some wrapping. This is especially the case with "\d" > output for tables, where, at 80 columns, very often the only wrapping is in > the table borders and constraint/trigger definitions. > > Usually I turn the pager off completely, and only switch it on when I am > about to execute something that will return many rows, but what I'd really > like is some way to tell psql to activate the pager as normal for height, > but to ignore width. My first thought was an alternate mode to \pset pager > -- to {'on' | 'off' | 'always'} we could add 'height'. > > Another option is to add the ability to specify the number of columns > which psql considers "too wide", analogous to pager_min_lines. I could > then set pager_min_cols to something around 150 which would work nicely for > my situation. > > I don't have strong opinions about how the options are constructed, as > long as it is possible to obtain the behaviour. > > I would be happy to produce a patch, if this seems like an acceptable > feature add. > I'd like a feature like this. I often run into the problem where one or two lines of an EXPLAIN plan are wide enough to wrap, which causes the pager to kick in. Then when I exit the pager, it clears the contents so I can't see that plan vertically adjacent to the one I'm trying to compare it to. I'd rather have the pager kick in if greater than some settable number of the lines are too wide, or if the wrapped lines would push the height above the height limit. If just one line is too wide, I'd rather just deal with them being wrapped. (You can configure the pager not to redraw the screen when exited, but I want it to redraw the screen after looking at 10,000 rows, just not after looking ten rows, one of which was 170 characters wide) Cheers, Jeff
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Pavel, I have not any other comments. The implementation is trivial. [...] Indeed. I'll mark this patch as ready for commiters. Thanks for the review. -- 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] psql - add special variable to reflect the last query status
2017-05-22 21:33 GMT+02:00 Fabien COELHO: > > Please find attached a v2 which hopefully takes into account all your >>> points above. >>> >>> Open question: should it gather more PQerrorResultField, or the two >>> selected one are enough? If more, which should be included? >>> >> >> >> I don't think so it is necessary. No in this moment. ERROR_CODE and >> ERROR_MESSAGE are fundamental - and if we add other, then we should to add >> all. Has not sense to add only some. >> > > Ok. I'm fine with stopping at CODE & MESSAGE. I have not any other comments. The implementation is trivial. I rerun all tests and tests passed. I'll mark this patch as ready for commiters. Regards Pavel > > > -- > Fabien. >
Re: [HACKERS] psql - add special variable to reflect the last query status
Please find attached a v2 which hopefully takes into account all your points above. Open question: should it gather more PQerrorResultField, or the two selected one are enough? If more, which should be included? I don't think so it is necessary. No in this moment. ERROR_CODE and ERROR_MESSAGE are fundamental - and if we add other, then we should to add all. Has not sense to add only some. Ok. I'm fine with stopping at CODE & MESSAGE. -- 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] psql - add special variable to reflect the last query status
2017-05-22 9:40 GMT+02:00 Fabien COELHO: > > Hello Pavel, > > After some discussions about what could be useful since psql scripts now accepts tests, this patch sets a few variables which can be used by psql after a "front door" (i.e. actually typed by the user) query: - RESULT_STATUS: the status of the query - ERROR: whether the query failed - ERROR_MESSAGE: ... - ROW_COUNT: #rows affected SELECT * FROM ; \if :ERROR \echo oops \q \endif I'm not sure that the names are right. Maybe STATUS would be better than RESULT_STATUS. >>> >>> I am sending review of this patch: >> >> 1. I agree so STATUS is better name, than RESULT status. >> > > Ok, looks simpler. > > Currently it returns values with prefix PGRES (like PGRES_FATAL_ERROR, >> PGRES_TUPLES_OK). Maybe we should to cut this prefix. FATAL_ERROR, >> TUPLES_OK looks better for custom level. The PGRES prefix has not sense in >> psql. >> > > Indeed. I skipped "PGRES_". > > 2. I propose availability to read ERROR_CODE - sometimes it can be more >> practical than parsing error possible translated message >> > > Ok. > > 3. The fields ERROR_MESSAGE and ROW_COUNT are set only when it has sense. >> This behave is maybe too strict for psql and the processing needs more >> nesting \if command. What do you think about -1 or 0 for ROW_COUNT (for >> DDL) and "" for ERROR_MESSAGE when there are not any error? It will be >> consistent with already implemented LASTOID variable (and other state psql >> variables). Using default values are not strict clean, but it can reduce >> complexity of psql scripts. >> > > My intention was that it could be tested with the "is defined" syntax, > which is yet to be agreed upon and implemented, so maybe generating empty > string is a better option. > > For ROW_COUNT, I think that it should be consistent with what PL/pgSQL > does, so it think that 0 should be the default. > > 4. all regress tests passed >> 5. there are not any problem with doc building >> > > Please find attached a v2 which hopefully takes into account all your > points above. > > Open question: should it gather more PQerrorResultField, or the two > selected one are enough? If more, which should be included? I don't think so it is necessary. No in this moment. ERROR_CODE and ERROR_MESSAGE are fundamental - and if we add other, then we should to add all. Has not sense to add only some. Regards Pavel > > -- > Fabien.
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Pavel, After some discussions about what could be useful since psql scripts now accepts tests, this patch sets a few variables which can be used by psql after a "front door" (i.e. actually typed by the user) query: - RESULT_STATUS: the status of the query - ERROR: whether the query failed - ERROR_MESSAGE: ... - ROW_COUNT: #rows affected SELECT * FROM ; \if :ERROR \echo oops \q \endif I'm not sure that the names are right. Maybe STATUS would be better than RESULT_STATUS. I am sending review of this patch: 1. I agree so STATUS is better name, than RESULT status. Ok, looks simpler. Currently it returns values with prefix PGRES (like PGRES_FATAL_ERROR, PGRES_TUPLES_OK). Maybe we should to cut this prefix. FATAL_ERROR, TUPLES_OK looks better for custom level. The PGRES prefix has not sense in psql. Indeed. I skipped "PGRES_". 2. I propose availability to read ERROR_CODE - sometimes it can be more practical than parsing error possible translated message Ok. 3. The fields ERROR_MESSAGE and ROW_COUNT are set only when it has sense. This behave is maybe too strict for psql and the processing needs more nesting \if command. What do you think about -1 or 0 for ROW_COUNT (for DDL) and "" for ERROR_MESSAGE when there are not any error? It will be consistent with already implemented LASTOID variable (and other state psql variables). Using default values are not strict clean, but it can reduce complexity of psql scripts. My intention was that it could be tested with the "is defined" syntax, which is yet to be agreed upon and implemented, so maybe generating empty string is a better option. For ROW_COUNT, I think that it should be consistent with what PL/pgSQL does, so it think that 0 should be the default. 4. all regress tests passed 5. there are not any problem with doc building Please find attached a v2 which hopefully takes into account all your points above. Open question: should it gather more PQerrorResultField, or the two selected one are enough? If more, which should be included? -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 3b86612..d33da32 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3449,6 +3449,35 @@ bar + ERROR + + + Whether the last query failed, as a boolean. + + + + + + ERROR_CODE + + + The error code associated to the last query, or + 0 if no error occured. + + + + + + ERROR_MESSAGE + + + If the last query failed, the associated error message, + otherwise an empty string. + + + + + FETCH_COUNT @@ -3653,6 +3682,24 @@ bar + STATUS + + + Text status of the last query. + + + + + + ROW_COUNT + + + How many rows were returned or affected by the last query. + + + + + QUIET diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index a2f1259..4a3c6a9 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -1213,6 +1213,62 @@ PrintQueryResults(PGresult *results) return success; } +/* + * Set special variables for "front door" queries + * - STATUS: last query status + * - ERROR: TRUE/FALSE, whether an error occurred + * - ERROR_CODE: code if an error occured, or "0" + * - ERROR_MESSAGE: message if an error occured, or "" + * - ROW_COUNT: how many rows were returned or affected, or "0" + */ +static void +SetResultVariables(PGresult *results) +{ + bool success; + ExecStatusType restat = PQresultStatus(results); + char *code = PQresultErrorField(results, PG_DIAG_SQLSTATE); + char *mesg = PQresultErrorField(results, PG_DIAG_MESSAGE_PRIMARY); + + SetVariable(pset.vars, "STATUS", PQresStatus(restat) + strlen("PGRES_")); + SetVariable(pset.vars, "ERROR_CODE", code ? code : "0"); + SetVariable(pset.vars, "ERROR_MESSAGE", mesg ? mesg : ""); + + /* check all possible PGRES_ */ + switch (restat) + { + case PGRES_EMPTY_QUERY: + case PGRES_TUPLES_OK: + case PGRES_COMMAND_OK: + case PGRES_COPY_OUT: + case PGRES_COPY_IN: + case PGRES_COPY_BOTH: + case PGRES_SINGLE_TUPLE: + success = true; + break; + case PGRES_BAD_RESPONSE: + case PGRES_NONFATAL_ERROR: + case PGRES_FATAL_ERROR: + success = false; + break; + default: + /* dead code */ + success = false; + psql_error("unexpected PQresultStatus: %d\n", restat); + break; + } + + if (success) + { + char *ntuples = PQcmdTuples(results); + SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0"); + SetVariable(pset.vars, "ERROR", "TRUE"); + } + else + { + SetVariable(pset.vars, "ROW_COUNT", "0"); + SetVariable(pset.vars, "ERROR", "FALSE"); + } +} /* * SendQuery: send
Re: [HACKERS] psql - add special variable to reflect the last query status
Hi 2017-04-04 23:01 GMT+02:00 Pavel Stehule: > > > 2017-04-04 22:05 GMT+02:00 Fabien COELHO : > >> >> After some discussions about what could be useful since psql scripts now >> accepts tests, this patch sets a few variables which can be used by psql >> after a "front door" (i.e. actually typed by the user) query: >> >> - RESULT_STATUS: the status of the query >> - ERROR: whether the query failed >> - ERROR_MESSAGE: ... >> - ROW_COUNT: #rows affected >> >> SELECT * FROM ; >> \if :ERROR >>\echo oops >>\q >> \endif >> >> I'm not sure that the names are right. Maybe STATUS would be better than >> RESULT_STATUS. > > I am sending review of this patch: 1. I agree so STATUS is better name, than RESULT status. Currently it returns values with prefix PGRES (like PGRES_FATAL_ERROR, PGRES_TUPLES_OK). Maybe we should to cut this prefix. FATAL_ERROR, TUPLES_OK looks better for custom level. The PGRES prefix has not sense in psql. 2. I propose availability to read ERROR_CODE - sometimes it can be more practical than parsing error possible translated message 3. The fields ERROR_MESSAGE and ROW_COUNT are set only when it has sense. This behave is maybe too strict for psql and the processing needs more nesting \if command. What do you think about -1 or 0 for ROW_COUNT (for DDL) and "" for ERROR_MESSAGE when there are not any error? It will be consistent with already implemented LASTOID variable (and other state psql variables). Using default values are not strict clean, but it can reduce complexity of psql scripts. 4. all regress tests passed 5. there are not any problem with doc building Regards Pavel > > good ideas > > Regards > > Pavel > >> >> >> -- >> 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] psql - add special variable to reflect the last query status
2017-04-04 22:05 GMT+02:00 Fabien COELHO: > > After some discussions about what could be useful since psql scripts now > accepts tests, this patch sets a few variables which can be used by psql > after a "front door" (i.e. actually typed by the user) query: > > - RESULT_STATUS: the status of the query > - ERROR: whether the query failed > - ERROR_MESSAGE: ... > - ROW_COUNT: #rows affected > > SELECT * FROM ; > \if :ERROR >\echo oops >\q > \endif > > I'm not sure that the names are right. Maybe STATUS would be better than > RESULT_STATUS. good ideas Regards Pavel > > > -- > 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHOwrote: > What would be the mnemonic for "," an "@"? Oh, I just picked it because control-@ is the nul character, and your commands would be nullified. I realize that's pretty weak, but we're talking about finding a punctuation mark to represent the concept of commands-are-currently-being-skipped, and it doesn't seem particularly worse than ^ to represent single-line mode. If somebody's got a better idea, fine, but there aren't that many unused punctuation marks to choose from, and I think it's better to use a punctuation mark rather than, say, a letter, like 's' for skip. Otherwise you might have the prompt change from: banana=> to bananas> Which I think is less obvious than banana@> -- 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
For two states: * for being executed (beware, it is ***important***) It does lend importance, but that's also the line continuation marker for "comment". Would that be a problem? Argh. Indeed, even if people seldom type C comments in psql interactive mode... Remaining ASCII characters I can thing of, hopefully avoiding already used ones: +%,@$\`|&:;_ So, maybe consider these ones: "+" for it is "on" "`" which is a "sub-shell execution" "&" for "and the next command is ..." / for not (under the hood, and it is opposed to *) +1, I was going to suggest '/' for a false state, with two possible metaphors to justify it 1. the slash in a "no" sign ("no smoking", ghostbusters, etc) 2. the leading char of a c/java/javascript comment (what is written here is just words, not code) Great. -- 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > @in't gonna execute it? >> > > Hmmm... This is too much of an Americanism, IMHO. The @ looks like a handwritten 'a'. @in't gonna => ain't gonna => will not. It's a bad joke, made as a way of saying that I also could not think of a good mnemonic for '@' or ','. > I'm here all week, try the veal. >> > > Sorry, syntax error, you have lost me. Some googling suggests a reference > to post WW2 "lounge entertainers", probably in the USA. I also do not > understand why this would mean "yes". It's a thing lounge entertainers said after they told a bad joke. > . for z (waiting for the end of the sentence, i.e. endif) > +1 ... if we end up displaying the not-true-and-not-evaluated 'z' state. > & for t (no real mnemonic) > > For two states: > * for being executed (beware, it is ***important***) > It does lend importance, but that's also the line continuation marker for "comment". Would that be a problem? > / for not (under the hood, and it is opposed to *) > +1, I was going to suggest '/' for a false state, with two possible metaphors to justify it 1. the slash in a "no" sign ("no smoking", ghostbusters, etc) 2. the leading char of a c/java/javascript comment (what is written here is just words, not code)
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Corey, If I can find some simple mnemonic for "," vs "@" for being executed vs ignored, I could live with that, but nothing obvious comes to my mind. @in't gonna execute it? Hmmm... This is too much of an Americanism, IMHO. I'm here all week, try the veal. Sorry, syntax error, you have lost me. Some googling suggests a reference to post WW2 "lounge entertainers", probably in the USA. I also do not understand why this would mean "yes". I'd be fine with either of these on aesthetic grounds. On technical grounds, 'z' is harder to show. I'm not sure that this valid technical point should be a good reason for guiding what feedback should be provided to the user, but it makes it simpler to choose two states:-) For three states with more culturally neutral mnemonics, I thought of: ? for f (waiting for a true answer...) . for z (waiting for the end of the sentence, i.e. endif) & for t (no real mnemonic) For two states: * for being executed (beware, it is ***important***) / for not (under the hood, and it is opposed to *) Otherwise I still like "?[tfz]", but it is two characters long. -- 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHOwrote: > > Hello Robert, > > [...] I think we should try to make this REALLY simple. We don't really >> want to have everybody have to change their PROMPT1 and PROMPT2 strings for >> this one feature. >> > > Ok. I think that we agree that the stack was too much details. > > How about just introducing a new value for %R? >> > > Yes. That is indeed one of the idea being discussed. > > [...] , or @ if commands are currently being ignored because of the result >> of an \if test. >> > ,-or-@ has one advantage over t/f/z: we cannot infer the 'z' state purely from pset.active_state, and the if-stack itself is sequestered in scan_state, which is not visible to the get_prompt() function. I suppose if somebody wanted it, a separate slash command that does a verbose printing of the current if-stack would be nice, but mostly just to explain to people how the if-stack works. > If I can find some simple mnemonic for "," vs "@" for being executed vs > ignored, I could live with that, but nothing obvious comes to my mind. > @in't gonna execute it? I'm here all week, try the veal. To sum up your points: just update %R (ok), keep it simple/short (ok... but > how simple [2 vs 3 states] and short [1 or 2 chars]), and no real need to > be too nice with the user beyond the vital (ok, that significantly > simplifies things). I'd be fine with either of these on aesthetic grounds. On technical grounds, 'z' is harder to show.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Mon, Feb 13, 2017 at 11:29 AM, Robert Haaswrote: > possible states here. Just when you've figured out what tfzffft I agree with what you've said, but wanted to point out that any condition that follows a 'z' would itself be 'z'. Not that tfz is much better.
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello Robert, [...] I think we should try to make this REALLY simple. We don't really want to have everybody have to change their PROMPT1 and PROMPT2 strings for this one feature. Ok. I think that we agree that the stack was too much details. How about just introducing a new value for %R? Yes. That is indeed one of the idea being discussed. [...] , or @ if commands are currently being ignored because of the result of an \if test. Currently I find that %R logic is quite good, with "=" for give me something, "^" is start line regular expression for one line, "!" for beware someting is amiss, and in prompt2 "-" for continuation, '"' for in double quotes, "(" for in parenthesis and so on. What would be the mnemonic for "," an "@"? By shortening one of the suggestion down to two characters, we may have three cases: "?t" for "in condition, in true block" "?f" for "in condition, in false block (but true yet to come)" "?z" for "in condition, waiting for the end (true has been executed)". So no indication about the stack depth and contents. tfz for true false and sleeping seem quite easy to infer and understand. "?" is also needed as a separator with the previous field which is the database name sometimes: calvin=> \if false calvin?f=> \echo 1 calvin?f=> \elif true calvin?t=> \echo 2 2 calvin?t=> \else calvin?z=> \echo 3 calvin?z=> \endif calvin=> With the suggested , and @: calvin=> \if false calvin,=> \echo 1 calvin,=> \elif true calvin@=> \echo 2 2 calvin@=> \else calvin,=> \echo 3 calvin,=> \endif calvin=> If I can find some simple mnemonic for "," vs "@" for being executed vs ignored, I could live with that, but nothing obvious comes to my mind. The "?" for condition and Corey's [tfz] looked quite intuitive/mnemonic to me. The drawback is that it is 2 chars vs one char in above. [...] I think that's all you need here: a way to alert users as to whether commands are being ignored, or not. Yep. [...] To sum up your points: just update %R (ok), keep it simple/short (ok... but how simple [2 vs 3 states] and short [1 or 2 chars]), and no real need to be too nice with the user beyond the vital (ok, that significantly simplifies things). -- 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On Sat, Feb 11, 2017 at 2:43 AM, Fabien COELHOwrote: >> Ok, so that's not just PROMPT_READY, that's every prompt...which might be >> ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd >> level always being '.'? > > Yep. The idea is to keep it short, but to still have something to say "there > are more levels" in the stack, hence the one dot. Basically I just > compressed your 4 level proposal, and added a separator to deal with the > preceding stuff and suggest the conditional. I think we should try to make this REALLY simple. We don't really want to have everybody have to change their PROMPT1 and PROMPT2 strings for this one feature. How about just introducing a new value for %R? The documentation currently says: In prompt 1 normally =, but ^ if in single-line mode, or ! if the session is disconnected from the database (which can happen if \connect fails). ...and suppose we just extend that to add: , or @ if commands are currently being ignored because of the result of an \if test. The latter would include being in the \if section when the conditional was true as well as being in the \else section when the conditional was false. I think that's all you need here: a way to alert users as to whether commands are being ignored, or not. Putting details in about precisely why they are being ignored seems like it's too complicated; people won't remember how to decode some bizarre series of glyphs that we output. Telling them whether their next command is set to be ignored or executed is good enough; if the answer isn't what they expect, they can debug their script to figure out what they screwed up. Also, keep in mind that people don't need to know everything from the current prompt. They can try to debug things by looking back at previous prompts. They'll understand that \if is going to introduce a new nesting level and \endif is going to end one, and that \else and \elseif may change things. Aside from keeping the code simple so we can maintain it and the output simple so that users can remember what it means, I just don't believe that it's really going to be helpful to convey much detail here. People aren't going to paste in a gigaton of commands and then look only at the last line of the output and try to understand what it's telling them, or if they do that and are confused, I think nobody will really feel bad about giving them the advice "scroll up" or "try a simpler test case first". Further keep in mind that eventually somebody's going to code \while or \for or something, and then there are going to be even more possible states here. Just when you've figured out what tfzffft means, they'll be the case of a \while loop which is getting skipped because the condition at the top turned out to be false on the first iteration, or where (half-joking) we're skipping commands until we find the label that matches an executed \goto. Writing maintainable code includes leaving room open for other people to do stuff we can't even foresee today, and that means we need not to use up a disproportionate number of the glyphs that can reasonably be used in a psql prompt just on this. This is one small feature out of many that psql has, and one small hint to the user about whether it's currently causing commands to be skipped seems sufficient. All IMHO, of course. -- 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