Thread
Commits
-
Fix minor memory leaks in psql's tab completion.
- 90474c16a7d2 16.0 landed
- 00cf40328a31 15.0 landed
-
Further tweaks for psql's new tab-completion logic.
- f0cd9097cfac 15.0 landed
-
Treat case of tab-completion keywords a bit more carefully.
- 020258fbd30d 15.0 landed
-
psql: improve tab-complete's handling of variant SQL names.
- 02b8048ba5dc 15.0 landed
-
Make PQcancel use the PGconn's tcp_user_timeout and keepalives settings.
- 5987feb70b5b 15.0 cited
-
Use a WaitLatch for vacuum/autovacuum sleeping
- 4753ef37e0ed 14.0 cited
-
Support tab completion for upper character inputs in psql
Tang, Haiying <tanghy.fnst@cn.fujitsu.com> — 2021-02-07T07:06:09Z
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
-
Re: Support tab completion for upper character inputs in psql
Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-07T18:55:00Z
"Tang, Haiying" <tanghy.fnst@cn.fujitsu.com> 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
-
Re: Support tab completion for upper character inputs in psql
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-02-08T08:02:12Z
At Sun, 07 Feb 2021 13:55:00 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in > "Tang, Haiying" <tanghy.fnst@cn.fujitsu.com> 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 <tanghy.fnst@cn.fujitsu.com> — 2021-02-08T12:12:35Z
At Sun, 07 Feb 2021 13:55:00 -0500, Tom Lane <tgl@sss.pgh.pa.us> 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 <horikyota.ntt@gmail.com> 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
-
RE: Support tab completion for upper character inputs in psql
Tang, Haiying <tanghy.fnst@cn.fujitsu.com> — 2021-02-09T14:48:02Z
At Sun, 07 Feb 2021 13:55:00 -0500, Tom Lane <tgl@sss.pgh.pa.us> 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 <name> 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
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-03-15T20:20:22Z
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 <name> 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<tab> 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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2021-03-22T12:41:41Z
On Tuesday, March 16, 2021 5:20 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: >The cases that complete with a query >result are not case insensitive right now. This affects things like > >UPDATE T<tab> > >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
-
Re: Support tab completion for upper character inputs in psql
David Zhang <david.zhang@highgo.ca> — 2021-03-30T19:05:10Z
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.fnst@fujitsu.com wrote: > On Tuesday, March 16, 2021 5:20 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > >> The cases that complete with a query >> result are not case insensitive right now. This affects things like >> >> UPDATE T<tab> >> >> 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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2021-04-01T09:40:50Z
On Wednesday, March 31, 2021 4:05 AM, David Zhang <david.zhang@highgo.ca> 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
-
Re: Support tab completion for upper character inputs in psql
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-04-08T07:13:44Z
On 01.04.21 11:40, tanghy.fnst@fujitsu.com wrote: > On Wednesday, March 31, 2021 4:05 AM, David Zhang <david.zhang@highgo.ca> 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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2021-04-14T13:34:11Z
On Thursday, April 8, 2021 4:14 PM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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
-
Re: Support tab completion for upper character inputs in psql
Peter Smith <smithpb2250@gmail.com> — 2021-04-21T04:23:43Z
On Wed, Apr 14, 2021 at 11:34 PM tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote: > > On Thursday, April 8, 2021 4:14 PM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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 FROM" Why not use strnicmp for case insensitive compare here instead of strdup'ing another string (and not freeing it)? Or maybe use pg_strncasecmp. ====== 6. byte_length == 0? @@ -4556,7 +4582,16 @@ _complete_from_query(const char *simple_query, while (list_index < PQntuples(result) && (item = PQgetvalue(result, list_index++, 0))) if (pg_strncasecmp(text, item, byte_length) == 0) - return pg_strdup(item); + { + if (byte_length == 0 || completion_case_sensitive) + return pg_strdup(item); + else + /* + * If case insensitive matching was requested initially, + * adjust the case according to setting. + */ + return pg_strdup_keyword_case(item, text); + } } The byte_length was not being checked before, so why is the check needed now? ====== 7. test typo "ralation" +# check query command completion for upper character ralation name +check_completion("update TAB1 SET \t", qr/update TAB1 SET \af/, "complete column name for TAB1"); ====== 8. test typo "case-insensitiveq" +# check schema query(upper case) which is case-insensitiveq +check_completion("select oid from Pg_cla\t", qq/select oid from Pg_cla\b\b\b\b\bG_CLASS /, "complete schema query with uppper case string"); ------ Kind Regards, Peter Smith. Fujitsu Australia -
RE: Support tab completion for upper character inputs in psql
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2021-04-22T12:43:42Z
On Wednesday, April 21, 2021 1:24 PM, Peter Smith <smithpb2250@gmail.com> 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
-
Re: Support tab completion for upper character inputs in psql
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-04-23T02:58:12Z
At Thu, 22 Apr 2021 12:43:42 +0000, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> wrote in > On Wednesday, April 21, 2021 1:24 PM, Peter Smith <smithpb2250@gmail.com> 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<tab> 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 <tab> 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
Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-23T03:17:19Z
Kyotaro Horiguchi <horikyota.ntt@gmail.com> 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
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-04-23T03:25:36Z
At Fri, 23 Apr 2021 11:58:12 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > Any further comment is very welcome. Oh, I accidentally found a doubious behsbior. =# alter table public.<tab> public.c1 public.d1 public."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."<tab> =# alter table public."t -- candidates are "t" and "tt"? =# alter table public."tt<tab> -- nothing happenes =# alter table public."TT<tab> -- also nothing happenes regards. -- Kyotaro Horiguchi NTT Open Source Software Center -
Re: Support tab completion for upper character inputs in psql
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-04-23T03:34:07Z
At Thu, 22 Apr 2021 23:17:19 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in > Kyotaro Horiguchi <horikyota.ntt@gmail.com> 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
Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-23T04:17:35Z
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes: > At Thu, 22 Apr 2021 23:17:19 -0400, Tom Lane <tgl@sss.pgh.pa.us> 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
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-04-23T05:44:43Z
FWIW... At Fri, 23 Apr 2021 00:17:35 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes: > > At Thu, 22 Apr 2021 23:17:19 -0400, Tom Lane <tgl@sss.pgh.pa.us> 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
Laurenz Albe <laurenz.albe@cybertec.at> — 2021-04-23T08:33:39Z
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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2021-04-26T13:47:13Z
Hi I've updated the patch to V7 based on the following comments. On Friday, April 23, 2021 11:58 AM, Kyotaro Horiguchi <horikyota.ntt@gmail.com> 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<tab> / SET Byt<tab> 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 <horikyota.ntt@gmail.com> wrote >Oh, I accidentally found a doubious behsbior. > >=# alter table public.<tab> >public.c1 public.d1 public."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 -
RE: Support tab completion for upper character inputs in psql
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2021-06-23T12:43:53Z
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.144443.2058612313278551429.horikyota.ntt%40gmail.com https://www.postgresql.org/message-id/a75a6574c0e3d4773ba20a73d493c2c9983c0657.camel%40cybertec.at Regards, Tang
-
Re: Support tab completion for upper character inputs in psql
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-09-07T08:25:20Z
On 23.06.21 14:43, tanghy.fnst@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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2021-09-10T13:50:31Z
On Tuesday, September 7, 2021 5:25 PM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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.144443.2058612313278551429.horikyota.ntt%40gmail.com [3] https://www.postgresql.org/message-id/a75a6574c0e3d4773ba20a73d493c2c9983c0657.camel%40cybertec.at Regards, Tang -
Re: Support tab completion for upper character inputs in psql
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-01-06T07:46:48Z
On 10.09.21 15:50, tanghy.fnst@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
Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-06T14:56:36Z
Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2022-01-07T02:12:23Z
On Thursday, January 6, 2022 11:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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
-
Re: Support tab completion for upper character inputs in psql
Japin Li <japinli@hotmail.com> — 2022-01-07T04:08:17Z
On Fri, 07 Jan 2022 at 10:12, tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote: > On Thursday, January 6, 2022 11:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2022-01-07T05:17:21Z
On Friday, January 7, 2022 1:08 PM, Japin Li <japinli@hotmail.com> 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 -
Re: Support tab completion for upper character inputs in psql
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-01-13T11:29:54Z
On 07.01.22 06:17, tanghy.fnst@fujitsu.com wrote: > On Friday, January 7, 2022 1:08 PM, Japin Li <japinli@hotmail.com> 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-500efaf23b25@enterprisedb.com -
Re: Support tab completion for upper character inputs in psql
Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-15T18:51:26Z
Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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<TAB> immediately completes mytab, ignoring mixedTab (b) "m<TAB> immediately completes "mixedTab", ignoring mytab (c) "my<TAB> fails to find anything (d) mi<TAB> fails to find anything (e) M<TAB> 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<TAB> fails to find anything, which seems like a POLA violation given that mY<TAB> 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
Julien Rouhaud <rjuju123@gmail.com> — 2022-01-19T08:59:20Z
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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2022-01-20T07:37:18Z
On Sunday, January 16, 2022 3:51 AM, Tom Lane <tgl@sss.pgh.pa.us> said: > Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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<TAB> immediately completes mytab, ignoring mixedTab > > (b) "m<TAB> immediately completes "mixedTab", ignoring mytab > > (c) "my<TAB> fails to find anything > > (d) mi<TAB> fails to find anything > > (e) M<TAB> 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<TAB> fails to find anything, which seems > like a POLA violation given that mY<TAB> 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
-
Re: Support tab completion for upper character inputs in psql
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-01-24T09:35:36Z
On 20.01.22 08:37, tanghy.fnst@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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2022-01-25T05:22:32Z
On Monday, January 24, 2022 6:36 PM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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 -
Re: Support tab completion for upper character inputs in psql
Julien Rouhaud <rjuju123@gmail.com> — 2022-01-25T09:43:50Z
Hi, On Tue, Jan 25, 2022 at 05:22:32AM +0000, tanghy.fnst@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
Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-25T23:11:27Z
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.
-
RE: Support tab completion for upper character inputs in psql
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2022-01-26T01:46:01Z
On Tuesday, January 25, 2022 6:44 PM, Julien Rouhaud <rjuju123@gmail.com> 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
Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-27T20:23:42Z
I wrote: > 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. Here's a fleshed-out patch series for this idea. 0001 below is a more evolved version of my previous WIP patch. The main thing that's changed is that I found that it no longer works to handle extra keywords (such as adding COLUMN to the list of attribute names after ALTER TABLE tab RENAME) via tab-complete's traditional method of sticking on "UNION SELECT 'foo'". That's because such a result row looks like something that needs to be double-quoted, which of course makes the completion incorrect. So I've created a side-channel whereby _complete_from_query() can return some verbatim keywords alongside the actual query results. (I think this is a good thing anyway, because the UNION method is incredibly wasteful of server cycles. Yeah, I know that these queries only need to run at human speed, but if your server is heavily loaded you might still not appreciate the extra cycles.) Having done that, I solved the format_type problem by just dropping the use of format_type altogether, and returning only the normal pg_type.typname entries. The only cases where format_type did anything useful for us are the small number of built-in types where it substitutes a SQL-mandated name, and we can treat those like keywords (as indeed they are). The list of such types changes seldom enough that I don't think it's a huge maintenance burden to have one more place that knows about them. BTW, I was amused to notice that many of the format_type special cases don't actually work as completions, and never have. For example, if we return both "timestamp with time zone" and "timestamp without time zone", readline can complete as far as "timestamp with", but further completion fails because "timestamp" is now seen as a previous word that's not part of what's to be completed. So I've dropped those cases from the keyword list. Maybe somebody will get interested in figuring a way to make that work, but IMO the cost/benefit ratio for such effort would be pretty bad. Incidentally, I found that some of the completion queries were intentionally ignoring the given prefix text, with stuff like /* the silly-looking length condition is just to eat up the current word */ " WHERE ... (%d = pg_catalog.length('%s'))" I'm not sure why we ever thought that was a good idea, but it definitely doesn't work anymore, since I removed the filtering that _complete_from_query() used to do on the query results. It's now incumbent on the queries to only return valid matches, so I replaced all instances of this pattern with the regular substring() checks, or even added a substring() check in a couple of queries where there was nothing at all. 0001 takes care of quoting and case-folding issues for the actual subject name of a completion operation, but there's more to do. A lot of queries have to reference a previously-entered name (for example, ALTER TABLE tab1 DROP COLUMN <TAB> has to find the column names of table tab1), and we had variously shoddy code for dealing with those names. Only a few queries even attempted to handle schema-qualified names, and none at all of them would downcase unquoted names. So 0002 tries to fix that up, using the same code to parse/downcase/de-quote the name as we would use if it were the subject text. It didn't take long to find that the existing methods for this were incredibly tedious, requiring near-duplicate queries depending on whether the previous name was schema-qualified or not. So I've extended the SchemaQuery mechanism to support adding qualifications based on an additional name, and now we use that wherever we need a possibly-schema-qualified previous name. A couple of the existing queries of this sort used WHERE oid IN (sub-SELECT), which I didn't see a great way to jam into the SchemaQuery mechanism. What I've done here is to convert those semijoins into plain joins, which might yield multiple instances of wanted names, and then stick DISTINCT onto the queries. It's not very pretty, but it works fine. In 0001 and 0002, I left the core of _complete_from_query() un-reindented, in hopes of making the actual code changes more readily reviewable. 0003 is just an application of pgindent to fix that up and make the finished code legible again. Finally, 0004 adds some test cases. I'm not too confident about how portable these will be, but I don't think they are making any assumptions the existing tests didn't make already. They do pass for me on Linux (readline 7.0) and macOS (Apple's libedit). This is sufficiently invasive to tab-complete.c that I'd like to get it pushed fairly soon, before that code changes under me. Thoughts? regards, tom lane -
RE: Support tab completion for upper character inputs in psql
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2022-01-28T06:37:07Z
On Friday, January 28, 2022 5:24 AM, Tom Lane <tgl@sss.pgh.pa.us> 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
Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-28T16:03:16Z
"tanghy.fnst@fujitsu.com" <tanghy.fnst@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<TAB> 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<TAB> --- completes immediately to id alter table atest rename I<TAB> --- 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<TAB> 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
Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-28T21:25:57Z
I wrote: > 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<TAB> to complete > immediately to id, which is arguably fine. Here's a patch series that does it like that. I have to admit that after playing with it, this is probably better. There's less magic-looking behavior involved, and it lets me drop an ugly hack I had to work around a case where Readline didn't want to play along. 0001 also cleans up one oversight in the previous version, which is to beware of multibyte characters in parse_identifier(). I'm not sure there is any actual hazard there, since we weren't looking for backslashes, but it's better to be sure. I added the keyword handling I'd left out before, too. 0002-0004 are largely as before. I've also added 0005, which changes the prefix-matching clauses in the SQL queries from "substring(foo,1,%d)='%s'" to "foo LIKE '%s'". This simplifies reading the queries a little bit, but the real reason to do it is that the planner can optimize the catalog searches a lot better. It knows a lot about LIKE prefix queries and exactly nothing about substring(). For example, DROP TYPE foo<TAB> now produces a query like this: explain SELECT t.typname, NULL::pg_catalog.text FROM pg_catalog.pg_type t WHERE (t.typrelid = 0 OR (SELECT c.relkind = 'c' FROM pg_catalog.pg_class c WHERE c.oid = t.typrelid)) AND t.typname !~ '^_' AND (t.typname) LIKE 'foo%' AND pg_catalog.pg_type_is_visible(t.oid); QUERY PLAN ------------------------------------------------------------------------------------------------------------------------------------------ Index Scan using pg_type_typname_nsp_index on pg_type t (cost=0.28..16.63 rows=1 width=96) Index Cond: ((typname >= 'foo'::text) AND (typname < 'fop'::text)) Filter: ((typname !~ '^_'::text) AND (typname ~~ 'foo%'::text) AND pg_type_is_visible(oid) AND ((typrelid = '0'::oid) OR (SubPlan 1))) SubPlan 1 -> Index Scan using pg_class_oid_index on pg_class c (cost=0.28..8.30 rows=1 width=1) Index Cond: (oid = t.typrelid) (6 rows) where before you got a seqscan: explain SELECT pg_catalog.format_type(t.oid, NULL) FROM pg_catalog.pg_type t WHERE (t.typrelid = 0 OR (SELECT c.relkind = 'c' FROM pg_catalog.pg_class c WHERE c.oid = t.typrelid)) AND t.typname !~ '^_' AND substring(pg_catalog.format_type(t.oid, NULL),1,3)='foo' AND pg_catalog.pg_type_is_visible(t.oid); QUERY PLAN ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- Seq Scan on pg_type t (cost=0.00..16691.86 rows=1 width=32) Filter: ((typname !~ '^_'::text) AND ("substring"(format_type(oid, NULL::integer), 1, 3) = 'foo'::text) AND pg_type_is_visible(oid) AND ((typrelid = '0'::oid) OR (SubPlan 1))) SubPlan 1 -> Index Scan using pg_class_oid_index on pg_class c (cost=0.28..8.30 rows=1 width=1) Index Cond: (oid = t.typrelid) (5 rows) Again, while these queries only have to run at human speed, that doesn't mean it's okay to be wasteful. I seem to recall hearing complaints that they are noticeably slow in installations with many thousand tables, too. This should help. regards, tom lane -
Re: Support tab completion for upper character inputs in psql
Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-28T22:16:32Z
I wrote: > [ v15 patch set ] Sigh ... per the cfbot, this was already blindsided by 95787e849. As I said, I don't want to sit on this for very long. regards, tom lane
-
RE: Support tab completion for upper character inputs in psql
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2022-01-30T07:07:17Z
On Saturday, January 29, 2022 1:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "tanghy.fnst@fujitsu.com" <tanghy.fnst@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<TAB> 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<TAB> --- completes immediately to id > alter table atest rename I<TAB> --- 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<TAB> 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.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2022-01-30T07:13:40Z
On Saturday, January 29, 2022 7:17 AM, Tom Lane <tgl@sss.pgh.pa.us> 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
Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-30T18:34:39Z
"tanghy.fnst@fujitsu.com" <tanghy.fnst@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
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-01-31T01:28:45Z
Tom Lane <tgl@sss.pgh.pa.us> writes: > "tanghy.fnst@fujitsu.com" <tanghy.fnst@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
-
Re: Support tab completion for upper character inputs in psql
Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-31T01:54:57Z
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> 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<TAB> 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 <tgl@sss.pgh.pa.us> — 2022-01-31T21:39:01Z
I wrote: > =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> 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
-
Re: Support tab completion for upper character inputs in psql
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-02-01T13:15:45Z
Tom Lane <tgl@sss.pgh.pa.us> writes: > I wrote: >> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> 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
-
Re: Support tab completion for upper character inputs in psql
Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-01T22:06:58Z
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes: > Tom Lane <tgl@sss.pgh.pa.us> 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 <tgl@sss.pgh.pa.us> — 2022-02-04T23:41:21Z
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes: > Tom Lane <tgl@sss.pgh.pa.us> 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<TAB><TAB> 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<TAB>" 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
tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2022-07-22T08:30:48Z
On Monday, January 31, 2022 3:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "tanghy.fnst@fujitsu.com" <tanghy.fnst@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
-
Re: Support tab completion for upper character inputs in psql
Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-22T14:55:27Z
"tanghy.fnst@fujitsu.com" <tanghy.fnst@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