Re: psql completion for ids in multibyte string

Hello, thank you for the comments.

At Wed, 2 Mar 2016 10:10:55 +1300, Thomas Munro <thomas.munro@enterprisedb.com> wrote in
<CAEepm=2JjPY-v1JWWrJyBone-=t1A7TJRyKSfaseQLzh5sMQGg@mail.gmail.com>
> On Wed, Mar 2, 2016 at 7:54 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Mon, Feb 29, 2016 at 6:32 PM, Thomas Munro
> > <thomas.munro@enterprisedb.com> wrote:
> >> On Fri, Feb 26, 2016 at 6:34 PM, Kyotaro HORIGUCHI
> >> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >>> Hello, this is the second patch plitted out. This allows
> >>> multibyte names to be completed in psql.
> >>>
> >>> At Fri, 06 Nov 2015 11:47:17 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
wrotein <20151106.114717.170526268.horiguchi.kyotaro@lab.ntt.co.jp>
 
> >>>> At Thu, 5 Nov 2015 18:32:59 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<563B224B.3020400@lab.ntt.co.jp>
> >>>> > On 2015/11/05 18:10, Kyotaro HORIGUCHI wrote:
> >>>> > > Hello. I don't know whether this is a bug fix or improvement,
> >>>> >
> >>>> > Would it be 50-50? :-)
> >>>>
> >>>> Yeah, honestly saying, I doubt that this is valuable but feel
> >>>> uneasy to see some of the candidates vanish as completon proceeds
> >>>> for no persuasive reason.
> >>
> >> +1 from me, it's entirely reasonable to want to name database objects
> >> in any human language and use auto-completion.  It's not working today
> >> as you showed.
> >>
> >>> The current version of tab-completion failed to complete names
> >>> with multibyte characters.
> >>>
> >>> =# create table いろは (あ int);
> >>> =# create table いこい (あ int);
> >>> =# drop table <tab>
> >>> "いろは"             hint_plan.           pg_toast.
> >>> "いこい"             information_schema.  pg_toast_temp_1.
> >>> ALL IN TABLESPACE    pg_catalog.          public.
> >>> dbms_stats.          pg_temp_1.
> >>> postgres=# alter table "い
> >>> =# drop table "い<tab>
> >>> =# drop table "い           /* No candidate offered */
> >>>
> >>> This is because _complet_from_query counts the string length in
> >>> bytes, instead of characters. With this patch the candidates will
> >>> appear.
> >>>
> >>> =# drop table "い<tab>
> >>> "いこい"  "いろは"
> >>> postgres=# drpo table "い
> >>
> >> The patch looks correct to me: it counts characters rather than bytes,
> >> which is the right thing to do because the value is passed to substr()
> >> which also works in characters rather than bytes.  I tested with
> >> "éclair", and without the patch, tab completion doesn't work if you
> >> press tab after 'é'.  With the patch it does.
> >
> > OK, but I am a little concerned about this code:
> >
> >     /* Find something that matches */
> >     if (result && PQresultStatus(result) == PGRES_TUPLES_OK)
> >     {
> >         const char *item;
> >
> >         while (list_index < PQntuples(result) &&
> >                (item = PQgetvalue(result, list_index++, 0)))
> >             if (pg_strncasecmp(text, item, string_length) == 0)
> >                 return pg_strdup(item);
> >     }
> >
> > Every other use of string_length in this function is using it as the
> > argument to the SQL substring function, which cares about characters,
> > not bytes.  But this use seems to care about bytes, not characters.
> >
> > Am I wrong?
> 
> Ugh, and the other problem is that string_length is always 0 there if
> state isn't 0 (in master it is static so that the value is reused for
> subsequent calls, but this patch made it automatic).

Thanks for pointing it out.

> I think we should leave string_length as it is and use a new variable
> for character-based length, as in the attached.

Basically agreed but I like byte_length for the previous
string_length and string_length for string_length_cars. Also
text_length is renamed in the attached patch.

What do you think about this?

# I named it as version 3 at my own decision.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5f27120..ed92912 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3198,9 +3198,8 @@ static char *_complete_from_query(int is_schema_query, const char *text, int state){    static
int   list_index,
 
-                string_length;
+                byte_length;    static PGresult *result = NULL;
-    /*     * If this is the first time for this completion, we fetch a list of our     * "things" from the backend.
@@ -3211,9 +3210,18 @@ _complete_from_query(int is_schema_query, const char *text, int state)        char
*e_text;       char       *e_info_charp;        char       *e_info_charp2;
 
+        const char *pstr = text;
+        int            string_length = 0;        list_index = 0;
-        string_length = strlen(text);
+        byte_length = strlen(text);
+
+        /* Count length as number of characters (not bytes), for passing to substring */
+        while (*pstr)
+        {
+            string_length++;
+            pstr += PQmblen(pstr, pset.encoding);
+        }        /* Free any prior result */        PQclear(result);
@@ -3353,7 +3361,7 @@ _complete_from_query(int is_schema_query, const char *text, int state)        while (list_index <
PQntuples(result)&&               (item = PQgetvalue(result, list_index++, 0)))
 
-            if (pg_strncasecmp(text, item, string_length) == 0)
+            if (pg_strncasecmp(text, item, byte_length) == 0)                return pg_strdup(item);    }
@@ -3372,7 +3380,7 @@ _complete_from_query(int is_schema_query, const char *text, int state)static char
*complete_from_list(constchar *text, int state){
 
-    static int    string_length,
+    static int    byte_length,                list_index,                matches;    static bool casesensitive;
@@ -3385,7 +3393,7 @@ complete_from_list(const char *text, int state)    if (state == 0)    {        list_index = 0;
-        string_length = strlen(text);
+        byte_length = strlen(text);        casesensitive = completion_case_sensitive;        matches = 0;    }
@@ -3393,14 +3401,14 @@ complete_from_list(const char *text, int state)    while ((item =
completion_charpp[list_index++]))   {        /* First pass is case sensitive */
 
-        if (casesensitive && strncmp(text, item, string_length) == 0)
+        if (casesensitive && strncmp(text, item, byte_length) == 0)        {            matches++;            return
pg_strdup(item);       }        /* Second pass is case insensitive, don't bother counting matches */
 
-        if (!casesensitive && pg_strncasecmp(text, item, string_length) == 0)
+        if (!casesensitive && pg_strncasecmp(text, item, byte_length) == 0)        {            if
(completion_case_sensitive)               return pg_strdup(item);
 
@@ -3627,13 +3635,13 @@ pg_strdup_keyword_case(const char *s, const char *ref)static char *escape_string(const char
*text){
-    size_t        text_length;
+    size_t        byte_length;    char       *result;
-    text_length = strlen(text);
+    byte_length = strlen(text);
-    result = pg_malloc(text_length * 2 + 1);
-    PQescapeStringConn(pset.db, result, text, text_length, NULL);
+    result = pg_malloc(byte_length * 2 + 1);
+    PQescapeStringConn(pset.db, result, text, byte_length, NULL);    return result;}