Re: [HACKERS] psql \d sequence display

2017-09-29 Thread Peter Eisentraut
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

2017-09-25 Thread Fabien COELHO


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

2017-09-20 Thread Fabien COELHO



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

2017-09-20 Thread Robins Tharakan
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

2017-09-20 Thread Fabien COELHO


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

2017-09-20 Thread Robins Tharakan
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

2017-09-20 Thread Robins Tharakan
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 COELHO  wrote:

>
> 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

2017-09-20 Thread Fabien COELHO


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

2017-09-19 Thread Robins Tharakan
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 Thread Pavel Stehule
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

2017-09-18 Thread 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.)

-- 
Á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-16 Thread Pavel Stehule
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 Thread 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.

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 Thread 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'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

2017-09-13 Thread Fabien COELHO


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

2017-09-13 Thread Tom Lane
"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

2017-09-13 Thread David G. Johnston
On Wed, Sep 13, 2017 at 12:46 PM, Fabien COELHO  wrote:

>
> 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

2017-09-13 Thread Fabien COELHO


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

2017-09-13 Thread Tom Lane
Alvaro Herrera  writes:
> 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

2017-09-13 Thread Alvaro Herrera
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 Thread Pavel Stehule
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

2017-09-13 Thread 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.

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-13 Thread Fabien COELHO



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

2017-09-13 Thread Fabien COELHO


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

2017-09-13 Thread Fabien COELHO



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-12 Thread Pavel Stehule
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

2017-09-12 Thread Tom Lane
Fabien COELHO  writes:
> 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

2017-09-12 Thread Fabien COELHO



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

2017-09-12 Thread Tom Lane
Robert Haas  writes:
> 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

2017-09-12 Thread Robert Haas
On Tue, Sep 12, 2017 at 3:12 PM, Tom Lane  wrote:
>> 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

2017-09-12 Thread Tom Lane
Robert Haas  writes:
> 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 Thread Pavel Stehule
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

2017-09-12 Thread 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.

-- 
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-09-12 Thread Fabien COELHO


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 Thread Pavel Stehule
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

2017-09-11 Thread 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.


--
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-09-11 Thread Tom Lane
Fabien COELHO  writes:
>> 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

2017-09-11 Thread Fabien COELHO


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

2017-09-11 Thread Tom Lane
Fabien COELHO  writes:
> 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

2017-09-11 Thread 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

-- 
Á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-09 Thread Daniel Verite
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

2017-09-09 Thread Tomas Vondra
On 09/09/2017 01:24 AM, Tom Lane wrote:
> 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.
> 

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

2017-09-09 Thread Pavel Stehule
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

2017-09-08 Thread 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.

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

2017-09-08 Thread Tomas Vondra
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

2017-09-07 Thread Fabien COELHO


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-07 Thread Pavel Stehule
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

2017-09-07 Thread 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.


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

2017-09-07 Thread Fabien COELHO


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

2017-09-07 Thread Pavel Stehule
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

2017-09-06 Thread 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.

--
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

2017-09-06 Thread Fabien COELHO


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

2017-09-05 Thread Fabien COELHO



* 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

2017-09-05 Thread Tom Lane
Fabien COELHO  writes:
>> * 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

2017-09-05 Thread Fabien COELHO


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

2017-09-05 Thread Tom Lane
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

2017-08-28 Thread Fabien COELHO



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 Thread Pavel Stehule
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

2017-08-28 Thread 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.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


Re: [HACKERS] psql --batch

2017-08-28 Thread Fabien COELHO



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

2017-08-28 Thread Craig Ringer
On 28 August 2017 at 15:34, Pavel Stehule  wrote:

>
>
> 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 Thread Pavel Stehule
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

2017-08-28 Thread 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.


--
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 Thread Pavel Stehule
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

2017-08-28 Thread Craig Ringer
On 28 August 2017 at 14:56, Fabien COELHO  wrote:

>
> 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

2017-08-28 Thread 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"?


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

2017-08-26 Thread Fabien COELHO



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

2017-08-26 Thread Corey Huinker
On Sat, Aug 26, 2017 at 2:09 PM, Pavel Stehule 
wrote:

>
>
> 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 Thread Pavel Stehule
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

2017-08-26 Thread 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.

--
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

2017-08-26 Thread Fabien COELHO


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 Thread Pavel Stehule
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 Thread Pavel Stehule
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

2017-06-28 Thread 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.


--
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 Thread Pavel Stehule
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

2017-06-28 Thread 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.

--
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-28 Thread Pavel Stehule
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

2017-06-27 Thread 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.


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

2017-06-27 Thread Pavel Stehule
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

2017-06-22 Thread Brendan Jurd
On Tue, 30 May 2017 at 05:30 Christoph Berg  wrote:

> 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

2017-06-22 Thread Robert Haas
On Fri, Mar 24, 2017 at 7:10 AM, Rushabh Lathia
 wrote:
> 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-18 Thread 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");

Regards

Pavel

>
>
> --
> Fabien.


Re: [HACKERS] psql - add special variable to reflect the last query status

2017-06-16 Thread 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.

--
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

2017-05-29 Thread Christoph Berg
Re: Jeff Janes 2017-05-29 

Re: [HACKERS] psql: Activate pager only for height, not width

2017-05-29 Thread Jeff Janes
On Sun, May 28, 2017 at 10:09 PM, Brendan Jurd  wrote:

> 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

2017-05-22 Thread Fabien COELHO


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 Thread Pavel Stehule
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

2017-05-22 Thread 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.

--
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 Thread Pavel Stehule
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

2017-05-22 Thread 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?


--
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

2017-05-21 Thread Pavel Stehule
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 Thread 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.


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)

2017-02-14 Thread Robert Haas
On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHO  wrote:
> 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)

2017-02-14 Thread Fabien COELHO



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)

2017-02-14 Thread Corey Huinker
>
> @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)

2017-02-13 Thread Fabien COELHO


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)

2017-02-13 Thread Corey Huinker
On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHO  wrote:

>
> 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)

2017-02-13 Thread Corey Huinker
On Mon, Feb 13, 2017 at 11:29 AM, Robert Haas  wrote:

> 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)

2017-02-13 Thread Fabien COELHO


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)

2017-02-13 Thread Robert Haas
On Sat, Feb 11, 2017 at 2:43 AM, Fabien COELHO  wrote:
>> 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


  1   2   3   4   5   6   7   8   9   10   >