psql: show only failed queries

On Sat, Mar 1, 2014 at 8:01 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hello

I was asked, how can be showed only failed queries in psql.

I am thinking, so it is not possible now. But implementation is very

simple

What do you think about it?

bash-4.1$ psql postgres -v ECHO=error -f data.sql
INSERT 0 1
Time: 27.735 ms
INSERT 0 1
Time: 8.303 ms
psql:data.sql:3: ERROR: value too long for type character varying(2)
insert into foo values('bbb');
Time: 0.178 ms
INSERT 0 1
Time: 8.285 ms
psql:data.sql:5: ERROR: value too long for type character varying(2)
insert into foo values('ssssss');
Time: 0.422 ms

The patch works fine, but I think we must add some prefix to printed query.
Like that:

fabrizio=# \set ECHO error
fabrizio=# insert into foo values ('XXX');
ERROR: value too long for type character varying(2)
DETAIL: insert into foo values ('XXX');

or

fabrizio=# \set ECHO error
fabrizio=# insert into foo values ('XXX');
ERROR: value too long for type character varying(2)
QUERY: insert into foo values ('XXX');

This may help to filter the output with some tool like 'grep'.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

2014-03-04 6:35 GMT+01:00 Fabrízio de Royes Mello <fabriziomello@gmail.com>:

On Sat, Mar 1, 2014 at 8:01 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hello

I was asked, how can be showed only failed queries in psql.

I am thinking, so it is not possible now. But implementation is very

simple

What do you think about it?

bash-4.1$ psql postgres -v ECHO=error -f data.sql
INSERT 0 1
Time: 27.735 ms
INSERT 0 1
Time: 8.303 ms
psql:data.sql:3: ERROR: value too long for type character varying(2)
insert into foo values('bbb');
Time: 0.178 ms
INSERT 0 1
Time: 8.285 ms
psql:data.sql:5: ERROR: value too long for type character varying(2)
insert into foo values('ssssss');
Time: 0.422 ms

The patch works fine, but I think we must add some prefix to printed
query. Like that:

fabrizio=# \set ECHO error
fabrizio=# insert into foo values ('XXX');

ERROR: value too long for type character varying(2)
DETAIL: insert into foo values ('XXX');

or

fabrizio=# \set ECHO error
fabrizio=# insert into foo values ('XXX');

ERROR: value too long for type character varying(2)
QUERY: insert into foo values ('XXX');

This may help to filter the output with some tool like 'grep'.

sure, good idea.

I add link to your notice to commitfest app

Regards

Pavel

Hello

updated patch - only one change: query is prefixed by "QUERY: "

current state:

[pavel@localhost ~]$ src/postgresql/src/bin/psql/psql postgres -q -f
data.sql
psql:data.sql:6: ERROR: value too long for type character varying(3)

Show only errors mode:

[pavel@localhost ~]$ src/postgresql/src/bin/psql/psql postgres -q -v
ECHO=error -f data.sql
psql:data.sql:6: ERROR: value too long for type character varying(3)
QUERY: INSERT INTO bubu VALUES('Ahoj');

Now, when I am thinking about these results, I am thinking, so second
variant is more practical and can be default.

Opinions, notes?

Regards

Pavel

2014-03-04 8:52 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:

On 6/4/14, 11:54 AM, Pavel Stehule wrote:

updated patch - only one change: query is prefixed by "QUERY: "

In the backend server log, this is called "STATEMENT: ".

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-06-04 18:16 GMT+02:00 Peter Eisentraut <peter_e@gmx.net>:

On 6/4/14, 11:54 AM, Pavel Stehule wrote:

updated patch - only one change: query is prefixed by "QUERY: "

In the backend server log, this is called "STATEMENT: ".

good idea

updated patch

Pavel

Hi Pavel,

After applying patch, on error condition it displays error message two
times as follows:

ERROR: column "abc" does not exist at character 23
STATEMENT: insert into ax
values(abc);
psql:a.sql:7: ERROR: column "abc" does not exist
LINE 2: values(abc);

user may confuse because of repeated error messages. so I think its better
to display only one message, one of the possible ways is as follows:

ERROR: column "abc" does not exist at character 23
STATEMENT: insert into ax
values(abc);

Am I missing something ?

On Wed, Jun 4, 2014 at 9:52 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-06-04 18:16 GMT+02:00 Peter Eisentraut <peter_e@gmx.net>:

On 6/4/14, 11:54 AM, Pavel Stehule wrote:

updated patch - only one change: query is prefixed by "QUERY: "

In the backend server log, this is called "STATEMENT: ".

good idea

updated patch

Pavel

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Regards,

Samrat Revgade

2014-06-25 12:32 GMT+02:00 Samrat Revagade <revagade.samrat@gmail.com>:

Hi Pavel,

After applying patch, on error condition it displays error message two
times as follows:

ERROR: column "abc" does not exist at character 23
STATEMENT: insert into ax
values(abc);
psql:a.sql:7: ERROR: column "abc" does not exist
LINE 2: values(abc);

user may confuse because of repeated error messages. so I think its better
to display only one message, one of the possible ways is as follows:

ERROR: column "abc" does not exist at character 23
STATEMENT: insert into ax
values(abc);

Am I missing something ?

LINE info is a part of error message and should be eliminated by terse mode

[pavel@localhost ~]$ psql -v ECHO=error -f test.sql postgres > /dev/null
psql:test.sql:4: ERROR: syntax error at or near ";"
LINE 2: 10 + ;
^
psql:test.sql:4: STATEMENT: select
10 + ;
psql:test.sql:8: ERROR: syntax error at end of input
LINE 2: 30 +
^
psql:test.sql:8: STATEMENT: select
30 +

but you can switch to terse mode:

[pavel@localhost ~]$ psql -v ECHO=error -v VERBOSITY=terse -f test.sql
postgres > /dev/null
psql:test.sql:4: ERROR: syntax error at or near ";" at character 13
psql:test.sql:4: STATEMENT: select
10 + ;
psql:test.sql:8: ERROR: syntax error at end of input at character 13
psql:test.sql:8: STATEMENT: select
30 +

What is what you would

I am sending updated patch - buggy statement is printed via more logical
psql_error function instead printf

Regards

Pavel

ECHO_HIDDEN?

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

I am sending updated patch - buggy statement is printed via more logical

psql_error function instead printf

Thank you for updating patch, I really appreciate your efforts.

Now, everything is good from my side.
* it apply cleanly to the current git master
* includes necessary docs
* I think It is very good and necessary feature.

If Kumar Rajeev Rastogi do not have any extra comments, then I think patch
is ready for committer.

2014-06-26 8:22 GMT+02:00 Samrat Revagade <revagade.samrat@gmail.com>:

I am sending updated patch - buggy statement is printed via more logical

psql_error function instead printf

Thank you for updating patch, I really appreciate your efforts.

Now, everything is good from my side.
* it apply cleanly to the current git master
* includes necessary docs
* I think It is very good and necessary feature.

If Kumar Rajeev Rastogi do not have any extra comments, then I think patch
is ready for committer.

Thank you very much

Regards

Pavel

On 26 June 2014 11:53, Samrat Revagade Wrote:

I am sending updated patch - buggy statement is printed via more logical psql_error function instead printf

Thank you for updating patch, I really appreciate your efforts.

Now, everything is good from my side.
* it apply cleanly to the current git master
* includes necessary docs
* I think It is very good and necessary feature.

If Kumar Rajeev Rastogi do not have any extra comments, then I think patch is ready for committer.

I have reviewed this patch. Please find my review comments below:

1. Command start-up option (e.g. -a/--echo-all for --ECHO=all), for new functionality is not provided.

2. New Command start-up option should be added in "psql --help" as well as in documentation.

Also as I understand, this new option is kind of sub-set of existing option (ECHO=query), so should not we display
query string in the same format as it was getting printed earlier.
Though I also feel that prefixing query with STATEMENT word will be helpful to grep but at the same time I am worried
about inconsistency with existing option.

Thanks and Regards,
Kumar Rajeev Rastogi

2014-06-30 8:17 GMT+02:00 Rajeev rastogi <rajeev.rastogi@huawei.com>:

On 26 June 2014 11:53, Samrat Revagade Wrote:

I am sending updated patch - buggy statement is printed via more

logical psql_error function instead printf

Thank you for updating patch, I really appreciate your efforts.

Now, everything is good from my side.

* it apply cleanly to the current git master

* includes necessary docs

* I think It is very good and necessary feature.

If Kumar Rajeev Rastogi do not have any extra comments, then I

think patch is ready for committer.

I have reviewed this patch. Please find my review comments below:

1. Command start-up option (e.g. -a/--echo-all for --ECHO=all), for
new functionality is not provided.

all not options entered via psql variables has psql option and psql
comment. I'll plan add new decription to --help-variables list.

If it is necessary I can add long option --echo-errors, I didn't a good
char for short option. Any idea?

2. New Command start-up option should be added in "psql --help" as
well as in documentation.

depends on previous,

Also as I understand, this new option is kind of sub-set of existing
option (ECHO=query), so should not we display

query string in the same format as it was getting printed earlier.

Though I also feel that prefixing query with STATEMENT word will be
helpful to grep but at the same time I am worried

about inconsistency with existing option.

This is question. And I am not strong in feeling what should be preferred.
But still I am inclined to prefer a variant with STATEMENT prefix. Mode
with -a is used with different purpose than mode "show errors only" - and
output with prefix is much more consistent with log entry - and displaying
error. So I agree, so there is potential inconsistency (but nowhere is
output defined), but this output is more practical, when you are
concentrated to error's processing.

Regards

Pavel

On 30 June 2014 12:24, Pavel Stehule Wrote:

I have reviewed this patch. Please find my review comments below:

1. Command start-up option (e.g. -a/--echo-all for --ECHO=all), for new functionality is not provided.

all not options entered via psql variables has psql option and psql comment. I'll plan add new decription to --help-variables list.
If it is necessary I can add long option --echo-errors, I didn't a good char for short option. Any idea?

But the new option we are adding are on a track of existing option, so better we add start-up option for this also.
Yeah long option –echo-errors seems to be fine to me also. For short option, I think we can use “-b” stands for blunder. This is the closest one I could think of.

2. New Command start-up option should be added in "psql --help" as well as in documentation.

depends on previous,

Right.

Also as I understand, this new option is kind of sub-set of existing option (ECHO=query), so should not we display
query string in the same format as it was getting printed earlier.
Though I also feel that prefixing query with STATEMENT word will be helpful to grep but at the same time I am worried
about inconsistency with existing option.

This is question. And I am not strong in feeling what should be preferred. But still I am inclined to prefer a variant with STATEMENT prefix. Mode with -a is used with different purpose than mode "show errors only" - and output with prefix is much
more consistent with log entry - and displaying error. So I agree, so there is potential inconsistency (but nowhere is output defined), but this output is more practical, when you are concentrated to error's processing.

Yeah right, I just wanted to raise point to provoke other thought to see if anyone having different opinion. If no objection from others, we can go ahead with the current prefixing approach.
Thanks and Regards,
Kumar Rajeev Rastogi

2014-06-30 11:20 GMT+02:00 Rajeev rastogi <rajeev.rastogi@huawei.com>:

On 30 June 2014 12:24, Pavel Stehule Wrote:

I have reviewed this patch. Please find my review comments below:

1. Command start-up option (e.g. -a/--echo-all for --ECHO=all), for

new functionality is not provided.

all not options entered via psql variables has psql option and psql

comment. I'll plan add new decription to --help-variables list.

If it is necessary I can add long option --echo-errors, I didn't a good

char for short option. Any idea?

But the new option we are adding are on a track of existing option, so
better we add start-up option for this also.

Yeah long option –echo-errors seems to be fine to me also. For short
option, I think we can use “-b” stands for blunder. This is the closest one
I could think of.

fixed

see a attachment pls

2. New Command start-up option should be added in "psql --help" as

well as in documentation.

depends on previous,

Right.

Also as I understand, this new option is kind of sub-set of existing

option (ECHO=query), so should not we display

query string in the same format as it was getting printed earlier.

Though I also feel that prefixing query with STATEMENT word will be

helpful to grep but at the same time I am worried

about inconsistency with existing option.

This is question. And I am not strong in feeling what should be

preferred. But still I am inclined to prefer a variant with STATEMENT
prefix. Mode with -a is used with different purpose than mode "show errors
only" - and output with prefix is much

more consistent with log entry - and displaying error. So I agree, so

there is potential inconsistency (but nowhere is output defined), but this
output is more practical, when you are concentrated to error's processing.

Yeah right, I just wanted to raise point to provoke other thought to see
if anyone having different opinion. If no objection from others, we can go
ahead with the current prefixing approach.

ok, we can wait two days

Regards

Pavel

At 2014-06-30 12:48:30 +0200, pavel.stehule@gmail.com wrote:

+      <para>
+      Print a failed SQL commands to standard error output. This is
+      equivalent to setting the variable <varname>ECHO</varname> to
+      <literal>errors</literal>.

No "a", just "Print failed SQL commands …".

-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>error</literal> then only
+        failed queries are displayed.

Should be "errors" here, not "error".

printf(_(" -a, --echo-all echo all input from script\n"));
+ printf(_(" -b --echo-errors echo failed commands sent to server\n"));
printf(_(" -e, --echo-queries echo commands sent to server\n"));

Should have a comma after -b to match other options. Also I would remove
"sent to server" from the description: "echo failed commands" is fine.

Otherwise looks fine. I see no reason to wait for further feedback, so
I'll mark this ready for committer if you make the above corrections.

At some point, you should probably also update your --help-variables
patch to add this new value to the description of ECHO.

-- Abhijit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-06-30 13:01 GMT+02:00 Abhijit Menon-Sen <ams@2ndquadrant.com>:

At 2014-06-30 12:48:30 +0200, pavel.stehule@gmail.com wrote:

+      <para>
+      Print a failed SQL commands to standard error output. This is
+      equivalent to setting the variable <varname>ECHO</varname> to
+      <literal>errors</literal>.

No "a", just "Print failed SQL commands …".

-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>error</literal> then

only

+ failed queries are displayed.

Should be "errors" here, not "error".

printf(_(" -a, --echo-all echo all input from

script\n"));

+ printf(_(" -b --echo-errors echo failed commands sent to

server\n"));

printf(_(" -e, --echo-queries echo commands sent to

server\n"));

Should have a comma after -b to match other options. Also I would remove
"sent to server" from the description: "echo failed commands" is fine.

fixed

Otherwise looks fine. I see no reason to wait for further feedback, so
I'll mark this ready for committer if you make the above corrections.

At some point, you should probably also update your --help-variables
patch to add this new value to the description of ECHO.

I have it in TODO. But I don't would to introduce a dependency between
these patches - so when first patch will be committed, than I update second
patch

Regards

Pavel

On Mon, Jun 30, 2014 at 8:33 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2014-06-30 13:01 GMT+02:00 Abhijit Menon-Sen <ams@2ndquadrant.com>:

At 2014-06-30 12:48:30 +0200, pavel.stehule@gmail.com wrote:

+      <para>
+      Print a failed SQL commands to standard error output. This is
+      equivalent to setting the variable <varname>ECHO</varname> to
+      <literal>errors</literal>.

No "a", just "Print failed SQL commands …".

-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>error</literal> then
only
+        failed queries are displayed.

Should be "errors" here, not "error".

printf(_(" -a, --echo-all echo all input from
script\n"));
+ printf(_(" -b --echo-errors echo failed commands sent to
server\n"));
printf(_(" -e, --echo-queries echo commands sent to
server\n"));

Should have a comma after -b to match other options. Also I would remove
"sent to server" from the description: "echo failed commands" is fine.

fixed

$ psql -b
bin/psql: invalid option -- 'b'
Try "psql --help" for more information.

I got this error. ISTM you forgot to add 'b' into the third argument of
getopt_long in startup.c.

         <application>psql</application> merely prints all queries as
         they are sent to the server. The switch for this is
-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>errors</literal> then only
+        failed queries are displayed.

I think that where failed queries are output should be documented here.
Otherwise users might misunderstand they are output to standard output
like ECHO=all and queries do.

It's better to add "The switch for this is <option>-b</option>." into the doc.

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "ECHO") == 0)
+        {
+            static const char *const my_list[] =
+            {"none", "errors", "queries", "all", NULL};
+
+            COMPLETE_WITH_LIST_CS(my_list);
+        }
+        else if (strcmp(prev_wd, "ECHO_HIDDEN") == 0)
+        {
+            static const char *const my_list[] =
+            {"noexec", "off", "on", NULL};
+
+            COMPLETE_WITH_LIST_CS(my_list);
+        }
+    }

I think that adding tab-completions of psql variables is good, but
adding those of only ECHO and ECHO_HIDDEN seems half-baked.
Probably this part should be split into separate patch.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi

2014-07-09 7:07 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Mon, Jun 30, 2014 at 8:33 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-06-30 13:01 GMT+02:00 Abhijit Menon-Sen <ams@2ndquadrant.com>:

At 2014-06-30 12:48:30 +0200, pavel.stehule@gmail.com wrote:

+      <para>
+      Print a failed SQL commands to standard error output. This is
+      equivalent to setting the variable <varname>ECHO</varname> to
+      <literal>errors</literal>.

No "a", just "Print failed SQL commands …".

-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>error</literal> then
only
+        failed queries are displayed.

Should be "errors" here, not "error".

printf(_(" -a, --echo-all echo all input from
script\n"));
+ printf(_(" -b --echo-errors echo failed commands sent

to

server\n"));
printf(_(" -e, --echo-queries echo commands sent to
server\n"));

Should have a comma after -b to match other options. Also I would remove
"sent to server" from the description: "echo failed commands" is fine.

fixed

$ psql -b
bin/psql: invalid option -- 'b'
Try "psql --help" for more information.

I got this error. ISTM you forgot to add 'b' into the third argument of
getopt_long in startup.c.

fixed

<application>psql</application> merely prints all queries as
they are sent to the server. The switch for this is
-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>errors</literal> then only
+        failed queries are displayed.

I think that where failed queries are output should be documented here.
Otherwise users might misunderstand they are output to standard output
like ECHO=all and queries do.

It's better to add "The switch for this is <option>-b</option>." into the
doc.

fixed

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "ECHO") == 0)
+        {
+            static const char *const my_list[] =
+            {"none", "errors", "queries", "all", NULL};
+
+            COMPLETE_WITH_LIST_CS(my_list);
+        }
+        else if (strcmp(prev_wd, "ECHO_HIDDEN") == 0)
+        {
+            static const char *const my_list[] =
+            {"noexec", "off", "on", NULL};
+
+            COMPLETE_WITH_LIST_CS(my_list);
+        }
+    }

I think that adding tab-completions of psql variables is good, but
adding those of only ECHO and ECHO_HIDDEN seems half-baked.
Probably this part should be split into separate patch.

fixed

please, see updated patch in attachment

Thank you

Regards

Pavel

On Wed, Jul 9, 2014 at 9:06 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

2014-07-09 7:07 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Mon, Jun 30, 2014 at 8:33 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-06-30 13:01 GMT+02:00 Abhijit Menon-Sen <ams@2ndquadrant.com>:

At 2014-06-30 12:48:30 +0200, pavel.stehule@gmail.com wrote:

+      <para>
+      Print a failed SQL commands to standard error output. This is
+      equivalent to setting the variable <varname>ECHO</varname> to
+      <literal>errors</literal>.

No "a", just "Print failed SQL commands …".

-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>error</literal> then
only
+        failed queries are displayed.

Should be "errors" here, not "error".

printf(_(" -a, --echo-all echo all input from
script\n"));
+ printf(_(" -b --echo-errors echo failed commands sent
to
server\n"));
printf(_(" -e, --echo-queries echo commands sent to
server\n"));

Should have a comma after -b to match other options. Also I would
remove
"sent to server" from the description: "echo failed commands" is fine.

fixed

$ psql -b
bin/psql: invalid option -- 'b'
Try "psql --help" for more information.

I got this error. ISTM you forgot to add 'b' into the third argument of
getopt_long in startup.c.

fixed

<application>psql</application> merely prints all queries as
they are sent to the server. The switch for this is
-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>errors</literal> then
only
+        failed queries are displayed.

I think that where failed queries are output should be documented here.
Otherwise users might misunderstand they are output to standard output
like ECHO=all and queries do.

It's better to add "The switch for this is <option>-b</option>." into the
doc.

fixed

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "ECHO") == 0)
+        {
+            static const char *const my_list[] =
+            {"none", "errors", "queries", "all", NULL};
+
+            COMPLETE_WITH_LIST_CS(my_list);
+        }
+        else if (strcmp(prev_wd, "ECHO_HIDDEN") == 0)
+        {
+            static const char *const my_list[] =
+            {"noexec", "off", "on", NULL};
+
+            COMPLETE_WITH_LIST_CS(my_list);
+        }
+    }

I think that adding tab-completions of psql variables is good, but
adding those of only ECHO and ECHO_HIDDEN seems half-baked.
Probably this part should be split into separate patch.

fixed

please, see updated patch in attachment

Thanks for updating the patch!

Barring any objection, I will commit this patch except tab-completion part.

I'm not against adding tab-completion support for psql variables, but I'm
not sure if it's good idea or not to treat only one or two variables special
and add tab-completions for them. There are other variables which accept
special argument (e.g., COMP_KEYWORD_CASE) and it's basically worth
adding tab-completion support for them.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-07-09 14:39 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Wed, Jul 9, 2014 at 9:06 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

2014-07-09 7:07 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Mon, Jun 30, 2014 at 8:33 PM, Pavel Stehule <pavel.stehule@gmail.com

wrote:

2014-06-30 13:01 GMT+02:00 Abhijit Menon-Sen <ams@2ndquadrant.com>:

At 2014-06-30 12:48:30 +0200, pavel.stehule@gmail.com wrote:

+      <para>
+      Print a failed SQL commands to standard error output. This

is

+ equivalent to setting the variable <varname>ECHO</varname>

to

+ <literal>errors</literal>.

No "a", just "Print failed SQL commands …".

-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>error</literal>

then

only
+ failed queries are displayed.

Should be "errors" here, not "error".

printf(_(" -a, --echo-all echo all input from
script\n"));
+ printf(_(" -b --echo-errors echo failed commands

sent

to
server\n"));
printf(_(" -e, --echo-queries echo commands sent to
server\n"));

Should have a comma after -b to match other options. Also I would
remove
"sent to server" from the description: "echo failed commands" is

fine.

fixed

$ psql -b
bin/psql: invalid option -- 'b'
Try "psql --help" for more information.

I got this error. ISTM you forgot to add 'b' into the third argument of
getopt_long in startup.c.

fixed

<application>psql</application> merely prints all queries as
they are sent to the server. The switch for this is
-        <option>-e</option>.
+        <option>-e</option>. If set to <literal>errors</literal> then
only
+        failed queries are displayed.

I think that where failed queries are output should be documented here.
Otherwise users might misunderstand they are output to standard output
like ECHO=all and queries do.

It's better to add "The switch for this is <option>-b</option>." into

the

doc.

fixed

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "ECHO") == 0)
+        {
+            static const char *const my_list[] =
+            {"none", "errors", "queries", "all", NULL};
+
+            COMPLETE_WITH_LIST_CS(my_list);
+        }
+        else if (strcmp(prev_wd, "ECHO_HIDDEN") == 0)
+        {
+            static const char *const my_list[] =
+            {"noexec", "off", "on", NULL};
+
+            COMPLETE_WITH_LIST_CS(my_list);
+        }
+    }

I think that adding tab-completions of psql variables is good, but
adding those of only ECHO and ECHO_HIDDEN seems half-baked.
Probably this part should be split into separate patch.

fixed

please, see updated patch in attachment

Thanks for updating the patch!

Barring any objection, I will commit this patch except tab-completion part.

Thank you

I'm not against adding tab-completion support for psql variables, but I'm
not sure if it's good idea or not to treat only one or two variables
special
and add tab-completions for them. There are other variables which accept
special argument (e.g., COMP_KEYWORD_CASE) and it's basically worth
adding tab-completion support for them.

It can be a second discussion, but I am thinking so anywhere when we can
use autocomplete, then we should it - Although it should not to substitute
documentation, it is fast help about available options, mainly in situation
where variables can hold a relative different content. I can prepare, if
there will not any objection addition patch with complete autocomplete for
all psql variables described in doc.

Regards

Pavel

On Wed, Jul 9, 2014 at 9:44 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Barring any objection, I will commit this patch except tab-completion
part.

Committed.

It can be a second discussion, but I am thinking so anywhere when we can use
autocomplete, then we should it - Although it should not to substitute
documentation, it is fast help about available options, mainly in situation
where variables can hold a relative different content. I can prepare, if
there will not any objection addition patch with complete autocomplete for
all psql variables described in doc.

I have no objection. But I found that the tab-completion for psql
variables names
are not complete. For example, COMP_KEYWORD_CASE is not displayed.
Probably we should address this problem, too.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-07-10 7:32 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Wed, Jul 9, 2014 at 9:44 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Barring any objection, I will commit this patch except tab-completion
part.

Committed.

Thank you very much

It can be a second discussion, but I am thinking so anywhere when we can

use

autocomplete, then we should it - Although it should not to substitute
documentation, it is fast help about available options, mainly in

situation

where variables can hold a relative different content. I can prepare, if
there will not any objection addition patch with complete autocomplete

for

all psql variables described in doc.

I have no objection. But I found that the tab-completion for psql
variables names
are not complete. For example, COMP_KEYWORD_CASE is not displayed.
Probably we should address this problem, too.

I prepare patch for next commitfest - it is relative trivial task

Pavel

Hello

here is a proposed patch - autocomplete for known psql variable content

Regards

Pavel

2014-07-10 9:50 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

On Thu, Jul 10, 2014 at 9:56 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hello

here is a proposed patch - autocomplete for known psql variable content

Even after applying the patch, the following psql variables were not displayed
on the tab-completion of \set command.

HISTFILE
HISTSIZE
HOST
IGNOREEOF
LASTOID

I'm not sure if it's worth displaying all of them on the tab-completion of \set
because it seems strange to change some of them by \set command, for example
HOST, though.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-07-15 11:29 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Jul 10, 2014 at 9:56 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hello

here is a proposed patch - autocomplete for known psql variable content

Even after applying the patch, the following psql variables were not
displayed
on the tab-completion of \set command.

HISTFILE
HISTSIZE
HOST
IGNOREEOF
LASTOID

I'm not sure if it's worth displaying all of them on the tab-completion of
\set
because it seems strange to change some of them by \set command, for
example
HOST, though.

For these variables are not default - so doesn't exist and cannot be
completed by default.

there are two fix:

a) fix autocomplete source - preferred
b) create default - but it is probably impossible

Regards

Pavel

On Tue, Jul 15, 2014 at 7:01 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2014-07-15 11:29 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Jul 10, 2014 at 9:56 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hello

here is a proposed patch - autocomplete for known psql variable content

Even after applying the patch, the following psql variables were not
displayed
on the tab-completion of \set command.

HISTFILE
HISTSIZE
HOST
IGNOREEOF
LASTOID

I'm not sure if it's worth displaying all of them on the tab-completion of
\set
because it seems strange to change some of them by \set command, for
example
HOST, though.

For these variables are not default - so doesn't exist and cannot be
completed by default.

there are two fix:

a) fix autocomplete source - preferred

+1

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-07-15 12:07 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Tue, Jul 15, 2014 at 7:01 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-07-15 11:29 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Jul 10, 2014 at 9:56 PM, Pavel Stehule <pavel.stehule@gmail.com

wrote:

Hello

here is a proposed patch - autocomplete for known psql variable

content

Even after applying the patch, the following psql variables were not
displayed
on the tab-completion of \set command.

HISTFILE
HISTSIZE
HOST
IGNOREEOF
LASTOID

I'm not sure if it's worth displaying all of them on the tab-completion

of

\set
because it seems strange to change some of them by \set command, for
example
HOST, though.

For these variables are not default - so doesn't exist and cannot be
completed by default.

there are two fix:

a) fix autocomplete source - preferred

+1

here is the patch

Regards

Pavel

On Wed, Jul 16, 2014 at 4:34 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2014-07-15 12:07 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Tue, Jul 15, 2014 at 7:01 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-07-15 11:29 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Jul 10, 2014 at 9:56 PM, Pavel Stehule
<pavel.stehule@gmail.com>
wrote:

Hello

here is a proposed patch - autocomplete for known psql variable
content

Even after applying the patch, the following psql variables were not
displayed
on the tab-completion of \set command.

HISTFILE
HISTSIZE
HOST
IGNOREEOF
LASTOID

I'm not sure if it's worth displaying all of them on the tab-completion
of
\set
because it seems strange to change some of them by \set command, for
example
HOST, though.

For these variables are not default - so doesn't exist and cannot be
completed by default.

there are two fix:

a) fix autocomplete source - preferred

+1

here is the patch

Thanks for the patch!

I got the following compiler warnings.

tab-complete.c:4155: warning: assignment discards qualifiers from
pointer target type
tab-complete.c:4166: warning: assignment discards qualifiers from
pointer target type

+ "FETCH_COUNT", "HISTCONTROL", "HISTFILE", "HISTSIZE", "HOST",
"IGNOREOFF",

Typo: IGNOREOFF -> IGNOREEOF

+    /* all psql known variables are included in list by default */
+    for (known_varname = known_varnames; *known_varname; known_varname++)
+        varnames[nvars++] = pg_strdup(*known_varname);

Don't we need to append both prefix and suffix to even known variables?

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "AUTOCOMMIT") == 0)

ISTM that some psql variables like IGNOREEOF are not there. Why not?

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello

updated version patch in attachment

2014-08-05 13:31 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Wed, Jul 16, 2014 at 4:34 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-07-15 12:07 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Tue, Jul 15, 2014 at 7:01 PM, Pavel Stehule <pavel.stehule@gmail.com

wrote:

2014-07-15 11:29 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Jul 10, 2014 at 9:56 PM, Pavel Stehule
<pavel.stehule@gmail.com>
wrote:

Hello

here is a proposed patch - autocomplete for known psql variable
content

Even after applying the patch, the following psql variables were not
displayed
on the tab-completion of \set command.

HISTFILE
HISTSIZE
HOST
IGNOREEOF
LASTOID

I'm not sure if it's worth displaying all of them on the

tab-completion

of
\set
because it seems strange to change some of them by \set command, for
example
HOST, though.

For these variables are not default - so doesn't exist and cannot be
completed by default.

there are two fix:

a) fix autocomplete source - preferred

+1

here is the patch

Thanks for the patch!

I got the following compiler warnings.

tab-complete.c:4155: warning: assignment discards qualifiers from
pointer target type
tab-complete.c:4166: warning: assignment discards qualifiers from
pointer target type

fixed

+ "FETCH_COUNT", "HISTCONTROL", "HISTFILE", "HISTSIZE", "HOST",
"IGNOREOFF",

Typo: IGNOREOFF -> IGNOREEOF

fixed

+    /* all psql known variables are included in list by default */
+    for (known_varname = known_varnames; *known_varname; known_varname++)
+        varnames[nvars++] = pg_strdup(*known_varname);

Don't we need to append both prefix and suffix to even known variables?

??? I am not sure if I understand well - probably system "read only"
variables as DBNAME, USER, VERSION should not be there

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "AUTOCOMMIT") == 0)

ISTM that some psql variables like IGNOREEOF are not there. Why not?

yes, there are not complete for DBNAME, ENCODING, FETCH_COUNT, HISTCONTROL,
HISTFILE, HISTSIZE, HOST, IGNOREEOFF, PROMPT*,USER, VERSION

There are more reasons:

* paremeter is not a enum (string, number or both): FETCH_COUNT, PROMPT,
HISTSIZE, ..

* variable is pseudo read only - change has not any effect: DBNAME,
ENCODING, VERSION

Regards

Pavel

On Thu, Aug 7, 2014 at 6:26 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hello

updated version patch in attachment

Thanks! But ISTM you forgot to attached the patch.

+    /* all psql known variables are included in list by default */
+    for (known_varname = known_varnames; *known_varname; known_varname++)
+        varnames[nvars++] = pg_strdup(*known_varname);

Don't we need to append both prefix and suffix to even known variables?

??? I am not sure if I understand well - probably system "read only"
variables as DBNAME, USER, VERSION should not be there

I had that question because complete_from_variables() is also called by the
tab-completion of "\echo :" and it shows half-baked variables list. So
I thought probably we need to change complete_from_variables() more to
fix the problem.

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "AUTOCOMMIT") == 0)

ISTM that some psql variables like IGNOREEOF are not there. Why not?

yes, there are not complete for DBNAME, ENCODING, FETCH_COUNT, HISTCONTROL,
HISTFILE, HISTSIZE, HOST, IGNOREEOFF, PROMPT*,USER, VERSION

There are more reasons:

* paremeter is not a enum (string, number or both): FETCH_COUNT, PROMPT,
HISTSIZE, ..

* variable is pseudo read only - change has not any effect: DBNAME,
ENCODING, VERSION

So HISTCONTROL should be there because it doesn't have such reasons at all?

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-08-07 7:10 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Aug 7, 2014 at 6:26 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hello

updated version patch in attachment

Thanks! But ISTM you forgot to attached the patch.

grr .. I am sorry

+    /* all psql known variables are included in list by default */
+    for (known_varname = known_varnames; *known_varname;

known_varname++)

+ varnames[nvars++] = pg_strdup(*known_varname);

Don't we need to append both prefix and suffix to even known variables?

??? I am not sure if I understand well - probably system "read only"
variables as DBNAME, USER, VERSION should not be there

I had that question because complete_from_variables() is also called by the
tab-completion of "\echo :" and it shows half-baked variables list. So
I thought probably we need to change complete_from_variables() more to
fix the problem.

I understand now.

I fixed it.

I have a question.\echo probably should not to show empty known variable.

data for autocomplete for \echo should be same as result of "\set"

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "AUTOCOMMIT") == 0)

ISTM that some psql variables like IGNOREEOF are not there. Why not?

yes, there are not complete for DBNAME, ENCODING, FETCH_COUNT,

HISTCONTROL,

HISTFILE, HISTSIZE, HOST, IGNOREEOFF, PROMPT*,USER, VERSION

There are more reasons:

* paremeter is not a enum (string, number or both): FETCH_COUNT, PROMPT,
HISTSIZE, ..

* variable is pseudo read only - change has not any effect: DBNAME,
ENCODING, VERSION

So HISTCONTROL should be there because it doesn't have such reasons at all?

yes

Pavel

On Fri, Aug 8, 2014 at 4:31 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2014-08-07 7:10 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Aug 7, 2014 at 6:26 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hello

updated version patch in attachment

Thanks! But ISTM you forgot to attached the patch.

grr .. I am sorry

No problem. Thanks for the patch! Attached is the revised version of the patch.

+    /* all psql known variables are included in list by default */
+    for (known_varname = known_varnames; *known_varname;
known_varname++)
+        varnames[nvars++] = pg_strdup(*known_varname);

Don't we need to append both prefix and suffix to even known variables?

??? I am not sure if I understand well - probably system "read only"
variables as DBNAME, USER, VERSION should not be there

I had that question because complete_from_variables() is also called by
the
tab-completion of "\echo :" and it shows half-baked variables list. So
I thought probably we need to change complete_from_variables() more to
fix the problem.

I understand now.

I fixed it.

I have a question.\echo probably should not to show empty known variable.

data for autocomplete for \echo should be same as result of "\set"

Agreed. I think that only the variables having the set values should be
displayed in "\echo :" case. So I modified complete_from_variables()
so that the unset variables are not shown in that case but all the variables
are shown in the tab-completion of "\set".

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "AUTOCOMMIT") == 0)

ISTM that some psql variables like IGNOREEOF are not there. Why not?

yes, there are not complete for DBNAME, ENCODING, FETCH_COUNT,
HISTCONTROL,
HISTFILE, HISTSIZE, HOST, IGNOREEOFF, PROMPT*,USER, VERSION

There are more reasons:

* paremeter is not a enum (string, number or both): FETCH_COUNT, PROMPT,
HISTSIZE, ..

* variable is pseudo read only - change has not any effect: DBNAME,
ENCODING, VERSION

So HISTCONTROL should be there because it doesn't have such reasons at
all?

yes

ISTM that you forgot to add HISTCONTROL to your patch. So I just added that.

I added the tab-completion for "\unset" command. Like "\echo :", only
the variables having the set values should be displayed in "\unset" case.

I changed complete_from_variables() so that it checks the memory size of
the variable array even when appending the known variables. If the memory
size is not enough, it's dynamically increased. Practically this check would
not be required because the initial size of the array is enough larger than
the number of the known variables. I added this as the safe-guard.

Typo: IGNOREEOFF -> IGNOREEOF

I removed the value "none" from the value list of "ECHO" because it's not
documented and a user might get confused when he or she sees the undocumented
value "none". Thought?

Regards,

--
Fujii Masao

Hi

2014-08-08 13:58 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Fri, Aug 8, 2014 at 4:31 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-08-07 7:10 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Aug 7, 2014 at 6:26 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hello

updated version patch in attachment

Thanks! But ISTM you forgot to attached the patch.

grr .. I am sorry

No problem. Thanks for the patch! Attached is the revised version of the
patch.

+    /* all psql known variables are included in list by default */
+    for (known_varname = known_varnames; *known_varname;
known_varname++)
+        varnames[nvars++] = pg_strdup(*known_varname);

Don't we need to append both prefix and suffix to even known

variables?

??? I am not sure if I understand well - probably system "read only"
variables as DBNAME, USER, VERSION should not be there

I had that question because complete_from_variables() is also called by
the
tab-completion of "\echo :" and it shows half-baked variables list. So
I thought probably we need to change complete_from_variables() more to
fix the problem.

I understand now.

I fixed it.

I have a question.\echo probably should not to show empty known variable.

data for autocomplete for \echo should be same as result of "\set"

Agreed. I think that only the variables having the set values should be
displayed in "\echo :" case. So I modified complete_from_variables()
so that the unset variables are not shown in that case but all the
variables
are shown in the tab-completion of "\set".

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "AUTOCOMMIT") == 0)

ISTM that some psql variables like IGNOREEOF are not there. Why not?

yes, there are not complete for DBNAME, ENCODING, FETCH_COUNT,
HISTCONTROL,
HISTFILE, HISTSIZE, HOST, IGNOREEOFF, PROMPT*,USER, VERSION

There are more reasons:

* paremeter is not a enum (string, number or both): FETCH_COUNT,

PROMPT,

HISTSIZE, ..

* variable is pseudo read only - change has not any effect: DBNAME,
ENCODING, VERSION

So HISTCONTROL should be there because it doesn't have such reasons at
all?

yes

ISTM that you forgot to add HISTCONTROL to your patch. So I just added
that.

I added the tab-completion for "\unset" command. Like "\echo :", only
the variables having the set values should be displayed in "\unset" case.

perfect

I changed complete_from_variables() so that it checks the memory size of
the variable array even when appending the known variables. If the memory
size is not enough, it's dynamically increased. Practically this check
would
not be required because the initial size of the array is enough larger than
the number of the known variables. I added this as the safe-guard.

Typo: IGNOREEOFF -> IGNOREEOF

I removed the value "none" from the value list of "ECHO" because it's not
documented and a user might get confused when he or she sees the
undocumented
value "none". Thought?

isn't better to fix doc? I don't know any reason why we should not to
support "none"

I looked to code, you removed a check against duplicate varname in list. Is
it ok?

Regards

Pavel

On Sat, Aug 9, 2014 at 4:16 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

2014-08-08 13:58 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Fri, Aug 8, 2014 at 4:31 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-08-07 7:10 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Aug 7, 2014 at 6:26 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hello

updated version patch in attachment

Thanks! But ISTM you forgot to attached the patch.

grr .. I am sorry

No problem. Thanks for the patch! Attached is the revised version of the
patch.

+    /* all psql known variables are included in list by default */
+    for (known_varname = known_varnames; *known_varname;
known_varname++)
+        varnames[nvars++] = pg_strdup(*known_varname);

Don't we need to append both prefix and suffix to even known
variables?

??? I am not sure if I understand well - probably system "read only"
variables as DBNAME, USER, VERSION should not be there

I had that question because complete_from_variables() is also called by
the
tab-completion of "\echo :" and it shows half-baked variables list. So
I thought probably we need to change complete_from_variables() more to
fix the problem.

I understand now.

I fixed it.

I have a question.\echo probably should not to show empty known
variable.

data for autocomplete for \echo should be same as result of "\set"

Agreed. I think that only the variables having the set values should be
displayed in "\echo :" case. So I modified complete_from_variables()
so that the unset variables are not shown in that case but all the
variables
are shown in the tab-completion of "\set".

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "AUTOCOMMIT") == 0)

ISTM that some psql variables like IGNOREEOF are not there. Why not?

yes, there are not complete for DBNAME, ENCODING, FETCH_COUNT,
HISTCONTROL,
HISTFILE, HISTSIZE, HOST, IGNOREEOFF, PROMPT*,USER, VERSION

There are more reasons:

* paremeter is not a enum (string, number or both): FETCH_COUNT,
PROMPT,
HISTSIZE, ..

* variable is pseudo read only - change has not any effect: DBNAME,
ENCODING, VERSION

So HISTCONTROL should be there because it doesn't have such reasons at
all?

yes

ISTM that you forgot to add HISTCONTROL to your patch. So I just added
that.

I added the tab-completion for "\unset" command. Like "\echo :", only
the variables having the set values should be displayed in "\unset" case.

perfect

I changed complete_from_variables() so that it checks the memory size of
the variable array even when appending the known variables. If the memory
size is not enough, it's dynamically increased. Practically this check
would
not be required because the initial size of the array is enough larger
than
the number of the known variables. I added this as the safe-guard.

Typo: IGNOREEOFF -> IGNOREEOF

I removed the value "none" from the value list of "ECHO" because it's not
documented and a user might get confused when he or she sees the
undocumented
value "none". Thought?

isn't better to fix doc? I don't know any reason why we should not to
support "none"

I'm OK with this. The attached patch adds the support of "none" value both
in ECHO and HISTCONTROL variables (because HISTCONTROL had the same
problem as ECHO had), and also adds the description of that value into
the document.

I looked to code, you removed a check against duplicate varname in list. Is
it ok?

Oh, just revived that code.

Regards,

--
Fujii Masao

2014-08-11 14:59 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Sat, Aug 9, 2014 at 4:16 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

2014-08-08 13:58 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Fri, Aug 8, 2014 at 4:31 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-08-07 7:10 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Aug 7, 2014 at 6:26 AM, Pavel Stehule <

pavel.stehule@gmail.com>

wrote:

Hello

updated version patch in attachment

Thanks! But ISTM you forgot to attached the patch.

grr .. I am sorry

No problem. Thanks for the patch! Attached is the revised version of the
patch.

+ /* all psql known variables are included in list by default

*/

+    for (known_varname = known_varnames; *known_varname;
known_varname++)
+        varnames[nvars++] = pg_strdup(*known_varname);

Don't we need to append both prefix and suffix to even known
variables?

??? I am not sure if I understand well - probably system "read

only"

variables as DBNAME, USER, VERSION should not be there

I had that question because complete_from_variables() is also called

by

the
tab-completion of "\echo :" and it shows half-baked variables list.

So

I thought probably we need to change complete_from_variables() more

to

fix the problem.

I understand now.

I fixed it.

I have a question.\echo probably should not to show empty known
variable.

data for autocomplete for \echo should be same as result of "\set"

Agreed. I think that only the variables having the set values should be
displayed in "\echo :" case. So I modified complete_from_variables()
so that the unset variables are not shown in that case but all the
variables
are shown in the tab-completion of "\set".

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "AUTOCOMMIT") == 0)

ISTM that some psql variables like IGNOREEOF are not there. Why

not?

yes, there are not complete for DBNAME, ENCODING, FETCH_COUNT,
HISTCONTROL,
HISTFILE, HISTSIZE, HOST, IGNOREEOFF, PROMPT*,USER, VERSION

There are more reasons:

* paremeter is not a enum (string, number or both): FETCH_COUNT,
PROMPT,
HISTSIZE, ..

* variable is pseudo read only - change has not any effect:

DBNAME,

ENCODING, VERSION

So HISTCONTROL should be there because it doesn't have such reasons

at

all?

yes

ISTM that you forgot to add HISTCONTROL to your patch. So I just added
that.

I added the tab-completion for "\unset" command. Like "\echo :", only
the variables having the set values should be displayed in "\unset"

case.

perfect

I changed complete_from_variables() so that it checks the memory size of
the variable array even when appending the known variables. If the

memory

size is not enough, it's dynamically increased. Practically this check
would
not be required because the initial size of the array is enough larger
than
the number of the known variables. I added this as the safe-guard.

Typo: IGNOREEOFF -> IGNOREEOF

I removed the value "none" from the value list of "ECHO" because it's

not

documented and a user might get confused when he or she sees the
undocumented
value "none". Thought?

isn't better to fix doc? I don't know any reason why we should not to
support "none"

I'm OK with this. The attached patch adds the support of "none" value both
in ECHO and HISTCONTROL variables (because HISTCONTROL had the same
problem as ECHO had), and also adds the description of that value into
the document.

I looked to code, you removed a check against duplicate varname in list.

Is

it ok?

Oh, just revived that code.

yes, It is looking well

regards

Pavel

On Tue, Aug 12, 2014 at 4:41 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2014-08-11 14:59 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Sat, Aug 9, 2014 at 4:16 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

2014-08-08 13:58 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Fri, Aug 8, 2014 at 4:31 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2014-08-07 7:10 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Thu, Aug 7, 2014 at 6:26 AM, Pavel Stehule
<pavel.stehule@gmail.com>
wrote:

Hello

updated version patch in attachment

Thanks! But ISTM you forgot to attached the patch.

grr .. I am sorry

No problem. Thanks for the patch! Attached is the revised version of
the
patch.

+    /* all psql known variables are included in list by default
*/
+    for (known_varname = known_varnames; *known_varname;
known_varname++)
+        varnames[nvars++] = pg_strdup(*known_varname);

Don't we need to append both prefix and suffix to even known
variables?

??? I am not sure if I understand well - probably system "read
only"
variables as DBNAME, USER, VERSION should not be there

I had that question because complete_from_variables() is also called
by
the
tab-completion of "\echo :" and it shows half-baked variables list.
So
I thought probably we need to change complete_from_variables() more
to
fix the problem.

I understand now.

I fixed it.

I have a question.\echo probably should not to show empty known
variable.

data for autocomplete for \echo should be same as result of "\set"

Agreed. I think that only the variables having the set values should be
displayed in "\echo :" case. So I modified complete_from_variables()
so that the unset variables are not shown in that case but all the
variables
are shown in the tab-completion of "\set".

+    else if (strcmp(prev2_wd, "\\set") == 0)
+    {
+        if (strcmp(prev_wd, "AUTOCOMMIT") == 0)

ISTM that some psql variables like IGNOREEOF are not there. Why
not?

yes, there are not complete for DBNAME, ENCODING, FETCH_COUNT,
HISTCONTROL,
HISTFILE, HISTSIZE, HOST, IGNOREEOFF, PROMPT*,USER, VERSION

There are more reasons:

* paremeter is not a enum (string, number or both): FETCH_COUNT,
PROMPT,
HISTSIZE, ..

* variable is pseudo read only - change has not any effect:
DBNAME,
ENCODING, VERSION

So HISTCONTROL should be there because it doesn't have such reasons
at
all?

yes

ISTM that you forgot to add HISTCONTROL to your patch. So I just added
that.

I added the tab-completion for "\unset" command. Like "\echo :", only
the variables having the set values should be displayed in "\unset"
case.

perfect

I changed complete_from_variables() so that it checks the memory size
of
the variable array even when appending the known variables. If the
memory
size is not enough, it's dynamically increased. Practically this check
would
not be required because the initial size of the array is enough larger
than
the number of the known variables. I added this as the safe-guard.

Typo: IGNOREEOFF -> IGNOREEOF

I removed the value "none" from the value list of "ECHO" because it's
not
documented and a user might get confused when he or she sees the
undocumented
value "none". Thought?

isn't better to fix doc? I don't know any reason why we should not to
support "none"

I'm OK with this. The attached patch adds the support of "none" value both
in ECHO and HISTCONTROL variables (because HISTCONTROL had the same
problem as ECHO had), and also adds the description of that value into
the document.

I looked to code, you removed a check against duplicate varname in list.
Is
it ok?

Oh, just revived that code.

yes, It is looking well

Ok, committed!

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Oh, just revived that code.

yes, It is looking well

Ok, committed!

super

thank you very much

Regards

Pavel