Re: Support tab completion for upper character inputs in psql
"tanghy.f...@fujitsu.com" writes: > I think 02b8048 forgot to free some used memory. > Attached a tiny patch to fix it. Please have a check. Right you are. Inspired by that, I tried running some tab-completion operations under valgrind, and found another nearby leak in patternToSQLRegex. Fixes pushed. regards, tom lane
RE: Support tab completion for upper character inputs in psql
On Monday, January 31, 2022 3:35 AM, Tom Lane wrote: > "tanghy.f...@fujitsu.com" writes: > > Thanks for your V16 patch, I tested it. > > The results LGTM. > > Pushed, thanks for looking. I think 02b8048 forgot to free some used memory. Attached a tiny patch to fix it. Please have a check. Regards, Tang v1-0001-fix-memory-leak-in-tab-completion.c.patch Description: v1-0001-fix-memory-leak-in-tab-completion.c.patch
Re: Support tab completion for upper character inputs in psql
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= writes: > Tom Lane writes: >>> We could do something hacky like matching case only when there's >>> no longer any matching object names, but that might be too magic. >> I experimented with that, and it actually doesn't seem as weird >> as I feared. See if you like this ... > That's a reasonable compromise, and the implementation is indeed less > hacky than one might have feared. Although I think putting the > `num_keywords` variable before `num_other` would read better. After a few days of using that, I'm having second thoughts about it, because it turns out to impede completion in common cases. For example, regression=# set transa TRANSACTION transaction_isolation transaction_deferrable transaction_read_only It won't fill in "ction" because of the case discrepancy between the offered alternatives. Maybe this trumps the question of whether you should be able to distinguish keywords from non-keywords in the menus. If we case-folded the keywords as per your original proposal, it'd do what I expect it to. In previous releases, this worked as expected: "set transa" immediately completes "ction", and then tabbing produces this menu: transaction transaction_isolation transaction_deferrable transaction_read_only That probably explains why these keywords were lower-cased in the previous code. However, I don't think we should blame your suggestion to upcase them, because the same problem arises in other completion contexts where we offer keywords. We should solve it across-the-board not just for these specific queries. regards, tom lane
Re: Support tab completion for upper character inputs in psql
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= writes: > Tom Lane writes: >>> We could do something hacky like matching case only when there's >>> no longer any matching object names, but that might be too magic. >> I experimented with that, and it actually doesn't seem as weird >> as I feared. See if you like this ... > That's a reasonable compromise, and the implementation is indeed less > hacky than one might have feared. Although I think putting the > `num_keywords` variable before `num_other` would read better. Hm... I renamed "num_other" to "num_query_other" instead. > Going through the uses of COMPLETE_WITH(_SCHEMA)_QUERY_PLUS, I noticed a > few that had the keywords in lower case, which is fixed in the attached > patch (except the hardcoded data types, which aren't really keywords). Yeah, my oversight. Pushed. regards, tom lane
Re: Support tab completion for upper character inputs in psql
Tom Lane writes:
> I wrote:
>> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= writes:
>>> First, as noted in the test, it doesn't preserve the case of the input
>>> for keywords appended to the query result. This is easily fixed by
>>> using `pg_strdup_keyword_case()`, per the first attached patch.
>
>> I thought about that, and intentionally didn't do it, because it
>> would also affect the menus produced by tab completion.
>> ...
>> We could do something hacky like matching case only when there's
>> no longer any matching object names, but that might be too magic.
>
> I experimented with that, and it actually doesn't seem as weird
> as I feared. See if you like this ...
That's a reasonable compromise, and the implementation is indeed less
hacky than one might have feared. Although I think putting the
`num_keywords` variable before `num_other` would read better.
Going through the uses of COMPLETE_WITH(_SCHEMA)_QUERY_PLUS, I noticed a
few that had the keywords in lower case, which is fixed in the attached
patch (except the hardcoded data types, which aren't really keywords).
While I was there, I also added completion of "AUTHORIZATION" after
"SHOW SESSSION", which is necessary since there are variables starting
with "session_".
> regards, tom lane
Cheers,
- ilmari
>From 9cc255e0cdddeda3d655c1265d698b1f6027aec6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?=
Date: Tue, 1 Feb 2022 13:13:07 +
Subject: [PATCH] Make all tab completion keywords upper case.
So they stand out from the object names in the same tab completion
menu.
Also fix tab completion of SHOW SESSION AUTHORIZATION in the face of
config variables starting with session_.
---
src/bin/psql/tab-complete.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b2ec50b4f2..d2744cdb6f 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2059,7 +2059,7 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH("SET", "RESET");
else if (Matches("ALTER", "SYSTEM", "SET|RESET"))
COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_alter_system_set_vars,
- "all");
+ "ALL");
else if (Matches("ALTER", "SYSTEM", "SET", MatchAny))
COMPLETE_WITH("TO");
/* ALTER VIEW */
@@ -4039,16 +4039,18 @@ psql_completion(const char *text, int start, int end)
/* Complete with a variable name */
else if (TailMatches("SET|RESET") && !TailMatches("UPDATE", MatchAny, "SET"))
COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_set_vars,
- "constraints",
- "transaction",
- "session",
- "role",
- "tablespace",
- "all");
+ "CONSTRAINTS",
+ "TRANSACTION",
+ "SESSION",
+ "ROLE",
+ "TABLESPACE",
+ "ALL");
else if (Matches("SHOW"))
COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_show_vars,
- "session authorization",
- "all");
+ "SESSION AUTHORIZATION",
+ "ALL");
+ else if (Matches("SHOW", "SESSION"))
+ COMPLETE_WITH("AUTHORIZATION");
/* Complete "SET TRANSACTION" */
else if (Matches("SET", "TRANSACTION"))
COMPLETE_WITH("SNAPSHOT", "ISOLATION LEVEL", "READ", "DEFERRABLE", "NOT DEFERRABLE");
--
2.30.2
Re: Support tab completion for upper character inputs in psql
I wrote:
> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= writes:
>> First, as noted in the test, it doesn't preserve the case of the input
>> for keywords appended to the query result. This is easily fixed by
>> using `pg_strdup_keyword_case()`, per the first attached patch.
> I thought about that, and intentionally didn't do it, because it
> would also affect the menus produced by tab completion.
> ...
> We could do something hacky like matching case only when there's
> no longer any matching object names, but that might be too magic.
I experimented with that, and it actually doesn't seem as weird
as I feared. See if you like this ...
regards, tom lane
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index c4f6552ac9..7a265e0676 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -40,7 +40,7 @@ $node->start;
# set up a few database objects
$node->safe_psql('postgres',
- "CREATE TABLE tab1 (f1 int primary key, f2 text);\n"
+ "CREATE TABLE tab1 (c1 int primary key, c2 text);\n"
. "CREATE TABLE mytab123 (f1 int, f2 text);\n"
. "CREATE TABLE mytab246 (f1 int, f2 text);\n"
. "CREATE TABLE \"mixedName\" (f1 int, f2 text);\n"
@@ -317,14 +317,30 @@ check_completion(
clear_line();
-# check completion of a keyword offered in addition to object names
-# (that code path currently doesn't preserve case of what's typed)
-check_completion(
- "comment on constraint foo on dom\t",
- qr|DOMAIN|,
- "offer keyword in addition to query result");
-
-clear_query();
+# check completion of a keyword offered in addition to object names;
+# such a keyword should obey COMP_KEYWORD_CASE once only keyword
+# completions are possible
+foreach (
+ [ 'lower', 'CO', 'column' ],
+ [ 'upper', 'co', 'COLUMN' ],
+ [ 'preserve-lower', 'co', 'column' ],
+ [ 'preserve-upper', 'CO', 'COLUMN' ],)
+{
+ my ($case, $in, $out) = @$_;
+
+ check_completion(
+ "\\set COMP_KEYWORD_CASE $case\n",
+ qr/postgres=#/,
+ "set completion case to '$case'");
+ check_completion("alter table tab1 rename c\t\t",
+ qr|COLUMN|,
+ "offer keyword COLUMN for input c, COMP_KEYWORD_CASE = $case");
+ clear_query();
+ check_completion("alter table tab1 rename $in\t\t\t",
+ qr|$out|,
+ "offer keyword $out for input $in, COMP_KEYWORD_CASE = $case");
+ clear_query();
+}
# send psql an explicit \q to shut it down, else pty won't close properly
$timer->start(5);
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b2ec50b4f2..bdc9760fba 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4742,7 +4742,8 @@ _complete_from_query(const char *simple_query,
{
static int list_index,
num_schema_only,
-num_other;
+num_other,
+num_keywords;
static PGresult *result = NULL;
static bool non_empty_object;
static bool schemaquoted;
@@ -4766,6 +4767,7 @@ _complete_from_query(const char *simple_query,
list_index = 0;
num_schema_only = 0;
num_other = 0;
+ num_keywords = 0;
PQclear(result);
result = NULL;
@@ -4986,7 +4988,10 @@ _complete_from_query(const char *simple_query,
/* In verbatim mode, we return all the items as-is */
if (verbatim)
+ {
+num_other++;
return pg_strdup(item);
+ }
/*
* In normal mode, a name requiring quoting will be returned only
@@ -5031,8 +5036,12 @@ _complete_from_query(const char *simple_query,
list_index++;
if (pg_strncasecmp(text, item, strlen(text)) == 0)
{
- num_other++;
- return pg_strdup(item);
+ num_keywords++;
+ /* Match keyword case if we are returning only keywords */
+ if (num_schema_only == 0 && num_other == 0)
+ return pg_strdup_keyword_case(item, text);
+ else
+ return pg_strdup(item);
}
}
}
@@ -5049,8 +5058,12 @@ _complete_from_query(const char *simple_query,
list_index++;
if (pg_strncasecmp(text, item, strlen(text)) == 0)
{
- num_other++;
- return pg_strdup(item);
+ num_keywords++;
+ /* Match keyword case if we are returning only keywords */
+ if (num_schema_only == 0 && num_other == 0)
+ return pg_strdup_keyword_case(item, text);
+ else
+ return pg_strdup(item);
}
}
}
@@ -5062,7 +5075,7 @@ _complete_from_query(const char *simple_query,
* completion subject text, which is not what we want.
*/
#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
- if (num_schema_only > 0 && num_other == 0)
+ if (num_schema_only > 0 && num_other == 0 && num_keywords == 0)
rl_completion_append_character = '\0';
#endif
Re: Support tab completion for upper character inputs in psql
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= writes: > First, as noted in the test, it doesn't preserve the case of the input > for keywords appended to the query result. This is easily fixed by > using `pg_strdup_keyword_case()`, per the first attached patch. I thought about that, and intentionally didn't do it, because it would also affect the menus produced by tab completion. Currently, keywords are (usually) visually distinct from non-keywords in those menus, thanks to being upper-case where the object names usually aren't: regression=# create table foo (c1 int, c2 int); CREATE TABLE regression=# alter table foo rename c c1 c2 COLUMN CONSTRAINT With this change, the keywords would be visually indistinguishable from the object names, which I felt wouldn't be a net improvement. We could do something hacky like matching case only when there's no longer any matching object names, but that might be too magic. > The second might be more of a matter of style or opinion, but I noticed > a bunch of `if (foo) free(foo);`, which is redundant given that > `free(NULL)` is a no-op. To simplify the code further, I also made > `escape_string(NULL)` be a no-op, returning `NULL`. Yeah. Our fairly longstanding convention is to avoid doing free(NULL), dating back to when some platforms would crash on it. I realize that's archaic now, but I'm not inclined to change it in just some places. regards, tom lane
Re: Support tab completion for upper character inputs in psql
Tom Lane writes:
> "tanghy.f...@fujitsu.com" writes:
>> Thanks for your V16 patch, I tested it.
>> The results LGTM.
>
> Pushed, thanks for looking.
I wasn't following this thread, but I noticed a few small potential
improvements when I saw the commit.
First, as noted in the test, it doesn't preserve the case of the input
for keywords appended to the query result. This is easily fixed by
using `pg_strdup_keyword_case()`, per the first attached patch.
The second might be more of a matter of style or opinion, but I noticed
a bunch of `if (foo) free(foo);`, which is redundant given that
`free(NULL)` is a no-op. To simplify the code further, I also made
`escape_string(NULL)` be a no-op, returning `NULL`.
- ilmari
>From 7b278ea5c48d386e1c6f277f3d94520505205cd2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?=
Date: Mon, 31 Jan 2022 00:42:00 +
Subject: [PATCH 1/2] Respect COMP_KEYWORD_CASE for additional keywords
When tacking on keywords to the result of a completion query, use
pg_strdup_keyword_case().
---
src/bin/psql/t/010_tab_completion.pl | 26 --
src/bin/psql/tab-complete.c | 4 ++--
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index c4f6552ac9..c4a1d5e1bc 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -317,12 +317,26 @@ sub clear_line
clear_line();
-# check completion of a keyword offered in addition to object names
-# (that code path currently doesn't preserve case of what's typed)
-check_completion(
- "comment on constraint foo on dom\t",
- qr|DOMAIN|,
- "offer keyword in addition to query result");
+foreach (
+ ['lower', 'DOM', 'domain'],
+ ['upper', 'dom', 'DOMAIN'],
+ ['preserve-lower', 'dom', 'domain'],
+ ['preserve-upper', 'DOM', 'DOMAIN'],
+) {
+ my ($case, $in, $out) = @$_;
+
+ # check completion of a keyword offered in addition to object names
+ # and obeys COMP_KEYWORD_CASE
+ check_completion(
+ "\\set COMP_KEYWORD_CASE $case\n",
+ qr/postgres=#/,
+ "set completion case to '$case'");
+ check_completion(
+ "comment on constraint foo on $in\t",
+ qr|$out|,
+ "offer keyword in addition to query result (COMP_KEYWORD_CASE=$case)");
+ clear_line();
+}
clear_query();
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b2ec50b4f2..0c7a99be0a 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -5032,7 +5032,7 @@ _complete_from_query(const char *simple_query,
if (pg_strncasecmp(text, item, strlen(text)) == 0)
{
num_other++;
- return pg_strdup(item);
+ return pg_strdup_keyword_case(item, text);
}
}
}
@@ -5050,7 +5050,7 @@ _complete_from_query(const char *simple_query,
if (pg_strncasecmp(text, item, strlen(text)) == 0)
{
num_other++;
- return pg_strdup(item);
+ return pg_strdup_keyword_case(item, text);
}
}
}
--
2.30.2
>From f4d38e9a819fa8650bea201784511654ec1d858e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?=
Date: Mon, 31 Jan 2022 00:09:14 +
Subject: [PATCH 2/2] psql tab-complete: remove unnecessary null checks
free(NULL) is specified to be a no-op, so no need to check before
calling it. Also make escape_string(NULL) a no-op to match.
---
src/bin/psql/tab-complete.c | 37 -
1 file changed, 12 insertions(+), 25 deletions(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 0c7a99be0a..5115f02934 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4555,11 +4555,9 @@ psql_completion(const char *text, int start, int end)
free(previous_words);
free(words_buffer);
free(text_copy);
- if (completion_ref_object)
- free(completion_ref_object);
+ free(completion_ref_object);
completion_ref_object = NULL;
- if (completion_ref_schema)
- free(completion_ref_schema);
+ free(completion_ref_schema);
completion_ref_schema = NULL;
/* Return our Grand List O' Matches */
@@ -4790,21 +4788,9 @@ _complete_from_query(const char *simple_query,
* up suitably-escaped copies of all the strings we need.
*/
e_object_like = make_like_pattern(objectname);
-
- if (schemaname)
- e_schemaname = escape_string(schemaname);
- else
- e_schemaname = NULL;
-
- if (completion_ref_object)
- e_ref_object = escape_string(completion_ref_object);
- else
- e_ref_object = NULL;
-
- if (completion_ref_schema)
- e_ref_schema = escape_string(completion_ref_schema);
- else
- e_ref_schema = NULL;
+ e_schemaname = escape_string(schemaname);
+ e_ref_object = escape_string(completion_ref_object);
+ e_ref_schema = escape_string(completion_ref_schema);
initPQExpBuffer(&query_buffer);
@@ -4959,12 +4945,9 @@ _complete_from_query(const char *simple_query,
/* Clean up */
termPQExpBuffer(&query_buffer);
free(e_object_li
Re: Support tab completion for upper character inputs in psql
"tanghy.f...@fujitsu.com" writes: > Thanks for your V16 patch, I tested it. > The results LGTM. Pushed, thanks for looking. regards, tom lane
RE: Support tab completion for upper character inputs in psql
On Saturday, January 29, 2022 7:17 AM, Tom Lane wrote: > Sigh ... per the cfbot, this was already blindsided by 95787e849. > As I said, I don't want to sit on this for very long. Thanks for your V16 patch, I tested it. The results LGTM. Regards, Tang
RE: Support tab completion for upper character inputs in psql
On Saturday, January 29, 2022 1:03 AM, Tom Lane wrote: > "tanghy.f...@fujitsu.com" writes: > > I did some tests on it and here are something cases I feel we need to > > confirm > > whether they are suitable. > > > 1) postgres=# create table atest(id int, "iD" int, "ID" int); > > 2) CREATE TABLE > > 3) postgres=# alter table atest rename i[TAB] > > 4) id"iD" > > 5) postgres=# alter table atest rename I[TAB] > > 6) id"iD" > > > The tab completion for 5) ignored "ID", is that correct? > > Perhaps I misunderstood your original complaint, but what I thought > you were unhappy about was that unquoted ID is a legal spelling of > "id" and so I ought to be willing to complete that. These > examples with case variants of the same word are of some interest, > but people aren't really going to create tables with these sorts of > names, so we shouldn't let them drive the design IMO. > > Anyway, the existing behavior for these examples is > > alter table atest rename i --- completes immediately to id > alter table atest rename I --- offers nothing > > It's certainly arguable that the first case is right as-is and we > shouldn't change it. I think that could be handled by tweaking my > patch so that it wouldn't offer completions that start with a quote > unless the input word does. That would also cause I to complete > immediately to id, which is arguably fine. > > > I think what we are trying to do is to ease the burden of typing double > > quote > for user. > > I'm not thinking about it that way at all. To me, the goal is to make > tab completion do something sensible when presented with legal variant > spellings of a word. The two cases where it currently fails to do > that are (1) unquoted input that needs to be downcased, and (2) input > that is quoted when it doesn't strictly need to be. > > To the extent that we can supply a required quote that the user > failed to type, that's fine, but it's not a primary goal of the patch. > Examples like these make me question whether it's even something we > want; it's resulting in extraneous matches that people might find more > annoying than helpful. Now I *think* that these aren't realistic > cases and that in real cases adding quotes will be helpful more often > than not, but it's debatable. > > > One the other hand, I'm not so comfortable with the output of "iD" in line > 13. > > If user doesn't type double quote, why we add double quote to the output? > > That's certainly a valid argument. > > > Could we make the output of 13) like below? > > 12) postgres=# alter table atest rename i[TAB] > > ??) id iD > > That doesn't seem sensible at all. Thanks for your kindly explanation. I'm fine with the current tap completion style with your V16 patch. Regards, Tang
Re: Support tab completion for upper character inputs in psql
"tanghy.f...@fujitsu.com" writes: > I did some tests on it and here are something cases I feel we need to confirm > whether they are suitable. > 1) postgres=# create table atest(id int, "iD" int, "ID" int); > 2) CREATE TABLE > 3) postgres=# alter table atest rename i[TAB] > 4) id"iD" > 5) postgres=# alter table atest rename I[TAB] > 6) id"iD" > The tab completion for 5) ignored "ID", is that correct? Perhaps I misunderstood your original complaint, but what I thought you were unhappy about was that unquoted ID is a legal spelling of "id" and so I ought to be willing to complete that. These examples with case variants of the same word are of some interest, but people aren't really going to create tables with these sorts of names, so we shouldn't let them drive the design IMO. Anyway, the existing behavior for these examples is alter table atest rename i --- completes immediately to id alter table atest rename I --- offers nothing It's certainly arguable that the first case is right as-is and we shouldn't change it. I think that could be handled by tweaking my patch so that it wouldn't offer completions that start with a quote unless the input word does. That would also cause I to complete immediately to id, which is arguably fine. > I think what we are trying to do is to ease the burden of typing double quote > for user. I'm not thinking about it that way at all. To me, the goal is to make tab completion do something sensible when presented with legal variant spellings of a word. The two cases where it currently fails to do that are (1) unquoted input that needs to be downcased, and (2) input that is quoted when it doesn't strictly need to be. To the extent that we can supply a required quote that the user failed to type, that's fine, but it's not a primary goal of the patch. Examples like these make me question whether it's even something we want; it's resulting in extraneous matches that people might find more annoying than helpful. Now I *think* that these aren't realistic cases and that in real cases adding quotes will be helpful more often than not, but it's debatable. > One the other hand, I'm not so comfortable with the output of "iD" in line 13. > If user doesn't type double quote, why we add double quote to the output? That's certainly a valid argument. > Could we make the output of 13) like below? > 12) postgres=# alter table atest rename i[TAB] > ??) id iD That doesn't seem sensible at all. regards, tom lane
RE: Support tab completion for upper character inputs in psql
On Friday, January 28, 2022 5:24 AM, Tom Lane wrote:
> Here's a fleshed-out patch series for this idea.
Thanks for you patch.
I did some tests on it and here are something cases I feel we need to confirm
whether they are suitable.
1) postgres=# create table atest(id int, "iD" int, "ID" int);
2) CREATE TABLE
3) postgres=# alter table atest rename i[TAB]
4) id"iD"
5) postgres=# alter table atest rename I[TAB]
6) id"iD"
The tab completion for 5) ignored "ID", is that correct?
7) postgres=# create table "aTest"("myCol" int, mycol int);
8) CREATE TABLE
9) postgres=# alter table a[TAB]
10) ALL IN TABLESPACE atest "aTest"
11) postgres=# alter table aT[TAB] -> atest
I think what we are trying to do is to ease the burden of typing double quote
for user.
But in line 11), the tab completion for "alter table aT[TAB]" is attest,
which makes the tab completion output of "aTest" at 10) no value.
Because if user needs to alter table aTest they still needs to
type double quote manually.
Another thing is the inconsistency of the output result.
12) postgres=# alter table atest rename i[TAB]
13) id"iD"
14) postgres=# alter table atest rename "i[TAB]
15) "id" "iD"
By applying the new fix, Line 15 added the output of "id".
I think it's good to keep user input '"' and convenient when using tab
completion.
One the other hand, I'm not so comfortable with the output of "iD" in line 13.
If user doesn't type double quote, why we add double quote to the output?
Could we make the output of 13) like below?
12) postgres=# alter table atest rename i[TAB]
??) id iD
Regards,
Tang
RE: Support tab completion for upper character inputs in psql
On Tuesday, January 25, 2022 6:44 PM, Julien Rouhaud wrote: > Thanks for updating the patch. When you do so, please check and update the > commitfest entry accordingly to make sure that people knows it's ready for > review. I'm switching the entry to Needs Review. > Thanks for your reminder. I'll watch out the status change as you suggested. Regards, Tang
Re: Support tab completion for upper character inputs in psql
I spent some time contemplating my navel about the concerns I raised
upthread about double-quoted identifiers. I concluded that the reason
things don't work well in that area is that we're trying to get all the
work done by applying quote_ident() on the backend side and then
ignoring quoting considerations in tab-complete itself. That sort of
works, but not terribly well. The currently proposed patch is sticking
a toe into the water of dealing with quoting/downcasing in tab-complete,
but we need to go a lot further. I propose that we ought to drop the
use of quote_ident() in the tab completion queries altogether, instead
having the backend return names as-is, and doing all the dequoting and
requoting work in tab-complete.
Attached is a very-much-WIP patch along these lines. I make no
pretense that it's complete; no doubt some of the individual
queries are broken or don't return quite the results we want.
But it seems to act the way I think it should for relation names.
One thing I'm particularly unsure what to do with is the queries
for type names, which want to match against the output of
format_type, which'll already have applied quote_ident. We can
probably hack something up there, but I ran out of time to mess
with that for today.
Anyway, I wanted to post this just to see what people think of
going in this direction.
regards, tom lane
PS: I omitted the proposed regression test changes here.
Many of them are not at all portable --- different versions
of readline/libedit will produce different control character
sequences for backspacing, for example. I got a lot of
failures when I tried to use those tests with this patch;
I've not run down which ones are test portability problems,
which are due to intentional behavior changes in this patch,
and which are due to breakage I've not fixed yet.
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 502b5c5751..2dadf7d945 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -47,6 +47,7 @@
#include "catalog/pg_class_d.h"
#include "common.h"
#include "libpq-fe.h"
+#include "mb/pg_wchar.h"
#include "pqexpbuffer.h"
#include "settings.h"
#include "stringutils.h"
@@ -148,8 +149,8 @@ typedef struct SchemaQuery
const char *namespace;
/*
- * Result --- the appropriately-quoted name to return, in the case of an
- * unqualified name. For example, "pg_catalog.quote_ident(c.relname)".
+ * Result --- the (unquoted) name to return, in the case of an unqualified
+ * name. For example, "c.relname".
*/
const char *result;
@@ -315,7 +316,7 @@ do { \
completion_info_charp = _completion_type; \
completion_info_charp2 = _completion_schema; \
} \
- matches = rl_completion_matches(text, complete_from_query); \
+ matches = rl_completion_matches(text, complete_from_query_verbatim); \
} while (0)
#define COMPLETE_WITH_FUNCTION_ARG(function) \
@@ -357,14 +358,14 @@ static const SchemaQuery Query_for_list_of_aggregates[] = {
.selcondition = "p.prokind = 'a'",
.viscondition = "pg_catalog.pg_function_is_visible(p.oid)",
.namespace = "p.pronamespace",
- .result = "pg_catalog.quote_ident(p.proname)",
+ .result = "p.proname",
},
{
.catname = "pg_catalog.pg_proc p",
.selcondition = "p.proisagg",
.viscondition = "pg_catalog.pg_function_is_visible(p.oid)",
.namespace = "p.pronamespace",
- .result = "pg_catalog.quote_ident(p.proname)",
+ .result = "p.proname",
}
};
@@ -378,7 +379,7 @@ static const SchemaQuery Query_for_list_of_datatypes = {
.viscondition = "pg_catalog.pg_type_is_visible(t.oid)",
.namespace = "t.typnamespace",
.result = "pg_catalog.format_type(t.oid, NULL)",
- .qualresult = "pg_catalog.quote_ident(t.typname)",
+ .qualresult = "t.typname",
};
static const SchemaQuery Query_for_list_of_composite_datatypes = {
@@ -390,7 +391,7 @@ static const SchemaQuery Query_for_list_of_composite_datatypes = {
.viscondition = "pg_catalog.pg_type_is_visible(t.oid)",
.namespace = "t.typnamespace",
.result = "pg_catalog.format_type(t.oid, NULL)",
- .qualresult = "pg_catalog.quote_ident(t.typname)",
+ .qualresult = "t.typname",
};
static const SchemaQuery Query_for_list_of_domains = {
@@ -398,7 +399,7 @@ static const SchemaQuery Query_for_list_of_domains = {
.selcondition = "t.typtype = 'd'",
.viscondition = "pg_catalog.pg_type_is_visible(t.oid)",
.namespace = "t.typnamespace",
- .result = "pg_catalog.quote_ident(t.typname)",
+ .result = "t.typname",
};
/* Note: this intentionally accepts aggregates as well as plain functions */
@@ -409,13 +410,13 @@ static const SchemaQuery Query_for_list_of_functions[] = {
.selcondition = "p.prokind != 'p'",
.viscondition = "pg_catalog.pg_function_is_visible(p.oid)",
.namespace = "p.pronamespace",
- .result = "pg_catalog.quote_ident(p.proname)",
+ .result = "p.proname",
},
{
.catname = "pg_catalog.pg_proc p",
.viscondition = "pg_catalog.pg_function_is_visible(p.oid)
Re: Support tab completion for upper character inputs in psql
Hi, On Tue, Jan 25, 2022 at 05:22:32AM +, tanghy.f...@fujitsu.com wrote: > > I tried to add a flag(casesensitive) in the _complete_from_query(). > Now the attached patch passed all the added tap tests. Thanks for updating the patch. When you do so, please check and update the commitfest entry accordingly to make sure that people knows it's ready for review. I'm switching the entry to Needs Review.
RE: Support tab completion for upper character inputs in psql
On Monday, January 24, 2022 6:36 PM, Peter Eisentraut
wrote:
> The way your patch works now is that the case-insensitive behavior you
> are implementing only works if the client encoding is a single-byte
> encoding. This isn't what downcase_identifier() does;
> downcase_identifier() always works for ASCII characters. As it is, this
> patch is nearly useless, since very few people use single-byte client
> encodings anymore. Also, I think it would be highly confusing if the
> tab completion behavior depended on the client encoding in a significant
> way.
Thanks for your review. I misunderstood the logic of downcase_identifier().
Modified the code to support ASCII characters input.
> Also, as I had previously suspected, your patch treats the completion of
> enum labels in a case-insensitive way (since it all goes through
> _complete_from_query()), but enum labels are not case insensitive. You
> can observe this behavior using this test case:
>
> +check_completion("ALTER TYPE enum1 RENAME VALUE 'F\t\t", qr|foo|, "FIXME");
> +
> +clear_line();
Your suspect is correct. I didn't aware enum labels are case sensitive.
I've added this test to the tap tests.
> You should devise a principled way to communicate to
> _complete_from_query() whether it should do case-sensitive or
> -insensitive completion. We already have COMPLETE_WITH() and
> COMPLETE_WITH_CS() etc. to do this in other cases, so it should be
> straightforward to adapt a similar system.
I tried to add a flag(casesensitive) in the _complete_from_query().
Now the attached patch passed all the added tap tests.
Regards,
Tang
v13-0001-Support-tab-completion-with-a-query-result-for-u.patch
Description: v13-0001-Support-tab-completion-with-a-query-result-for-u.patch
Re: Support tab completion for upper character inputs in psql
On 20.01.22 08:37, tanghy.f...@fujitsu.com wrote:
1. The downcasing logic in the patch bears very little resemblance
to the backend's actual downcasing logic, which can be found in
src/backend/parser/scansup.c's downcase_identifier(). Notably,
the patch's restriction to only convert all-ASCII strings seems
indefensible, because that's not how things really work. I fear
we can't always exactly duplicate the backend's behavior, because
it's dependent on the server's locale and encoding; but I think
we should at least get it right in the common case where psql is
using the same locale and encoding as the server.
Thanks for your suggestion, I removed ASCII strings check function
and added single byte encoding check just like downcase_identifier.
Also added PGCLIENTENCODING setting in the test script to make
test cases pass.
Now the patch supports tab-completion with none-quoted upper characters
available when client encoding is in single byte.
The way your patch works now is that the case-insensitive behavior you
are implementing only works if the client encoding is a single-byte
encoding. This isn't what downcase_identifier() does;
downcase_identifier() always works for ASCII characters. As it is, this
patch is nearly useless, since very few people use single-byte client
encodings anymore. Also, I think it would be highly confusing if the
tab completion behavior depended on the client encoding in a significant
way.
Also, as I had previously suspected, your patch treats the completion of
enum labels in a case-insensitive way (since it all goes through
_complete_from_query()), but enum labels are not case insensitive. You
can observe this behavior using this test case:
+check_completion("ALTER TYPE enum1 RENAME VALUE 'F\t\t", qr|foo|, "FIXME");
+
+clear_line();
You should devise a principled way to communicate to
_complete_from_query() whether it should do case-sensitive or
-insensitive completion. We already have COMPLETE_WITH() and
COMPLETE_WITH_CS() etc. to do this in other cases, so it should be
straightforward to adapt a similar system.
RE: Support tab completion for upper character inputs in psql
On Sunday, January 16, 2022 3:51 AM, Tom Lane said: > Peter Eisentraut writes: > > The rest of the patch seems ok in principle, since AFAICT enums are the > > only query result in tab-complete.c that are not identifiers and thus > > subject to case issues. > > I spent some time looking at this patch. I'm not very happy with it, > for two reasons: > > 1. The downcasing logic in the patch bears very little resemblance > to the backend's actual downcasing logic, which can be found in > src/backend/parser/scansup.c's downcase_identifier(). Notably, > the patch's restriction to only convert all-ASCII strings seems > indefensible, because that's not how things really work. I fear > we can't always exactly duplicate the backend's behavior, because > it's dependent on the server's locale and encoding; but I think > we should at least get it right in the common case where psql is > using the same locale and encoding as the server. Thanks for your suggestion, I removed ASCII strings check function and added single byte encoding check just like downcase_identifier. Also added PGCLIENTENCODING setting in the test script to make test cases pass. Now the patch supports tab-completion with none-quoted upper characters available when client encoding is in single byte. > 2. I don't think there's been much thought about the larger picture > of what is to be accomplished. Right now, we successfully > tab-complete inputs that are prefixes of the canonical spelling (per > quote_identifier) of the object's name, and don't try at all for > non-canonical spellings. I'm on board with trying to allow some of > the latter but I'm not sure that this patch represents much forward > progress. To be definite about it, suppose we have a DB containing > just two tables whose names start with "m", say mytab and mixedTab. > Then: > > (a) m immediately completes mytab, ignoring mixedTab > > (b) "m immediately completes "mixedTab", ignoring mytab > > (c) "my fails to find anything > > (d) mi fails to find anything > > (e) M fails to find anything > > This patch proposes to improve case (e), but to my taste cases (a) > through (c) are much bigger problems. It'd be nice if (d) worked too > --- that'd require injecting a double-quote where the user had not > typed one, but we already do the equivalent thing with single-quotes > for file names, so why not? (Although after fighting with readline > yesterday to try to get it to handle single-quoted enum labels sanely, > I'm not 100% sure if (d) is possible.) > > Also, even for case (e), what we have with this patch is that it > immediately completes mytab, ignoring mixedTab. Is that what we want? > Another example is that miX fails to find anything, which seems > like a POLA violation given that mY completes to mytab. > > I'm not certain how many of these alternatives can be supported > without introducing ambiguity that wasn't there before (which'd > manifest as failing to complete in cases where the existing code > chooses an alternative just fine). But I really don't like the > existing behavior for (b) and (c) --- I should be able to spell > a name with double quotes if I want, without losing completion > support. You are right, it's more convenient in that way. I haven't thought about it before. By now, the patch suppose: If user needs to type a table with name in upper character, they should input the double quotes by themselves. If the double quote is input by a user, only table name with upper character could be searched. I may try to implement as you expected but it seems not so easy. (as you said, without introducing ambiguity that wasn't there before) I'd appreciate if someone could give me a hint/hand on this. > BTW, another thing that maybe we should think about is how this > interacts with the pattern matching capability in \d and friends. > If people can tab-complete non-canonical spellings, they might > expect the same spellings to work in \d. I don't say that this > patch has to fix that, but we might want to look and be sure we're > not painting ourselves into a corner (especially since I see > that we already perform tab-completion in that context). Yes. Agreed, if we solve the previous problem, meta-command tab completion should also be considered. Regards, Tang v12-0001-Support-tab-completion-with-a-query-result-for-u.patch Description: v12-0001-Support-tab-completion-with-a-query-result-for-u.patch
Re: Support tab completion for upper character inputs in psql
Hi, On Sat, Jan 15, 2022 at 01:51:26PM -0500, Tom Lane wrote: > > I spent some time looking at this patch. I'm not very happy with it, > for two reasons: > [...] On top of that the patch doesn't apply anymore: http://cfbot.cputube.org/patch_36_2979.log === Applying patches on top of PostgreSQL commit ID 5987feb70b5bbb1fc4e64d433f490df08d91dd45 === === applying patch ./v11-0001-Support-tab-completion-with-a-query-result-for-u.patch patching file src/bin/psql/t/010_tab_completion.pl Hunk #1 FAILED at 41. Hunk #2 succeeded at 150 (offset 1 line). 1 out of 2 hunks FAILED -- saving rejects to file src/bin/psql/t/010_tab_completion.pl.rej I'm switching the CF entry to Waiting on Author.
Re: Support tab completion for upper character inputs in psql
Peter Eisentraut writes: > The rest of the patch seems ok in principle, since AFAICT enums are the > only query result in tab-complete.c that are not identifiers and thus > subject to case issues. I spent some time looking at this patch. I'm not very happy with it, for two reasons: 1. The downcasing logic in the patch bears very little resemblance to the backend's actual downcasing logic, which can be found in src/backend/parser/scansup.c's downcase_identifier(). Notably, the patch's restriction to only convert all-ASCII strings seems indefensible, because that's not how things really work. I fear we can't always exactly duplicate the backend's behavior, because it's dependent on the server's locale and encoding; but I think we should at least get it right in the common case where psql is using the same locale and encoding as the server. 2. I don't think there's been much thought about the larger picture of what is to be accomplished. Right now, we successfully tab-complete inputs that are prefixes of the canonical spelling (per quote_identifier) of the object's name, and don't try at all for non-canonical spellings. I'm on board with trying to allow some of the latter but I'm not sure that this patch represents much forward progress. To be definite about it, suppose we have a DB containing just two tables whose names start with "m", say mytab and mixedTab. Then: (a) m immediately completes mytab, ignoring mixedTab (b) "m immediately completes "mixedTab", ignoring mytab (c) "my fails to find anything (d) mi fails to find anything (e) M fails to find anything This patch proposes to improve case (e), but to my taste cases (a) through (c) are much bigger problems. It'd be nice if (d) worked too --- that'd require injecting a double-quote where the user had not typed one, but we already do the equivalent thing with single-quotes for file names, so why not? (Although after fighting with readline yesterday to try to get it to handle single-quoted enum labels sanely, I'm not 100% sure if (d) is possible.) Also, even for case (e), what we have with this patch is that it immediately completes mytab, ignoring mixedTab. Is that what we want? Another example is that miX fails to find anything, which seems like a POLA violation given that mY completes to mytab. I'm not certain how many of these alternatives can be supported without introducing ambiguity that wasn't there before (which'd manifest as failing to complete in cases where the existing code chooses an alternative just fine). But I really don't like the existing behavior for (b) and (c) --- I should be able to spell a name with double quotes if I want, without losing completion support. BTW, another thing that maybe we should think about is how this interacts with the pattern matching capability in \d and friends. If people can tab-complete non-canonical spellings, they might expect the same spellings to work in \d. I don't say that this patch has to fix that, but we might want to look and be sure we're not painting ourselves into a corner (especially since I see that we already perform tab-completion in that context). regards, tom lane
Re: Support tab completion for upper character inputs in psql
On 07.01.22 06:17, tanghy.f...@fujitsu.com wrote:
On Friday, January 7, 2022 1:08 PM, Japin Li wrote:
+/*
+ * pg_string_tolower - Fold a string to lower case if the string is not quoted
+ * and only contains ASCII characters.
+ * For German/Turkish etc text, no change will be made.
+ *
+ * The returned value has to be freed.
+ */
+static char *
+pg_string_tolower_if_ascii(const char *text)
+{
s/pg_string_tolower/pg_string_tolower_if_ascii/ for comments.
Thanks for your review.
Comment fixed in the attached V11 patch.
As I just posted over at [0], the tab completion of enum values appears
to be broken at the moment, so I can't really analyze what impact your
patch would have on it. (But it makes me suspicious about the test case
in your patch.) I suspect it would treat enum labels as
case-insensitive, which would be wrong. But we need to fix that issue
first before we can proceed here.
The rest of the patch seems ok in principle, since AFAICT enums are the
only query result in tab-complete.c that are not identifiers and thus
subject to case issues.
I would perhaps move the pg_string_tolower_if_ascii() calls to before
escape_string() in each case. It won't make a difference to the result,
but it seems conceptually better.
[0]:
https://www.postgresql.org/message-id/8ca82d89-ec3d-8b28-8291-500efaf23...@enterprisedb.com
RE: Support tab completion for upper character inputs in psql
On Friday, January 7, 2022 1:08 PM, Japin Li wrote:
> +/*
> + * pg_string_tolower - Fold a string to lower case if the string is not
> quoted
> + * and only contains ASCII characters.
> + * For German/Turkish etc text, no change will be made.
> + *
> + * The returned value has to be freed.
> + */
> +static char *
> +pg_string_tolower_if_ascii(const char *text)
> +{
>
> s/pg_string_tolower/pg_string_tolower_if_ascii/ for comments.
>
Thanks for your review.
Comment fixed in the attached V11 patch.
Regards,
Tang
v11-0001-Support-tab-completion-with-a-query-result-for-u.patch
Description: v11-0001-Support-tab-completion-with-a-query-result-for-u.patch
Re: Support tab completion for upper character inputs in psql
On Fri, 07 Jan 2022 at 10:12, tanghy.f...@fujitsu.com
wrote:
> On Thursday, January 6, 2022 11:57 PM, Tom Lane wrote:
>>
>> Peter Eisentraut writes:
>> > So the ordering of the suggested completions is different. I don't know
>> > offhand how that ordering is determined. Perhaps it's dependent on
>> > locale, readline version, or operating system. In any case, we need to
>> > figure this out to make this test stable.
>>
>> I don't think we want to get into the business of trying to make that
>> consistent across different readline/libedit versions. How about
>> adjusting the test case so that only one enum value is to be printed?
>>
>
> Thanks for your suggestion. Agreed.
> Fixed the test case to show only one enum value.
>
+/*
+ * pg_string_tolower - Fold a string to lower case if the string is not quoted
+ * and only contains ASCII characters.
+ * For German/Turkish etc text, no change will be made.
+ *
+ * The returned value has to be freed.
+ */
+static char *
+pg_string_tolower_if_ascii(const char *text)
+{
s/pg_string_tolower/pg_string_tolower_if_ascii/ for comments.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
RE: Support tab completion for upper character inputs in psql
On Thursday, January 6, 2022 11:57 PM, Tom Lane wrote: > > Peter Eisentraut writes: > > So the ordering of the suggested completions is different. I don't know > > offhand how that ordering is determined. Perhaps it's dependent on > > locale, readline version, or operating system. In any case, we need to > > figure this out to make this test stable. > > I don't think we want to get into the business of trying to make that > consistent across different readline/libedit versions. How about > adjusting the test case so that only one enum value is to be printed? > Thanks for your suggestion. Agreed. Fixed the test case to show only one enum value. Regards, Tang v10-0001-Support-tab-completion-with-a-query-result-for-u.patch Description: v10-0001-Support-tab-completion-with-a-query-result-for-u.patch
Re: Support tab completion for upper character inputs in psql
Peter Eisentraut writes: > So the ordering of the suggested completions is different. I don't know > offhand how that ordering is determined. Perhaps it's dependent on > locale, readline version, or operating system. In any case, we need to > figure this out to make this test stable. I don't think we want to get into the business of trying to make that consistent across different readline/libedit versions. How about adjusting the test case so that only one enum value is to be printed? regards, tom lane
Re: Support tab completion for upper character inputs in psql
On 10.09.21 15:50, tanghy.f...@fujitsu.com wrote: (A test case for the enum case should be doable easily.) Test added. The enum test is failing on *some* platforms: t/010_tab_completion.pl .. 26/? # Failed test 'complete enum values' # at t/010_tab_completion.pl line 211. # Actual output was "ALTER TYPE mytype1 RENAME VALUE '\a\r\n'BLUE' 'bLACK' 'green' \r\npostgres=# ALTER TYPE mytype1 RENAME VALUE '" # Did not match "(?^:'bLACK' + 'BLUE' + 'green')" So the ordering of the suggested completions is different. I don't know offhand how that ordering is determined. Perhaps it's dependent on locale, readline version, or operating system. In any case, we need to figure this out to make this test stable.
RE: Support tab completion for upper character inputs in psql
On Tuesday, September 7, 2021 5:25 PM, Peter Eisentraut
wrote:
>The coding of valid_input_text() seems a bit bulky. I think you can do
>the same thing using strspn() without a loop.
Thanks, modified in V9 patch.
>The name is also not great. It's not like other strings are not "valid".
Modified.
valid_input_text() renamed to check_input_text()
>There is also no explanation why that specific set of characters is
>allowed and not others. Does it have something to do with identifier
>syntax? This needs to be explained.
Added some comments for pg_string_tolower_if_ascii().
For language like German/Turkish, it's not a good idea to lower the input text
because the upper case words may not retain the same meaning.(Pointed at [1~3])
>Seeing that valid_input_text() is always called together with
>pg_string_tolower(), I think those could be combined into one function,
>like pg_string_tolower_if_ascii() is whatever. That would save a lot of
>repetition.
Modified.
>There are a couple of queries where the result is *not*
>case-insensitive, namely
>
>Query_for_list_of_enum_values
>Query_for_list_of_available_extension_versions
>
>(and their variants). These are cases where the query result is not
>used as an identifier but as a (single-quoted) string. So that needs to
>be handled somehow, perhaps by adding a COMPLETE_WITH_QUERY_CS() similar
>to COMPLETE_WITH_CS().
Hmm, I think 'a (single-quoted) string' identifier behaves the same way with or
without my patch.
Could your please give me an example on that?(to help me figure out why we need
something like COMPLETE_WITH_QUERY_CS())
>(A test case for the enum case should be doable easily.)
Test added.
BTW, I found tap completion for enum value is not perfect on HEAD.
Maybe I will fix this problem in another thread.
example:
=# create type pp_colors as enum ('green', 'blue', 'black');
=# ALTER TYPE pp_colors RENAME VALUE 'b[tab]
=# alter type pp_colors rename value 'b' <- blue is not auto completed as
expected
[1] https://www.postgresql.org/message-id/1282887.1619151455%40sss.pgh.pa.us
[2]
https://www.postgresql.org/message-id/20210423.13.2058612313278551429.horikyota.ntt%40gmail.com
[3]
https://www.postgresql.org/message-id/a75a6574c0e3d4773ba20a73d493c2c9983c0657.camel%40cybertec.at
Regards,
Tang
v9-0001-Support-tab-completion-with-a-query-result-for-up.patch
Description: v9-0001-Support-tab-completion-with-a-query-result-for-up.patch
Re: Support tab completion for upper character inputs in psql
On 23.06.21 14:43, tanghy.f...@fujitsu.com wrote: I've updated the patch to V8 since Tom, Kyotaro and Laurenz discussed the lower case issue of German/Turkish language at [1]. Differences from V7 are: * Add a function valid_input_text which checks the input text to see if it only contains alphabet letters, numbers etc. * Delete the flag setting of "completion_case_sensitive=false" which introduced in V1 patch and no use now. As you can see, now the patch limited the lower case transform of the input to alphabet letters. By doing that, language like German/Turkish will not affected by this patch. Any comment or suggestion on this patch is very welcome. The coding of valid_input_text() seems a bit bulky. I think you can do the same thing using strspn() without a loop. The name is also not great. It's not like other strings are not "valid". There is also no explanation why that specific set of characters is allowed and not others. Does it have something to do with identifier syntax? This needs to be explained. Seeing that valid_input_text() is always called together with pg_string_tolower(), I think those could be combined into one function, like pg_string_tolower_if_ascii() is whatever. That would save a lot of repetition. There are a couple of queries where the result is *not* case-insensitive, namely Query_for_list_of_enum_values Query_for_list_of_available_extension_versions (and their variants). These are cases where the query result is not used as an identifier but as a (single-quoted) string. So that needs to be handled somehow, perhaps by adding a COMPLETE_WITH_QUERY_CS() similar to COMPLETE_WITH_CS(). (A test case for the enum case should be doable easily.)
RE: Support tab completion for upper character inputs in psql
Hi I've updated the patch to V8 since Tom, Kyotaro and Laurenz discussed the lower case issue of German/Turkish language at [1]. Differences from V7 are: * Add a function valid_input_text which checks the input text to see if it only contains alphabet letters, numbers etc. * Delete the flag setting of "completion_case_sensitive=false" which introduced in V1 patch and no use now. As you can see, now the patch limited the lower case transform of the input to alphabet letters. By doing that, language like German/Turkish will not affected by this patch. Any comment or suggestion on this patch is very welcome. [1] https://www.postgresql.org/message-id/1282887.1619151455%40sss.pgh.pa.us https://www.postgresql.org/message-id/20210423.13.2058612313278551429.horikyota.ntt%40gmail.com https://www.postgresql.org/message-id/a75a6574c0e3d4773ba20a73d493c2c9983c0657.camel%40cybertec.at Regards, Tang V8-0001-Support-tab-completion-with-a-query-result-for-upper.patch Description: V8-0001-Support-tab-completion-with-a-query-result-for-upper.patch
RE: Support tab completion for upper character inputs in psql
Hi
I've updated the patch to V7 based on the following comments.
On Friday, April 23, 2021 11:58 AM, Kyotaro Horiguchi
wrote
>All usages of pg_string_tolower don't need a copy.
>So don't we change the function to in-place converter?
Refer to your later discussion with Tom. Keep the code as it is.
> if (completion_info_charp)
>+ {
> e_info_charp = escape_string(completion_info_charp);
>+ if(e_info_charp[0] == '"')
>+ completion_case_sensitive = true;
>+ else
>+ {
>+ le_str = pg_string_tolower(e_info_charp);
>
>It seems right to lower completion_info_charp and ..2 but it is not
>right that change completion_case_sensitive here, which only affects
>the returned candidates.
Agreed, code " completion_case_sensitive = true;" removed.
>By the way COMP_KEYWORD_CASE suggests that *keywords* are completed
>following the setting. However, they are not keywords, but
>identifiers. And some people (including me) might dislike that
>keywords and identifiers follow the same setting. Specifically I
>sometimes want keywords to be upper-cased but identifiers (always) be
>lower-cased.
Changed my design based on your suggestion. Now the upper character inputs for
identifiers will always turn to lower case(regardless COMP_KEYWORD_CASE) which
I think can be accepted by most of PG users.
Eg: SET BYT / SET Byt
output when apply V6 patch: SET BYTEA_OUTPUT
output when apply V7 patch: SET bytea_output
On Friday, April 23, 2021 12:26 PM, Kyotaro Horiguchi
wrote
>Oh, I accidentally found a doubious behsbior.
>
>=# alter table public.
>public.c1public.d1public."t" public.t public."tt"
>
>The "t" and "tt" are needlessly lower-cased.
Good catch. I didn’t think of schema stuff before.
Bug fixed. Add tap tests for this scenario.
Please let me know if you find more insufficient issue in the patch. Any
further suggestion is very welcome.
Regards,
Tang
V7-0001-Support-tab-completion-with-a-query-result-for-upper.patch
Description: V7-0001-Support-tab-completion-with-a-query-result-for-upper.patch
Re: Support tab completion for upper character inputs in psql
On Fri, 2021-04-23 at 14:44 +0900, Kyotaro Horiguchi wrote: > > The two examples I know of offhand are in German (eszett "ß" downcases to > > "ss") and Turkish (dotted "Í" downcases to "i", likewise dotless "I" > > According to Wikipedia, "ss" is equivalent to "ß" and their upper case > letters are "SS" and "ẞ" respectively. (I didn't even know of the > existence of "ẞ". AFAIK there's no word begins with eszett, but it > seems that there's a case where "ẞ" appears in a word is spelled only > with capital letters. This "capital sharp s" is a recent invention that has never got much traction. I notice that on my Fedora 32 system with glibc 2.31 and de_DE.utf8, SELECT lower(E'\u1E9E') = E'\u00DF', upper(E'\u00DF') = E'\u1E9E'; ?column? │ ?column? ══╪══ t│ f (1 row) which to me as a German speaker makes no sense. But Tom's example was the wrong way around: "ß" is a lower case letter, and the traditional upper case translation is "SS". But the Turkish example is correct: > > downcases to "ı"; one of each of those pairs is an ASCII letter, the > > other is not). Depending on which encoding is in use, these > > Upper dotless "I" and lower dotted "i" are in ASCII (or English > alphabet?). That's interesting. Yes. In languages other than Turkish, "i" is the lower case version of "I", and both are ASCII. Only Turkish has an "ı" (U+0131) and an "İ" (U+0130). That causes annoyance for Turks who create a table named KADIN and find that PostgreSQL turns it into "kadin". Yours, Laurenz Albe
Re: Support tab completion for upper character inputs in psql
FWIW... At Fri, 23 Apr 2021 00:17:35 -0400, Tom Lane wrote in > Kyotaro Horiguchi writes: > > At Thu, 22 Apr 2021 23:17:19 -0400, Tom Lane wrote in > >> Doesn't seem like a good idea, because that locks us into an assumption > >> that the downcasing conversion doesn't change the string's physical > >> length. There are a lot of counterexamples to that :-(. I'm not sure > > > Mmm. I didn't know of that. > > The two examples I know of offhand are in German (eszett "ß" downcases to > "ss") and Turkish (dotted "Í" downcases to "i", likewise dotless "I" According to Wikipedia, "ss" is equivalent to "ß" and their upper case letters are "SS" and "ẞ" respectively. (I didn't even know of the existence of "ẞ". AFAIK there's no word begins with eszett, but it seems that there's a case where "ẞ" appears in a word is spelled only with capital letters. > downcases to "ı"; one of each of those pairs is an ASCII letter, the > other is not). Depending on which encoding is in use, these Upper dotless "I" and lower dotted "i" are in ASCII (or English alphabet?). That's interesting. > transformations *could* be the same number of bytes, but they could > equally well not be. There are probably other examples. Yeah. Agreed. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Support tab completion for upper character inputs in psql
Kyotaro Horiguchi writes: > At Thu, 22 Apr 2021 23:17:19 -0400, Tom Lane wrote in >> Doesn't seem like a good idea, because that locks us into an assumption >> that the downcasing conversion doesn't change the string's physical >> length. There are a lot of counterexamples to that :-(. I'm not sure > Mmm. I didn't know of that. The two examples I know of offhand are in German (eszett "ß" downcases to "ss") and Turkish (dotted "Í" downcases to "i", likewise dotless "I" downcases to "ı"; one of each of those pairs is an ASCII letter, the other is not). Depending on which encoding is in use, these transformations *could* be the same number of bytes, but they could equally well not be. There are probably other examples. regards, tom lane
Re: Support tab completion for upper character inputs in psql
At Thu, 22 Apr 2021 23:17:19 -0400, Tom Lane wrote in > Kyotaro Horiguchi writes: > > All usages of pg_string_tolower don't need a copy. > > So don't we change the function to in-place converter? > > Doesn't seem like a good idea, because that locks us into an assumption > that the downcasing conversion doesn't change the string's physical > length. There are a lot of counterexamples to that :-(. I'm not sure Mmm. I didn't know of that. > that we actually implement such cases correctly today, but let's not > build APIs that prevent it from being fixed. Agreed. Thanks for the knowledge. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Support tab completion for upper character inputs in psql
At Fri, 23 Apr 2021 11:58:12 +0900 (JST), Kyotaro Horiguchi wrote in > > Any further comment is very welcome. Oh, I accidentally found a doubious behsbior. =# alter table public. public.c1public.d1public."t" public.t public."tt" The "t" and "tt" are needlessly lower-cased. # \d List of relations Schema |Name| Type| Owner ++---+-- public | T | partitioned table | horiguti public | TT | table | horiguti public | c1 | table | horiguti public | d1 | table | horiguti public | t | partitioned table | horiguti =# alter table public." =# alter table public."t-- candidates are "t" and "tt"? =# alter table public."tt -- nothing happenes =# alter table public."TT -- also nothing happenes regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Support tab completion for upper character inputs in psql
Kyotaro Horiguchi writes: > All usages of pg_string_tolower don't need a copy. > So don't we change the function to in-place converter? Doesn't seem like a good idea, because that locks us into an assumption that the downcasing conversion doesn't change the string's physical length. There are a lot of counterexamples to that :-(. I'm not sure that we actually implement such cases correctly today, but let's not build APIs that prevent it from being fixed. regards, tom lane
Re: Support tab completion for upper character inputs in psql
At Thu, 22 Apr 2021 12:43:42 +, "tanghy.f...@fujitsu.com"
wrote in
> On Wednesday, April 21, 2021 1:24 PM, Peter Smith
> Wrot> >4. Memory not freed in multiple places?
> oops. Memory free added.
All usages of pg_string_tolower don't need a copy.
So don't we change the function to in-place converter?
> >6. byte_length == 0?
> >The byte_length was not being checked before, so why is the check needed now?
>
> We need to make sure the empty input to be case sensitive as before(HEAD).
> For example
> CREATE TABLE onetab1 (f1 int);
> update onetab1 SET [tab]
>
> Without the check of "byte_length == 0", pg_strdup_keyword_case will make the
> column name "f1" to be upper case "F1".
> Namely, the output will be " update onetab1 SET F1" which is not so good.
>
> I added some tab tests for this empty input case, too.
>
> >7. test typo "ralation"
> >8. test typo "case-insensitiveq"
> Thanks, typo fixed.
>
> Any further comment is very welcome.
if (completion_info_charp)
+ {
e_info_charp = escape_string(completion_info_charp);
+ if(e_info_charp[0] == '"')
+ completion_case_sensitive = true;
+ else
+ {
+ le_str = pg_string_tolower(e_info_charp);
It seems right to lower completion_info_charp and ..2 but it is not
right that change completion_case_sensitive here, which only affects
the returned candidates. This change prevents the following operation
from getting the expected completion candidates.
=# create table "T" (a int) partition by range(a);
=# create table c1 partition of "T" for values from (0) to (10);
=# alter table "T" drop partition C
Is there any reason for doing that?
+ if (byte_length == 0 ||
completion_case_sensitive)
Is the condition "byte_length == 0 ||" right?
This results in a maybe-unexpected behavior,
=# \set COM_KEYWORD_CASE upper
=# create table t (a int) partition by range(a);
=# create table d1 partition of t for values from (0) to (10);
=# alter table t drop partition
This results in
=# alter table t drop partition d1
I think we are expecting D1 as the result.
By the way COMP_KEYWORD_CASE suggests that *keywords* are completed
following the setting. However, they are not keywords, but
identifiers. And some people (including me) might dislike that
keywords and identifiers follow the same setting. Specifically I
sometimes want keywords to be upper-cased but identifiers (always) be
lower-cased.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
RE: Support tab completion for upper character inputs in psql
On Wednesday, April 21, 2021 1:24 PM, Peter Smith Wrote >I tried playing a bit with your psql patch V5 and I did not find any >problems - it seemed to work as advertised. > >Below are a few code review comments. Thanks for you review. I've updated the patch to V6 according to your comments. >1. Patch applies with whitespace warnings. Fixed. >2. Unrelated "code tidy" fixes maybe should be another patch? Agreed. Will post this modification on another thread. >3. Unnecessary NULL check? Agreed. NULL check removed. >4. Memory not freed in multiple places? oops. Memory free added. >5. strncmp replacement? Agreed. Thanks for your advice. Since this modification has little relation with my patch here. I will merge this with comment(2) and push this on another patch. >6. byte_length == 0? >The byte_length was not being checked before, so why is the check needed now? We need to make sure the empty input to be case sensitive as before(HEAD). For example CREATE TABLE onetab1 (f1 int); update onetab1 SET [tab] Without the check of "byte_length == 0", pg_strdup_keyword_case will make the column name "f1" to be upper case "F1". Namely, the output will be " update onetab1 SET F1" which is not so good. I added some tab tests for this empty input case, too. >7. test typo "ralation" >8. test typo "case-insensitiveq" Thanks, typo fixed. Any further comment is very welcome. Regards, Tang V6-0001-Support-tab-completion-with-a-query-result-for-upper.patch Description: V6-0001-Support-tab-completion-with-a-query-result-for-upper.patch
Re: Support tab completion for upper character inputs in psql
On Wed, Apr 14, 2021 at 11:34 PM tanghy.f...@fujitsu.com
wrote:
>
> On Thursday, April 8, 2021 4:14 PM, Peter Eisentraut
> wrote
>
> >Seeing the tests you provided, it's pretty obvious that the current
> >behavior is insufficient. I think we could probably think of a few more
> >tests, for example exercising the "If case insensitive matching was
> >requested initially, adjust the case according to setting." case, or
> >something with quoted identifiers.
>
> Thanks for your review and suggestions on my patch.
> I've added more tests in the latest patch V5, the added tests helped me find
> some bugs in my patch and I fixed them.
> Now the patch can support not only the SET/SHOW [PARAMETER] but also UPDATE
> ["aTable"|ATABLE], also UPDATE atable SET ["aColumn"|ACOLUMN].
>
> I really hope someone can have more tests suggestions on my patch or kindly
> do some tests on my patch and share me if any bugs happened.
>
> Differences from V4 are:
> * fix some bugs related to quoted identifiers.
> * add some tap tests.
I tried playing a bit with your psql patch V5 and I did not find any
problems - it seemed to work as advertised.
Below are a few code review comments.
1. Patch applies with whitespace warnings.
[postgres@CentOS7-x64 oss_postgres_2PC]$ git apply
../patches_misc/V5-0001-Support-tab-completion-with-a-query-result-for-upper.patch
../patches_misc/V5-0001-Support-tab-completion-with-a-query-result-for-upper.patch:130:
trailing whitespace.
}
warning: 1 line adds whitespace errors.
2. Unrelated "code tidy" fixes maybe should be another patch?
I noticed there are a couple of "code tidy" fixes combined with this
patch - e.g. passing fixes to some code comments and blank lines etc
(see below). Although they are all good improvements, they maybe don't
really have anything to do with your feature/bugfix so I am not sure
if they should be included here. Maybe post a separate patch for these
ones?
@@ -1028,7 +1032,7 @@ static const VersionedQuery
Query_for_list_of_subscriptions[] = {
};
/*
- * This is a list of all "things" in Pgsql, which can show up after CREATE or
+ * This is a list of all "things" in pgsql, which can show up after CREATE or
* DROP; and there is also a query to get a list of them.
*/
@@ -4607,7 +4642,6 @@ complete_from_list(const char *text, int state)
if (completion_case_sensitive)
return pg_strdup(item);
else
-
/*
* If case insensitive matching was requested initially,
* adjust the case according to setting.
@@ -4660,7 +4694,6 @@ complete_from_const(const char *text, int state)
if (completion_case_sensitive)
return pg_strdup(completion_charp);
else
-
/*
* If case insensitive matching was requested initially, adjust
* the case according to setting.
3. Unnecessary NULL check?
@@ -4420,16 +4425,37 @@ _complete_from_query(const char *simple_query,
PQclear(result);
result = NULL;
- /* Set up suitably-escaped copies of textual inputs */
+ /* Set up suitably-escaped copies of textual inputs,
+ * then change the textual inputs to lower case.
+ */
e_text = escape_string(text);
+ if(e_text != NULL)
+ {
+ if(e_text[0] == '"')
+ completion_case_sensitive = true;
+ else
+ e_text = pg_string_tolower(e_text);
+ }
Perhaps that check "if(e_text != NULL)" is unnecessary. That function
hardly looks capable of returning a NULL, and other callers are not
checking the return like this.
4. Memory not freed in multiple places?
@@ -4420,16 +4425,37 @@ _complete_from_query(const char *simple_query,
PQclear(result);
result = NULL;
- /* Set up suitably-escaped copies of textual inputs */
+ /* Set up suitably-escaped copies of textual inputs,
+ * then change the textual inputs to lower case.
+ */
e_text = escape_string(text);
+ if(e_text != NULL)
+ {
+ if(e_text[0] == '"')
+ completion_case_sensitive = true;
+ else
+ e_text = pg_string_tolower(e_text);
+ }
if (completion_info_charp)
+ {
e_info_charp = escape_string(completion_info_charp);
+ if(e_info_charp[0] == '"')
+ completion_case_sensitive = true;
+ else
+ e_info_charp = pg_string_tolower(e_info_charp);
+ }
else
e_info_charp = NULL;
if (completion_info_charp2)
+ {
e_info_charp2 = escape_string(completion_info_charp2);
+ if(e_info_charp2[0] == '"')
+ completion_case_sensitive = true;
+ else
+ e_info_charp2 = pg_string_tolower(e_info_charp2);
+ }
else
e_info_charp2 = NULL;
The function escape_string has a comment saying "The returned value
has to be freed." but in the above code you are overwriting the
escape_string result with the strdup'ed pg_string_tolower but without
free-ing the original e_text/e_info_charp/e_info_charp2.
==
5. strncmp replacement?
@@ -4464,7 +4490,7 @@ _complete_from_query(const char *simple_query,
*/
if (strcmp(schema_query->catname,
"pg_catalog.pg_class c") == 0 &&
- strncmp(text, "pg_", 3) != 0)
+ strncmp(pg_string_tolower(text), "pg_", 3) != 0)
{
appendPQExpBufferStr(&query_buffer,
" AND c.relnamespace <> (SELECT oid
RE: Support tab completion for upper character inputs in psql
On Thursday, April 8, 2021 4:14 PM, Peter Eisentraut wrote >Seeing the tests you provided, it's pretty obvious that the current >behavior is insufficient. I think we could probably think of a few more >tests, for example exercising the "If case insensitive matching was >requested initially, adjust the case according to setting." case, or >something with quoted identifiers. Thanks for your review and suggestions on my patch. I've added more tests in the latest patch V5, the added tests helped me find some bugs in my patch and I fixed them. Now the patch can support not only the SET/SHOW [PARAMETER] but also UPDATE ["aTable"|ATABLE], also UPDATE atable SET ["aColumn"|ACOLUMN]. I really hope someone can have more tests suggestions on my patch or kindly do some tests on my patch and share me if any bugs happened. Differences from V4 are: * fix some bugs related to quoted identifiers. * add some tap tests. Regards, Tang V5-0001-Support-tab-completion-with-a-query-result-for-upper.patch Description: V5-0001-Support-tab-completion-with-a-query-result-for-upper.patch
Re: Support tab completion for upper character inputs in psql
On 01.04.21 11:40, tanghy.f...@fujitsu.com wrote: On Wednesday, March 31, 2021 4:05 AM, David Zhang wrote 8 postgres=# update tbl SET DATA = 9 10 postgres=# update TBL SET 11 12 postgres=# So, as you can see the difference is between line 8 and 10 in case 2. It looks like the lowercase can auto complete more than the uppercase; secondly, if you can add some test cases, it would be great. Thanks for your test. I fix the bug and add some tests for it. Please find attached the latest patch V4. Differences from v3 are: * fix an issue reported by Zhang [1] where a scenario was found which still wasn't able to realize tap completion in query. * add some tap tests. Seeing the tests you provided, it's pretty obvious that the current behavior is insufficient. I think we could probably think of a few more tests, for example exercising the "If case insensitive matching was requested initially, adjust the case according to setting." case, or something with quoted identifiers. I'll push this to the next commit fest for now. I encourage you to keep working on it.
RE: Support tab completion for upper character inputs in psql
On Wednesday, March 31, 2021 4:05 AM, David Zhang wrote > 8 postgres=# update tbl SET DATA = > 9 > 10 postgres=# update TBL SET > 11 > 12 postgres=# > >So, as you can see the difference is between line 8 and 10 in case 2. It >looks like the lowercase can auto complete more than the uppercase; >secondly, if you can add some test cases, it would be great. Thanks for your test. I fix the bug and add some tests for it. Please find attached the latest patch V4. Differences from v3 are: * fix an issue reported by Zhang [1] where a scenario was found which still wasn't able to realize tap completion in query. * add some tap tests. [1] https://www.postgresql.org/message-id/3140db2a-9808-c470-7e60-de39c431b3ab%40highgo.ca Regards, Tang V4-0001-Support-tab-completion-with-a-query-result-for-upper.patch Description: V4-0001-Support-tab-completion-with-a-query-result-for-upper.patch
Re: Support tab completion for upper character inputs in psql
Hi Tang, Thanks a lot for the patch. I did a quick test based on the latest patch V3 on latest master branch "commit 4753ef37e0eda4ba0af614022d18fcbc5a946cc9". Case 1: before patch 1 postgres=# set a 2 all allow_system_table_mods application_name array_nulls 3 postgres=# set A 4 5 postgres=# create TABLE tbl (data text); 6 CREATE TABLE 7 postgres=# update tbl SET DATA = 8 9 postgres=# update T 10 11 postgres=# Case 2: after patched 1 postgres=# set a 2 all allow_system_table_mods application_name array_nulls 3 postgres=# set A 4 ALL ALLOW_SYSTEM_TABLE_MODS APPLICATION_NAME ARRAY_NULLS 5 postgres=# create TABLE tbl (data text); 6 CREATE TABLE 7 8 postgres=# update tbl SET DATA = 9 10 postgres=# update TBL SET 11 12 postgres=# So, as you can see the difference is between line 8 and 10 in case 2. It looks like the lowercase can auto complete more than the uppercase; secondly, if you can add some test cases, it would be great. Best regards, David On 2021-03-22 5:41 a.m., tanghy.f...@fujitsu.com wrote: On Tuesday, March 16, 2021 5:20 AM, Peter Eisentraut wrote: The cases that complete with a query result are not case insensitive right now. This affects things like UPDATE T as well. I think your first patch was basically right. But we need to understand that this affects all completions with query results, not just the one you wanted to fix. So you should analyze all the callers and explain why the proposed change is appropriate. Thanks for your review and suggestion. Please find attached patch V3 which was based on the first patch[1]. Difference from the first patch is: Add tab completion support for all query results in psql. complete_from_query +complete_from_versioned_query +complete_from_schema_query +complete_from_versioned_schema_query [1] https://www.postgresql.org/message-id/a63cbd45e3884cf9b3961c2a6a95dcb7%40G08CNEXMBPEKD05.g08.fujitsu.local The modification to support case insensitive matching in " _complete_from_query" is based on "complete_from_const and "complete_from_list" . Please let me know if you find anything insufficient. Regards, Tang -- David Software Engineer Highgo Software Inc. (Canada) www.highgo.ca
RE: Support tab completion for upper character inputs in psql
On Tuesday, March 16, 2021 5:20 AM, Peter Eisentraut wrote: >The cases that complete with a query >result are not case insensitive right now. This affects things like > >UPDATE T > >as well. I think your first patch was basically right. But we need to >understand that this affects all completions with query results, not >just the one you wanted to fix. So you should analyze all the callers >and explain why the proposed change is appropriate. Thanks for your review and suggestion. Please find attached patch V3 which was based on the first patch[1]. Difference from the first patch is: Add tab completion support for all query results in psql. complete_from_query +complete_from_versioned_query +complete_from_schema_query +complete_from_versioned_schema_query [1] https://www.postgresql.org/message-id/a63cbd45e3884cf9b3961c2a6a95dcb7%40G08CNEXMBPEKD05.g08.fujitsu.local The modification to support case insensitive matching in " _complete_from_query" is based on "complete_from_const and "complete_from_list" . Please let me know if you find anything insufficient. Regards, Tang V3-0001-Support-tab-completion-with-a-query-result-for-upper.patch Description: V3-0001-Support-tab-completion-with-a-query-result-for-upper.patch
Re: Support tab completion for upper character inputs in psql
On 09.02.21 15:48, Tang, Haiying wrote: I'm still confused about the APPROPRIATE behavior of tab completion. It seems ALTER table/tablespace SET/RESET is already case-insensitive. For example # alter tablespace dbspace set(e[tab] # alter tablespace dbspace set(effective_io_concurrency # alter tablespace dbspace set(E[tab] # alter tablespace dbspace set(EFFECTIVE_IO_CONCURRENCY This case completes with a hardcoded list, which is done case-insensitively by default. The cases that complete with a query result are not case insensitive right now. This affects things like UPDATE T as well. I think your first patch was basically right. But we need to understand that this affects all completions with query results, not just the one you wanted to fix. So you should analyze all the callers and explain why the proposed change is appropriate.
RE: Support tab completion for upper character inputs in psql
At Sun, 07 Feb 2021 13:55:00 -0500, Tom Lane wrote in > > This looks like you're trying to force case-insensitive behavior > whether that is appropriate or not. Does not sound like a good idea. I'm still confused about the APPROPRIATE behavior of tab completion. It seems ALTER table/tablespace SET/RESET is already case-insensitive. For example # alter tablespace dbspace set(e[tab] # alter tablespace dbspace set(effective_io_concurrency # alter tablespace dbspace set(E[tab] # alter tablespace dbspace set(EFFECTIVE_IO_CONCURRENCY The above behavior is exactly the same as what the patch(attached in the following message) did for SET/RESET etc. https://www.postgresql.org/message-id/flat/a63cbd45e3884cf9b3961c2a6a95dcb7%40G08CNEXMBPEKD05.g08.fujitsu.local If anyone can share me some cases which show inappropriate scenarios of forcing case-insensitive inputs in psql. I'd be grateful for that. Regards, Tang
RE: Support tab completion for upper character inputs in psql
At Sun, 07 Feb 2021 13:55:00 -0500, Tom Lane wrote in > > This looks like you're trying to force case-insensitive behavior > whether that is appropriate or not. Does not sound like a good idea. Thanks for your reply. I raise this issue because I thought all SQL command should be case-insensitive. And the set/reset/show commands work well no matter the input configuration parameter is in upper or in lower case. My modification is not good enough, but I really think it's more convenient if we can support the tab-completion for upper character inputs. =# set APPLICATION_NAME to test; SET =# show APPLICATION_name; application_name -- test (1 row) From: Kyotaro Horiguchi Sent: Monday, February 8, 2021 5:02 PM >However set doesn't. If it is what is wanted, the following change on >Query_for_list_of_set_vars works (only for the case of SET/RESET commands). Thanks for your update. I applied your patch, it works well for SET/RESET commands. I added the same modification to SHOW command. The new patch(V2) can support tab completion for upper character inputs in psql for SET/RESET/SHOW commands. Regards, Tang V2-0001-Support-tab-completion-for-upper-character-inputs-in.patch Description: V2-0001-Support-tab-completion-for-upper-character-inputs-in.patch
Re: Support tab completion for upper character inputs in psql
At Sun, 07 Feb 2021 13:55:00 -0500, Tom Lane wrote in
> "Tang, Haiying" writes:
> > When using psql I found there's no tab completion for upper character
> > inputs. It's really inconvenient sometimes so I try to fix this problem in
> > the attached patch.
>
> This looks like you're trying to force case-insensitive behavior
> whether that is appropriate or not. Does not sound like a good
> idea.
Agreed. However I'm not sure what the OP exactly wants, \set behaves
in a different but similar way.
=# \set c[tab]
=# \set COMP_KEYWORD_CASE _
However set doesn't. If it is what is wanted, the following change on
Query_for_list_of_set_vars works (only for the case of SET/RESET
commands).
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5f0e775fd3..5c2a263785 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -725,7 +725,8 @@ static const SchemaQuery Query_for_list_of_statistics = {
" UNION ALL SELECT 'role' "\
" UNION ALL SELECT 'tablespace' "\
" UNION ALL SELECT 'all') ss "\
-" WHERE substring(name,1,%d)='%s'"
+" WHERE substring(name,1,%1$d)='%2$s' "\
+"OR pg_catalog.lower(substring(name,1,%1$d))=pg_catalog.lower('%2$s')"
#define Query_for_list_of_show_vars \
"SELECT name FROM "\
=# set AP[tab]
=# set application_name _
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Support tab completion for upper character inputs in psql
"Tang, Haiying" writes: > When using psql I found there's no tab completion for upper character inputs. > It's really inconvenient sometimes so I try to fix this problem in the > attached patch. This looks like you're trying to force case-insensitive behavior whether that is appropriate or not. Does not sound like a good idea. regards, tom lane
Support tab completion for upper character inputs in psql
Hi Hackers, When using psql I found there's no tab completion for upper character inputs. It's really inconvenient sometimes so I try to fix this problem in the attached patch. Here is the examples to show what this patch can do. Action: 1. connect the db using psql 2. input SQL command 3. enter TAB key(twice at the very first time) Results: [master] postgres=# set a all allow_system_table_mods application_name array_nulls postgres=# set A postgres=# set A [patched] postgres=# set a all allow_system_table_mods application_name array_nulls postgres=# set A ALL ALLOW_SYSTEM_TABLE_MODS APPLICATION_NAME ARRAY_NULLS postgres=# set A Please take a check at this patch. Any comment is welcome. Regards, Tang 0001-Support-tab-completion-for-upper-character-inputs-in.patch Description: 0001-Support-tab-completion-for-upper-character-inputs-in.patch
