Re: Support tab completion for upper character inputs in psql - Mailing list pgsql-hackers

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