On Wed, Apr 14, 2021 at 11:34 PM tanghy.f...@fujitsu.com
<tanghy.f...@fujitsu.com> wrote:
>
> On Thursday, April 8, 2021 4:14 PM, Peter Eisentraut
> <peter.eisentr...@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
