Thread: IF (NOT) EXISTS in psql-completion

Hello,

I considered how to make tab-completion robust for syntactical
noises, in other words, optional words in syntax. Typically "IF
(NOT) EXISTS", UNIQUE and TEMPORARY are words that don't affect
further completion. However, the current delimit-matching
mechanism is not so capable (or is complexty-prone) to live with
such noises. I have proposed to use regular expressions or
simplified one for the robustness but it was too complex to be
applied.

This is another answer for the problem. Removal of such words
on-the-fly makes further matching more robust.

Next, currently some CREATE xxx subsyntaxes of CREATE SCHEMA are
matched using TailMatching but it makes difficult the
options-removal operations, which needs forward matching.

So I introduced two things to resolve them by this patch.

1. HEAD_SHIFT macro
 It shifts the beginning index in previous_words for *MatchN macros. When the varialbe is 3 and previous_words is as
following,
 {"NOT", "IF", "CONCURRENTLY", "INDEX", "UNIQUE", "CREATE"}
 Match3("CONCURRENTLY", "IF", "NOT") reutrns true. HeadMatches and TailMatches works on the same basis. This allows us
tomatch "CREATE xxx" subsyntaxes of CREATE SCHEMA independently. SHIFT_TO_LAST1() macro finds the last appearance of
specifiedword and HEAD_SHIFT to there if found.
 

2. MidMatchAndRemoveN() macros
 These macros remove specified words starts from specfied number of words after the current beginning. Having
head_shift= 0 and the following previous_words,
 
 {"i_t1_a", "EXISTS", "IF", "INDEX", "DROP"}
 MidMatchAndRemove2(2, "IF", "EXISTS") leaves the following previous_words.
 {"i_t1_a", "INDEX", "DROP"}


Using these things, the patch as whole does the following things.

A. Allows "IF (NOT) EXISTS" at almost everywhere it allowed  syntactically.
  The boilerplate is like the following,
  | else if (MatchesN(words before IF EXISTS))  |      COMPLETE_WITH_XXX(... "UNION SELECT 'IF EXISTS'");  | else if
(HeadMatchesN(wordsbefore "IF EXISTS") &&  |          MidMatchAndRemoveM(N, words to be removed) &&  |
MatchesN(wordsbefore "IF EXISTS"))  |      COMPLETE_WITH_XXXX();
 
  The first "else if" makes suggestion for the 'thing' with "IF  EXISTS".
  The MidMatchAndRemoveM in the second "else if" actually  removes "IF EXISTS" if match and the third MatchesN or all
matchingmacros ever after don't see the removed words.  So  they can make a suggestion without seeing such noises.
 
  This looks a bit hackery but works well in the current  framework.


B. Masks the part in CREATE SCHEMA unrelated to the last CREATE subsyntax currently focused on.
  |    else if (HeadMatches2("CREATE", "SCHEMA") &&  |             SHIFT_TO_LAST1("CREATE") &&  |             false) {}
/*FALL THROUGH */
 
  The result of this, for the query like this,
   CREATE SCHEMA foo bar baz CREATE foe fee CREATE hoge hage
  all the following part of psql_completion works as if the  current query is just "CREATE hoge hage".


Does anybody have suggestions, opinions or objections?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 05.02.2016 11:09, Kyotaro HORIGUCHI wrote:
> Hello,
>
> I considered how to make tab-completion robust for syntactical
> noises, in other words, optional words in syntax. Typically "IF
> (NOT) EXISTS", UNIQUE and TEMPORARY are words that don't affect
> further completion. However, the current delimit-matching
> mechanism is not so capable (or is complexty-prone) to live with
> such noises. I have proposed to use regular expressions or
> simplified one for the robustness but it was too complex to be
> applied.
>
> This is another answer for the problem. Removal of such words
> on-the-fly makes further matching more robust.
>
> Next, currently some CREATE xxx subsyntaxes of CREATE SCHEMA are
> matched using TailMatching but it makes difficult the
> options-removal operations, which needs forward matching.
>
> So I introduced two things to resolve them by this patch.
>

I did some tests with your patch. But I am not confident in tab-complete.c.

And I have some notes:

1 - I execute git apply command and get the following warning:

../0001-Suggest-IF-NOT-EXISTS-for-tab-completion-of-psql.patch:302: 
trailing whitespace./*
warning: 1 line adds whitespace errors.

This is because of superfluous whitespace I think.

2 - In psql I write "create table if" and press <TAB>. psql adds the 
following:

create table IF NOT EXISTS

I think psql should continue with lower case if user wrote query with 
loser case text:

create table if not exists

3 - Same with "IF EXISTS". If a write "alter view if" and press <TAB> 
psql writes:

alter view IF EXISTS

-- 
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Hello, thank you for the comments.

At Mon, 15 Feb 2016 15:43:57 +0300, Artur Zakirov <a.zakirov@postgrespro.ru> wrote in
<56C1C80D.7020101@postgrespro.ru>
> On 05.02.2016 11:09, Kyotaro HORIGUCHI wrote:
> > Hello,
> >
> > I considered how to make tab-completion robust for syntactical
> > noises, in other words, optional words in syntax. Typically "IF
> > (NOT) EXISTS", UNIQUE and TEMPORARY are words that don't affect
> > further completion. However, the current delimit-matching
> > mechanism is not so capable (or is complexty-prone) to live with
> > such noises. I have proposed to use regular expressions or
> > simplified one for the robustness but it was too complex to be
> > applied.
> >
> > This is another answer for the problem. Removal of such words
> > on-the-fly makes further matching more robust.
> >
> > Next, currently some CREATE xxx subsyntaxes of CREATE SCHEMA are
> > matched using TailMatching but it makes difficult the
> > options-removal operations, which needs forward matching.
> >
> > So I introduced two things to resolve them by this patch.
> >
> 
> I did some tests with your patch. But I am not confident in
> tab-complete.c.
> 
> And I have some notes:
> 
> 1 - I execute git apply command and get the following warning:
> 
> ../0001-Suggest-IF-NOT-EXISTS-for-tab-completion-of-psql.patch:302:
> trailing whitespace.
>     /*
> warning: 1 line adds whitespace errors.
> 
> This is because of superfluous whitespace I think.

Oops. I'll remove it.

> 2 - In psql I write "create table if" and press <TAB>. psql adds the
> following:
> 
> create table IF NOT EXISTS
> 
> I think psql should continue with lower case if user wrote query with
> loser case text:

Good catch! Hmm. COMPLETE_WITH_SCHEMA_QUERY() does it. For
example, the following existing completion behaves in the same
way.

"drop index c" =<tab>=> "drop index CONCURRENTLY"

The results of schema queries should be treated in case-sensitive
way so the additional keywords by 'UNION' are also treated
case-sensitively.
   COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes,                  " UNION SELECT 'CONCURRENTLY'");

Fixing this is another problem. So I'd like to leave this alone
here.

> create table if not exists
> 
> 3 - Same with "IF EXISTS". If a write "alter view if" and press <TAB>
> psql writes:
> 
> alter view IF EXISTS

regards,

-- 
堀口恭太郎

日本電信電話株式会社 NTTオープンソースソフトウェアセンタ
Phone: 03-5860-5115 / Fax: 03-5463-5490

Hello, this is the new version of this patch.

At Fri, 26 Feb 2016 13:17:26 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160226.131726.54224488.horiguchi.kyotaro@lab.ntt.co.jp>
> Hello, thank you for the comments.
> 
> At Mon, 15 Feb 2016 15:43:57 +0300, Artur Zakirov <a.zakirov@postgrespro.ru> wrote in
<56C1C80D.7020101@postgrespro.ru>
> > I did some tests with your patch. But I am not confident in
> > tab-complete.c.
> > 
> > And I have some notes:
> > 
> > 1 - I execute git apply command and get the following warning:
> > 
> > ../0001-Suggest-IF-NOT-EXISTS-for-tab-completion-of-psql.patch:302:
> > trailing whitespace.
> >     /*
> > warning: 1 line adds whitespace errors.
> > 
> > This is because of superfluous whitespace I think.
> 
> Oops. I'll remove it.
> 
> > 2 - In psql I write "create table if" and press <TAB>. psql adds the
> > following:
> > 
> > create table IF NOT EXISTS
> > 
> > I think psql should continue with lower case if user wrote query with
> > loser case text:
> 
> Good catch! Hmm. COMPLETE_WITH_SCHEMA_QUERY() does it. For
> example, the following existing completion behaves in the same
> way.
> 
> "drop index c" =<tab>=> "drop index CONCURRENTLY"
> 
> The results of schema queries should be treated in case-sensitive
> way so the additional keywords by 'UNION' are also treated
> case-sensitively.
> 
>     COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes,
>                    " UNION SELECT 'CONCURRENTLY'");
> 
> Fixing this is another problem. So I'd like to leave this alone
> here.
> 
> > create table if not exists
> > 
> > 3 - Same with "IF EXISTS". If a write "alter view if" and press <TAB>
> > psql writes:
> > 
> > alter view IF EXISTS

Finally, the only thing done in this new patch is removing one
dangling space.

reagrds,


-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Feb 26, 2016 at 2:37 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Hello, this is the new version of this patch.

The CommitFest entry for this patch is messed up.  It shows no author,
even though I'm pretty sure that a patch has to have one by
definition.

https://commitfest.postgresql.org/9/518/

Also, this patch was listed as Waiting on Author, but it's been
updated since it was last reviewed, so I switched it back to Needs
Review.  Can someone please review it and, if appropriate, mark it
Ready for Committer?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/15/16 1:42 PM, Robert Haas wrote:
> On Fri, Feb 26, 2016 at 2:37 AM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> Hello, this is the new version of this patch.
> 
> The CommitFest entry for this patch is messed up.  It shows no author,
> even though I'm pretty sure that a patch has to have one by
> definition.
> 
> https://commitfest.postgresql.org/9/518/
> 
> Also, this patch was listed as Waiting on Author, but it's been
> updated since it was last reviewed, so I switched it back to Needs
> Review.  Can someone please review it and, if appropriate, mark it
> Ready for Committer?

Author has been set to Kyotaro Horiguchi.

-- 
-David
david@pgmasters.net

On 2/5/16 3:09 AM, Kyotaro HORIGUCHI wrote:
> I considered how to make tab-completion robust for syntactical
> noises, in other words, optional words in syntax. Typically "IF
> (NOT) EXISTS", UNIQUE and TEMPORARY are words that don't affect
> further completion.

To repeat the question I raised in the previous commit fest about tab
completion: Why do you want tab completion for IF NOT EXISTS?  When you
tab complete, the completion mechanism will show you whether the item in
question exists.  What is the use case?

Hello,

# It seems that I have been forgotten in the recepient list..

At Tue, 15 Mar 2016 22:09:59 -0400, Peter Eisentraut <peter_e@gmx.net> wrote in <56E8C077.2000903@gmx.net>
> On 2/5/16 3:09 AM, Kyotaro HORIGUCHI wrote:
> > I considered how to make tab-completion robust for syntactical
> > noises, in other words, optional words in syntax. Typically "IF
> > (NOT) EXISTS", UNIQUE and TEMPORARY are words that don't affect
> > further completion.
> 
> To repeat the question I raised in the previous commit fest about tab
> completion: Why do you want tab completion for IF NOT EXISTS?  When you
> tab complete, the completion mechanism will show you whether the item in
> question exists.  What is the use case?

Ah, I think I understand you question. It's not about IF EXISTS,
but only IF NOT EXSTS. It is needed when repeated execution of
the same SQL statement will be done using command line
history. Such stocks of commands in history is often
convenient. And sometimes I rely on psql-completion to write a
SQL script. The completions for such words seemingly useless on
instant-execution will be handy to do that.

Another thing I want to do by this patch is that we can get
completion even after such optional words. I have been annoyed
many times by this. Some of them, such as UNIQUE, TEMPORARY and
CONCURRENTLY are treated but they just doubles the matching
condition expressions.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Mmm. Have I broken the entry?

At Tue, 15 Mar 2016 13:55:24 -0400, David Steele <david@pgmasters.net> wrote in <56E84C8C.7060902@pgmasters.net>
> On 3/15/16 1:42 PM, Robert Haas wrote:
> > On Fri, Feb 26, 2016 at 2:37 AM, Kyotaro HORIGUCHI
> > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >> Hello, this is the new version of this patch.
> > 
> > The CommitFest entry for this patch is messed up.  It shows no author,
> > even though I'm pretty sure that a patch has to have one by
> > definition.
> > 
> > https://commitfest.postgresql.org/9/518/
> > 
> > Also, this patch was listed as Waiting on Author, but it's been
> > updated since it was last reviewed, so I switched it back to Needs
> > Review.  Can someone please review it and, if appropriate, mark it
> > Ready for Committer?
> 
> Author has been set to Kyotaro Horiguchi.

Thank you for repairing it.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Mar 16, 2016 at 2:42 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Feb 26, 2016 at 2:37 AM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> Hello, this is the new version of this patch.
>
> The CommitFest entry for this patch is messed up.  It shows no author,
> even though I'm pretty sure that a patch has to have one by
> definition.

The CF app does not require an author name when the entry is first
created. The author needs to be added afterwards. A message-id, a
description and a category are the mandatory things.
-- 
Michael

Hi Kyotaro,

On 3/18/16 3:22 AM, Pavel Stehule wrote:

>     I am looking this patch. It looks well, but this feature doesn't
>     respect upper or lower chars. It enforce upper chars. This is not
>     consistent with any other autocomplete.
> 
> 
> I checked it against sql help and these statements doesn't work
> 
> alter foreign table hhhh drop column
> alter text search configuration jjj drop mapping
> alter type hhh drop attribute
> drop cast
> drop extension
> drop operator
> drop text search
> drop transform -- missing autocomplete completely
> drop user mapping
> alter table jjj add column
> create temp sequence
> create sequence

Do you have an idea of when you will have a new patch ready?

Thanks,
-- 
-David
david@pgmasters.net

Thank you Pavel, David.

Thank you for pointing syntaxes to be addressed. Most of the are
addressed in the attached patch.


At Tue, 22 Mar 2016 12:57:27 -0400, David Steele <david@pgmasters.net> wrote in <56F17977.8040503@pgmasters.net>
> Hi Kyotaro,
> 
> On 3/18/16 3:22 AM, Pavel Stehule wrote:
> 
> >     I am looking this patch. It looks well, but this feature doesn't
> >     respect upper or lower chars. It enforce upper chars. This is not
> >     consistent with any other autocomplete.

As mentioned before, upper-lower problem is an existing
issue. The case of the words in a query result list cannot be
edited since it may contain words that should not be changed,
such as relation names. So we can address it only before issueing
a query but I haven't found simple way to do it.

> > I checked it against sql help and these statements doesn't work

Thank you very much.

> > alter foreign table hhhh drop column
> > drop cast
> > drop operator
> > drop transform -- missing autocomplete completely

These are not done. Each of them has issues to be addressed
before adding completion of IF EXISTS.

> > alter text search configuration jjj drop mapping
> > alter type hhh drop attribute
> > drop extension

Done.

> > drop text search

I don't see the syntax "drop text search [if exists]".  drop text
search (configuration|dictionary|parser|template) are already
addressed.

> > drop user mapping

"drop user" was not completed with "mapping". I added it then
addressed this. (This might be another issue.)

> > alter table jjj add column

Done if it is mentioning DROP COLUMN. But new two macros
HeadMatches6 and 7 are introduced together.   
> > create temp sequence
> > create sequence

DROP SEQUENCE is already completed with IF EXISTS. CREATE [TEMP]
SEQUENCE with IF NOT EXISTS is added.

> Do you have an idea of when you will have a new patch ready?

Sorry to to have been late. The attached is the revised version.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 29.03.2016 10:59, Pavel Stehule wrote:
> Hi
>
> 2016-03-29 8:43 GMT+02:00 Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp <mailto:horiguchi.kyotaro@lab.ntt.co.jp>>:
>
>     Thank you Pavel, David.
>
>     Thank you for pointing syntaxes to be addressed. Most of the are
>     addressed in the attached patch.
>
>
>     At Tue, 22 Mar 2016 12:57:27 -0400, David Steele
>     <david@pgmasters.net <mailto:david@pgmasters.net>> wrote in
>     <56F17977.8040503@pgmasters.net <mailto:56F17977.8040503@pgmasters.net>>
>     > Hi Kyotaro,
>     >
>     > On 3/18/16 3:22 AM, Pavel Stehule wrote:
>     >
>     > >     I am looking this patch. It looks well, but this feature doesn't
>     > >     respect upper or lower chars. It enforce upper chars. This is not
>     > >     consistent with any other autocomplete.
>
>     As mentioned before, upper-lower problem is an existing
>     issue. The case of the words in a query result list cannot be
>     edited since it may contain words that should not be changed,
>     such as relation names. So we can address it only before issueing
>     a query but I haven't found simple way to do it.
>
>
> This is unpleasant. I am sorry. I had very uncomfortable feeling from
> this behave. I am thinking so it should be solvable - you have to
> convert only keyword IF EXISTS or IF NOT EXISTS. Maybe there are not
> trivial solution, but this should be fixed.
>

Hello,

Can we do something like in the patch? This patch should be applied
after the patch
"0001-Suggest-IF-NOT-EXISTS-for-tab-completion-of-psql_v3.patch".

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Hi, Pavel.

At Tue, 29 Mar 2016 09:59:01 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRDEPgGyMz2aXgTL33PuD7X+xieaO++wa+V9nQPQiYDMGQ@mail.gmail.com>
> > > On 3/18/16 3:22 AM, Pavel Stehule wrote:
> > >
> > > >     I am looking this patch. It looks well, but this feature doesn't
> > > >     respect upper or lower chars. It enforce upper chars. This is not
> > > >     consistent with any other autocomplete.
> >
> > As mentioned before, upper-lower problem is an existing
> > issue. The case of the words in a query result list cannot be
> > edited since it may contain words that should not be changed,
> > such as relation names. So we can address it only before issueing
> > a query but I haven't found simple way to do it.
> >
> 
> This is unpleasant. I am sorry. I had very uncomfortable feeling from this
> behave. I am thinking so it should be solvable - you have to convert only
> keyword IF EXISTS or IF NOT EXISTS. Maybe there are not trivial solution,
> but this should be fixed.

I understand that and feel the same. But I don't want to put
puzzling code. Defining a macro enable this by writing as the
following.

>    else if (Matches2("ALTER", "TABLE"))
>        COMPLETE_WITH_SCHEMA_QUERY(
>            Query_for_list_of_tables,
>            ADDLIST("",
>                    "('IF EXISTS'), ('ALL IN TABLESPACE')",
>                    "('if exists'), ('all in tablespace')"));
...
>     else if (Matches2("ALTER", "POLICY"))
>         COMPLETE_WITH_QUERY(
>             ADDLIST(Query_for_list_of_policies,
>                     "('IF EXISTS')", "('if exists')"));

This will work as you expects but it looks quite redundant, but
avoiding dynamic string (and/or malloc) operation would lead to
the similar results. Integrating the ADDLIST into
COMPLETE... might make the situation better.

The attached patch does it only for "ALTER TABLE" and "ALTER
POLICY".

> > > > I checked it against sql help and these statements doesn't work
> >
> > Thank you very much.
> >
> > > > alter foreign table hhhh drop column
> > > > drop cast
> > > > drop operator
> > > > drop transform -- missing autocomplete completely
> >
> > These are not done. Each of them has issues to be addressed
> > before adding completion of IF EXISTS.
> >
> > > > alter text search configuration jjj drop mapping
> > > > alter type hhh drop attribute
> > > > drop extension
> >
> > Done.
> >
> > > > drop text search
> >
> > I don't see the syntax "drop text search [if exists]".  drop text
> > search (configuration|dictionary|parser|template) are already
> > addressed.
> >
> 
> ok, probably my mistake. I am sorry.
> 
> 
> >
> > > > drop user mapping
> >
> > "drop user" was not completed with "mapping". I added it then
> > addressed this. (This might be another issue.)
> >
> > > > alter table jjj add column
> >
> > Done if it is mentioning DROP COLUMN. But new two macros
> > HeadMatches6 and 7 are introduced together.
> >
> > > > create temp sequence
> > > > create sequence
> >
> > DROP SEQUENCE is already completed with IF EXISTS. CREATE [TEMP]
> > SEQUENCE with IF NOT EXISTS is added.
> >
> > > Do you have an idea of when you will have a new patch ready?
> >
> > Sorry to to have been late. The attached is the revised version.
> >
> 
> I'll check it today.

Thanks.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi, thank you for the suggestion.

At Tue, 29 Mar 2016 12:54:01 +0300, Artur Zakirov <a.zakirov@postgrespro.ru> wrote in
<56FA50B9.7000107@postgrespro.ru>
> On 29.03.2016 10:59, Pavel Stehule wrote:
> >     > >     I am looking this patch. It looks well, but this feature doesn't
> >     > >     respect upper or lower chars. It enforce upper chars. This is
> >     > >     not
> >     > >     consistent with any other autocomplete.
> >
> >     As mentioned before, upper-lower problem is an existing
> >     issue. The case of the words in a query result list cannot be
> >     edited since it may contain words that should not be changed,
> >     such as relation names. So we can address it only before issueing
> >     a query but I haven't found simple way to do it.
> >
> >
> > This is unpleasant. I am sorry. I had very uncomfortable feeling from
> > this behave. I am thinking so it should be solvable - you have to
> > convert only keyword IF EXISTS or IF NOT EXISTS. Maybe there are not
> > trivial solution, but this should be fixed.
> >
> 
> Hello,
> 
> Can we do something like in the patch? This patch should be applied
> after the patch
> "0001-Suggest-IF-NOT-EXISTS-for-tab-completion-of-psql_v3.patch".

Unfortunately, all completions are shown in upper-case with it
for some cases (since COMP_KEYWORD_CASE is upper as the default
on my environment). This prevents names, that are not SQL
keywords, from getting further completion.

> =# alter table <tab>
> ALL IN TABLESPACE    PG_CATALOG.          PUBLIC.
> ALPHA.               PG_TEMP_1.           X
> IF EXISTS            PG_TOAST.            
> INFORMATION_SCHEMA.  PG_TOAST_TEMP_1.     

pg_strdup_keyword_case follows COMP_KEYWORD_CASE pset variable
which instructs how *SQL keywords* should be handled. It is not
for this usage. And as mentioned before, SQL keywords and
identifiers cannot be treated in the same way, so the place is
not suitable to do this.

By the way, memory blocks that readline sees are freed by it but
blocks allocated by the additional pstrdup seems abandoned
without being freed. Actually, the address of newly allocated
blocks seems increasing time by time slowly *even without this
patch*..

Of course, no one seems to be able to rush into out of memory by
this leak, though..

Anyway, using malloc is one reason of additional complexity and
it is the main cause that I avoided dynamic memory allocation.

Thoughts?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Tue, 29 Mar 2016 13:12:06 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRCnrpdSqSozg4Y8__2LFyiNqUCE=KPzFw1+AF_LutmRiQ@mail.gmail.com>
> 2016-03-29 12:08 GMT+02:00 Kyotaro HORIGUCHI <
> > > > As mentioned before, upper-lower problem is an existing
> > > > issue. The case of the words in a query result list cannot be
> > > > edited since it may contain words that should not be changed,
> > > > such as relation names. So we can address it only before issueing
> > > > a query but I haven't found simple way to do it.
> > > >
> > >
> > > This is unpleasant. I am sorry. I had very uncomfortable feeling from
> > this
> > > behave. I am thinking so it should be solvable - you have to convert only
> > > keyword IF EXISTS or IF NOT EXISTS. Maybe there are not trivial solution,
> > > but this should be fixed.
> >
> > I understand that and feel the same. But I don't want to put
> > puzzling code. Defining a macro enable this by writing as the
> > following.
> >
> 
> puzzle is wrong, but nonconsistent behave is not acceptable

Mmm. Ok, The attched patch, which applies on top of the
IF(NOT)EXIST patch, does this in rather saner appearance, but
needs to run additional C function at runtime and additional some
macros. The function is called up to once per completion, so it
won't be a performance problem.

>    else if (Matches2("ALTER", "TABLE"))
>        COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables,
>            ADDLIST2("", "IF EXISTS", "ALL IN TABLESPACE"));

or

>    else if (Matches5("ALTER", "TABLE", MatchAny, "DROP", "CONSTRAINT"))
>    {
>        completion_info_charp = prev3_wd;
>        COMPLETE_WITH_QUERY(
>            ADDLIST1(Query_for_constraint_of_table, "IF EXISTS"));
>    }

I think this syntax is acceptable. Only keywords follows the
setting of COMP_KEYWORD_CASE, as Artur suggested.

=# alter table <tab>
ALL IN TABLESPACE    pg_catalog.          public.
alpha.               pg_temp_1.           x
IF EXISTS            pg_toast.            
information_schema.  pg_toast_temp_1.     
=# alter table i<tab>
if exists
information_schema.sql_features
...
=# alter table if<tab>
=# alter table if exists 
======
=# alter table I<tab>
=# alter table IF EXISTS    // "information_schema" doesn't match.

Since this is another problem from IF (NOT) EXISTS, this is
in separate form.

What do you think about this?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

No one should care of this but to make shure..

At Tue, 29 Mar 2016 20:12:03 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160329.201203.78219296.horiguchi.kyotaro@lab.ntt.co.jp>
> By the way, memory blocks that readline sees are freed by it but
> blocks allocated by the additional pstrdup seems abandoned
> without being freed. Actually, the address of newly allocated
> blocks seems increasing time by time slowly *even without this
> patch*..

psql doesn't leak memory. The increment was the result of new
history entry. src/common/exec.c does the following thing,

./common/exec.c:586:        putenv(strdup(env_path));
./common/exec.c:597:        putenv(strdup(env_path));

valgrind complains that the memory block allocated there is
definitely lost but it seems to be called once in lifetime of a
process.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

At Wed, 30 Mar 2016 09:23:49 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRBVKa6NG4JwZ2QmrO7inudFJws5w0+demVgZZNuF-HUkQ@mail.gmail.com>
> Hi
> 
> ...
> >> =# alter table if<tab>
> >> =# alter table if exists
> >> ======
> >> =# alter table I<tab>
> >> =# alter table IF EXISTS    // "information_schema" doesn't match.
> >>
> >> Since this is another problem from IF (NOT) EXISTS, this is
> >> in separate form.
> >>
> >> What do you think about this?
> >>
> >
> > +1
> >
> 
> The new behave is much better.

I'm glad to hear that.

> I found new warning
> 
>  tab-complete.c:1438:87: warning: right-hand operand of comma expression
> has no effect [-Wunused-value]

Mmm. Google said me that gcc 4.9 does so. I'm using 4.8.5 so I
haven't see the warning.

https://gcc.gnu.org/gcc-4.9/porting_to.html

1436:    else if (HeadMatches2("CREATE", "SCHEMA") &&
1437:             SHIFT_TO_LAST1("CREATE") &&
1438:             false) {} /* FALL THROUGH */

> #define SHIFT_TO_LAST1(p1) \
>     (HEADSHIFT(find_last_index_of(p1, previous_words, previous_words_count)), \
>      true)

> #define HEADSHIFT(n) \
>     (head_shift += n, true)

expanding the macros the lines at the error will be

else if (HeadMatches2("CREATE", "SCHEMA") &&        (head_shift +=   find_last_index_of("CREATE", previous_words,
previous_words_count),  true) &&        false) {} /* FALL THROUGH */
 

But the right hand value (true) is actually "used" in the
expression (even though not effective). Perhaps (true && false)
was potimized as false and the true is regarded to be unused?
That's stupid.. Using functions instead of macros seems to solve
this but they needed to be wraped by macros as
additional_kw_query(). That's a pain..

Any thougts?

> There is small minor issue - I don't know if it is solvable. Autocomplete
> is working only for "if" keyword. When I am writing "if " or "if " or "if
> exi" - then autocomplete doesn't work. But this issue is exactly same for
> other "multi words" completation like  "alter foreign data wrapper". So if
> it is fixable, then it can be out of scope this patch.

Yes.  It can be saved only by adding completion for every word,
as some of the similar completion is doing.

> anything else looks well.

Thanks.

regards,


-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 31 Mar 2016 11:22:20 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRD7vADuNOiApB8Exwc+C5cCis-rj2dPhvZCwZKgXjb_Xg@mail.gmail.com>
> 2016-03-30 10:34 GMT+02:00 Kyotaro HORIGUCHI <
> horiguchi.kyotaro@lab.ntt.co.jp>:
> 
> > Hi,
> >
> > At Wed, 30 Mar 2016 09:23:49 +0200, Pavel Stehule <pavel.stehule@gmail.com>
> > wrote in <
> > CAFj8pRBVKa6NG4JwZ2QmrO7inudFJws5w0+demVgZZNuF-HUkQ@mail.gmail.com>
> > > Hi
> > >
> > > ...
> > > >> =# alter table if<tab>
> > > >> =# alter table if exists
> > > >> ======
> > > >> =# alter table I<tab>
> > > >> =# alter table IF EXISTS    // "information_schema" doesn't match.
> > > >>
> > > >> Since this is another problem from IF (NOT) EXISTS, this is
> > > >> in separate form.
> > > >>
> > > >> What do you think about this?
> > > >>
> > > >
> > > > +1
> > > >
> > >
> > > The new behave is much better.
> >
> > I'm glad to hear that.
> >
> > > I found new warning
> > >
> > >  tab-complete.c:1438:87: warning: right-hand operand of comma expression
> > > has no effect [-Wunused-value]
> >
> > Mmm. Google said me that gcc 4.9 does so. I'm using 4.8.5 so I
> > haven't see the warning.
> >
> > https://gcc.gnu.org/gcc-4.9/porting_to.html
> >
> > 1436:   else if (HeadMatches2("CREATE", "SCHEMA") &&
> > 1437:                    SHIFT_TO_LAST1("CREATE") &&
> > 1438:                    false) {} /* FALL THROUGH */
> >
> > > #define SHIFT_TO_LAST1(p1) \
> > >     (HEADSHIFT(find_last_index_of(p1, previous_words,
> > previous_words_count)), \
> > >      true)
> >
> > > #define HEADSHIFT(n) \
> > >     (head_shift += n, true)
> >
> > expanding the macros the lines at the error will be
> >
> > else if (HeadMatches2("CREATE", "SCHEMA") &&
> >          (head_shift +=
> >           find_last_index_of("CREATE", previous_words,
> > previous_words_count),
> >            true) &&
> >          false) {} /* FALL THROUGH */
> >
> > But the right hand value (true) is actually "used" in the
> > expression (even though not effective). Perhaps (true && false)
> > was potimized as false and the true is regarded to be unused?
> > That's stupid.. Using functions instead of macros seems to solve
> > this but they needed to be wraped by macros as
> > additional_kw_query(). That's a pain..
> >
> > Any thougts?
> >
> > > There is small minor issue - I don't know if it is solvable. Autocomplete
> > > is working only for "if" keyword. When I am writing "if " or "if " or "if
> > > exi" - then autocomplete doesn't work. But this issue is exactly same for
> > > other "multi words" completation like  "alter foreign data wrapper". So
> > if
> > > it is fixable, then it can be out of scope this patch.
> >
> > Yes.  It can be saved only by adding completion for every word,
> > as some of the similar completion is doing.
> >
> > > anything else looks well.
> >
> > Thanks.
> >
> 
> please, final patch

This needs to use gcc 4.9 to address, but CentOS7 doesn't have
devtools-2 repo so now I'm building CentOS6 environment for this
purpose. Please wait for a while.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, sorry for being a bit late.
The attatched are the new version of the patch.. set.

1. 0001-Suggest-IF-NOT-EXISTS-for-tab-completion-of-psql.patch
Adds IF (NOT) EXISTS completion. It doesn't fix the issue thatthe case of additional keywords don't follow the input.

2. 0002-Make-added-keywords-for-completion-queries-follow-to.patch
Fixes the case-don't-follow issue by introducing a new macro setADDLISTn(). This leaves the issue for keywords along
withattributes.

3. 0003-Make-COMPLETE_WITH_ATTR-to-accept-additional-keyword.patch
 Fixes the issue left after 0002 patch.  This patch does the following things.  1. Change completion_charp from const
char* to PQExpBuffer.  2. Chnage COMPLETE_WITH_QUERY and COMPLETE_WITH_ATTR to accept    an expression instead of
stringliteral.  3. Replace all additional keyword lists in psql_copmletion with    ADDLISTn() expression.
 


At Fri, 01 Apr 2016 11:52:03 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160401.115203.98896697.horiguchi.kyotaro@lab.ntt.co.jp>
> > > > I found new warning
> > > >
> > > >  tab-complete.c:1438:87: warning: right-hand operand of comma expression
> > > > has no effect [-Wunused-value]
> > >
> > > Mmm. Google said me that gcc 4.9 does so. I'm using 4.8.5 so I
> > > haven't see the warning.
> > >
> > > https://gcc.gnu.org/gcc-4.9/porting_to.html
> > >
> > > 1436:   else if (HeadMatches2("CREATE", "SCHEMA") &&
> > > 1437:                    SHIFT_TO_LAST1("CREATE") &&
> > > 1438:                    false) {} /* FALL THROUGH */
...
> > > But the right hand value (true) is actually "used" in the
> > > expression (even though not effective). Perhaps (true && false)
> > > was potimized as false and the true is regarded to be unused?
> > > That's stupid.. Using functions instead of macros seems to solve
> > > this but they needed to be wraped by macros as
> > > additional_kw_query(). That's a pain..
...
> This needs to use gcc 4.9 to address, but CentOS7 doesn't have
> devtools-2 repo so now I'm building CentOS6 environment for this
> purpose. Please wait for a while.

Finally I settled it by replacing comma expression with logical
OR or AND expresssion. gcc 4.9 compains for some unused variables
in flex output but it is the another issue.

I forgot to address COMPLETE_WITH_ATTTR but it needed an overhaul
of some macros and changing the type of completion_charp. The
third patch does it but it might be unacceptable..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Thank you for testing. That is a silly mistake, sorry.

The attached is the fixed version.

# Can I add a suffix to format-patche's output files?

At Sat, 2 Apr 2016 07:18:32 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRADF3rmQ3y33aeR1C7wOi2QsS65C8bBtiRNqU0zWVWayg@mail.gmail.com>
> >> Finally I settled it by replacing comma expression with logical
> >> OR or AND expresssion. gcc 4.9 compains for some unused variables
> >> in flex output but it is the another issue.
> >>
> >> I forgot to address COMPLETE_WITH_ATTTR but it needed an overhaul
> >> of some macros and changing the type of completion_charp. The
> >> third patch does it but it might be unacceptable..
> >>
> >>
> > something is wrong, autocomplete for CREATE TABLE IF NOT EXISTS doesn't
> > work
> >
> 
> CREATE UNLOGGED/TEMP table is working.

Mmm. I mitakenly refactored multi-step matching.

>   else if (HeadMatches3("CREATE", MatchAny, "TABLE") &&
>        MidMatchAndRemove1(1, "TEMP|TEMPORARY|UNLOGGED") &&
>        Matches2("CREATE", "TABLE"))
>     COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables,
>                    ADDLIST1("IF NOT EXISTS"));

The completion runs only for CREATE AnyKeyword TABLE when
AnyKeyword is removable. It is wrong to do so when any of
prev_words[] that matches the last Matches() can be fileterd out
by the first Headmatches(). The same kind of mistake was found in
the following syntaxes. CREATE SEQENCE had one more mistake.


"CREATE [UNIQUE] INDEX"
"CREATE [TEMP] SEQUENCE"
"CREATE [TEMP..] TABLE"

It is arguable that it is proper to suggest existing object for
CREATE statement, but most of the statement is suggested. It is
semantically wrong but practically useful to know what kind of
word should be there.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 1. We want this patch - it increase a functionality of autocomplete

TBH, I do not think that is an agreed-to statement.  I concur with
Peter's comments upthread questioning how much use-case there is for
interactive completion of IF (NOT) EXISTS.  If it were a small and
uncomplicated patch, I wouldn't object ... but this is neither.

It increases the size of tab-complete.c by nearly 10%, and would
increase it more than that if it were adequately documented as to
what all these new macros and variables do.  (To take just the first
example, APPEND_COMP_CHARP and SET_COMP_CHARP not only lack any
documentation, but have been inserted between a comment documenting
some macros and those macros.  Another complaint in the same vein is
that MatchesN() no longer means even approximately what it did before,
but the comment for those macros hasn't been changed.)  On top of that,
it seems like there's an awful lot of klugery going on here.  An example
is the use of the COLLAPSE macro (hidden inside MidMatchAndRemoveN),
which seems like a seriously bad idea because it destroys data even if
the match the macro is embedded in ultimately fails.  That will create
order dependencies between match rules, which is not something we want
IMO, most especially when it's not clearly marked in the match rules
what's dependent on what.

Seeing things like "if (something-with-side-effects && false)" doesn't
fill me with any great admiration for the cleanliness of the code, either.

In short, I'm not sold that we need autocomplete for IF EXISTS,
and if the price we have to pay for it is klugery on this scale,
it's no sale.  I think this needs to be sent back for a considerable
amount of rethinking.

One thing that might be worth considering to get rid of this
side-effects-in-IF-conditions mess is to move the match rules into
a separate function so that after doing a successful match, we just
"return".  This could be implemented in most places by adding
return statements into the COMPLETE_WITH_FOO macros.  Then we would
not need the enormous else-if chain, but just simple independent
if statements, where we know a successful match will end with a
"return" instead of falling through to the next statement.  The
big advantage of that is that then you can do operations with
side-effects explicitly as separate statements, instead of having
to make them look like phony else-if cases.  So for example the
CREATE SCHEMA case might be handled like
   if (Matches2("CREATE", "SCHEMA"))   {       ... handle possible autocompletions of CREATE SCHEMA itself here ...
       /* Else, move head match point past CREATE SCHEMA */       if ((n = find_last_index_of("CREATE")) > 0)
HEAD_SHIFT(n);  }
 
   /*    * Statements that can appear in CREATE SCHEMA should be considered here!    */
   if (Matches2("CREATE", "TABLE"))       ... handle CREATE TABLE here ...
   ... handle other statements that can appear in CREATE SCHEMA here ...

After exhausting the possibilities for sub-statements of CREATE SCHEMA,
we could either return failure if we'd seen CREATE SCHEMA:
   /*    * Fail if we saw CREATE SCHEMA; no rules below here should be considered.    */   if (head_shift > 0)
returnfalse;
 

or reset the head match point before continuing with unrelated rules:
   /*    * Done considering CREATE SCHEMA sub-rules, so forget about    * whether we saw CREATE SCHEMA.    */
HEAD_SHIFT(0);

Immediately failing seems like the right thing for CREATE SCHEMA, but
maybe in other cases we just undo the head_shift change and keep trying.
This is still order dependent, but at least the places where we change
the match basis can be made to be fairly obvious actions instead of
being disguised as just-another-candidate-match.

I don't immediately have a better idea to replace COLLAPSE, but I really
don't like that as it stands.  I wonder whether we could dodge the need
for it by just increasing head_shift when deciding to skip over an
IF (NOT) EXISTS clause.  Otherwise, I think what I'd want to see is
some way to explicitly undo the effects of COLLAPSE when done with
rules that could benefit from it.  Or at least a coding convention
that makes it clear that you return failure once you've considered
all subsidiary rules, rather than continuing on with unrelated rules
that would have a risk of false match thanks to the data removal.
        regards, tom lane

Thank you for looking this and for the comment.

Since the end of this CF is quite soon and this seems in
uncommittable state, feel free to move this to the next CF if any
other patch with more priority.


The attached patch is rather WIPs. 

1. The same as the first one in the previous version. Just adding  suggestion and making further matching ignoring
them.

2. The same as the second one in the previous version. Fix so  that the case of almost all keywords follow an input,
except attributes.
 

3. The same as the third one in the previous version. Changes  COMPLETE_WITH_ATTR and COMPLETE_WITH_QUERY so that all
keywordscan follow an input.
 

== The followings are the new patches

4. Applicable on top of 3. Addressing the comments.  Refactors  tab-completion.c so that it no longer needs the
long-long else-if sequences. For the readability.  Getting rid of the  if(.. && false) messes.
 

5. A sample implement of completion code for CREATE/GRANT/REVOKE.



At Wed, 06 Apr 2016 18:40:57 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <4317.1459982457@sss.pgh.pa.us>
> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > 1. We want this patch - it increase a functionality of autocomplete
> 
> TBH, I do not think that is an agreed-to statement.  I concur with
> Peter's comments upthread questioning how much use-case there is for
> interactive completion of IF (NOT) EXISTS.  If it were a small and
> uncomplicated patch, I wouldn't object ... but this is neither.

The objective of this patch is not only suggesting "IF (NOT)
EXISTS", but also, enabling to continue completion even such
"noise" words are in command line, without providing multiple
match descriptions for both of with and without the
noise. Currently such words just prohibits continueing
completion.

So if many of us consider that completing such words is just a
crap, it is possible to remove only suggestion, but preserve
ignoring of such words.

> It increases the size of tab-complete.c by nearly 10%, and would

The increase is natural since it treats additional syntax but the
amount of 10% is arguable. However there's some more "noise"
words to be addressed.

CREATE ROLE name [[WITH] option [...]]
ALTER TABLE name ... col [SET DATA] TYPE
CREATE [LOCAL|GLOBAL] .. TABLE
CREATE [OR REPLACE] [TRUSTED] [PROCEDUAL] LANGUAGE
CREATE [OR REPLACE] FUNCTION
...

> increase it more than that if it were adequately documented as to
> what all these new macros and variables do.  (To take just the first
> example, APPEND_COMP_CHARP and SET_COMP_CHARP not only lack any
> documentation, but have been inserted between a comment documenting
> some macros and those macros. Another complaint in the same vein is
> that MatchesN() no longer means even approximately what it did before,
> but the comment for those macros hasn't been changed.) 

Added simple comments for them and others.

>  On top of that,
> it seems like there's an awful lot of klugery going on here.  An example
> is the use of the COLLAPSE macro (hidden inside MidMatchAndRemoveN),
> which seems like a seriously bad idea because it destroys data even if
> the match the macro is embedded in ultimately fails. That will create
> order dependencies between match rules, which is not something we want
> IMO, most especially when it's not clearly marked in the match rules
> what's dependent on what.

Currently some of the matching conditions (even remote each
other) have order dependencies. Altering them caused some
unexpected change of completion behavior. They are already not
order independent at all, I think. (I'm sorry not to give an
instance for now.. It might be resolved by recent changes.)

But I must admit that the usage of MidMatchAndRemoveN is actually
a bit complicated, but it is finally removed in this patch.

> Seeing things like "if (something-with-side-effects && false)" doesn't
> fill me with any great admiration for the cleanliness of the code, either.

This is also removed in the attached patch set.

> In short, I'm not sold that we need autocomplete for IF EXISTS,
> and if the price we have to pay for it is klugery on this scale,
> it's no sale.  I think this needs to be sent back for a considerable
> amount of rethinking.
> 
> One thing that might be worth considering to get rid of this
> side-effects-in-IF-conditions mess is to move the match rules into
> a separate function so that after doing a successful match, we just
> "return".  This could be implemented in most places by adding
> return statements into the COMPLETE_WITH_FOO macros.  Then we would
> not need the enormous else-if chain, but just simple independent
> if statements, where we know a successful match will end with a
> "return" instead of falling through to the next statement.  The
> big advantage of that is that then you can do operations with
> side-effects explicitly as separate statements, instead of having
> to make them look like phony else-if cases.  So for example the
> CREATE SCHEMA case might be handled like
> 
>     if (Matches2("CREATE", "SCHEMA"))
>     {
>         ... handle possible autocompletions of CREATE SCHEMA itself here ...
> 
>         /* Else, move head match point past CREATE SCHEMA */
>         if ((n = find_last_index_of("CREATE")) > 0)
>             HEAD_SHIFT(n);
>     }
> 
>     /*
>      * Statements that can appear in CREATE SCHEMA should be considered here!
>      */
> 
>     if (Matches2("CREATE", "TABLE"))
>         ... handle CREATE TABLE here ...
> 
>     ... handle other statements that can appear in CREATE SCHEMA here ...
> 
> After exhausting the possibilities for sub-statements of CREATE SCHEMA,
> we could either return failure if we'd seen CREATE SCHEMA:
> 
>     /*
>      * Fail if we saw CREATE SCHEMA; no rules below here should be considered.
>      */
>     if (head_shift > 0)
>         return false;

The previous patch designed so as not to make drastic changes to
psql_completion, but since it makes the function ugly I
refactored the function in this patch set.

And as an example, the last (5th) patch does this and introduces
CREATE SCHEMA completion in more matured way (of behavior, not
coding), and make the completion for GRANT/REVOKE more simplly.

> or reset the head match point before continuing with unrelated rules:
> 
>     /*
>      * Done considering CREATE SCHEMA sub-rules, so forget about
>      * whether we saw CREATE SCHEMA.
>      */
>     HEAD_SHIFT(0);
> 
> Immediately failing seems like the right thing for CREATE SCHEMA, but
> maybe in other cases we just undo the head_shift change and keep trying.

This patch is doing the former since the completion for "CREATE
SCHEMA" cannot does other than returning so far.

> This is still order dependent, but at least the places where we change
> the match basis can be made to be fairly obvious actions instead of
> being disguised as just-another-candidate-match.

> I don't immediately have a better idea to replace COLLAPSE, but I really
> don't like that as it stands.  I wonder whether we could dodge the need
> for it by just increasing head_shift when deciding to skip over an
> IF (NOT) EXISTS clause.

How COLLAPSE can be avoided is shown by the CREATE/GRANT/REOKE
code after applying the last patch. If this way is not bad, I'll
do this on the whole psql_completion.

>  Otherwise, I think what I'd want to see is
> some way to explicitly undo the effects of COLLAPSE when done with
> rules that could benefit from it.  Or at least a coding convention
> that makes it clear that you return failure once you've considered
> all subsidiary rules, rather than continuing on with unrelated rules
> that would have a risk of false match thanks to the data removal.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Apr 7, 2016 at 8:19 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Thank you for looking this and for the comment.
>
> Since the end of this CF is quite soon and this seems in
> uncommittable state, feel free to move this to the next CF if any
> other patch with more priority.

Moved.  It's appropriate to consider this "needs review" at this
point, I think so added to 2016-09 that way.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello,

At Sun, 4 Sep 2016 12:54:57 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRDWjv8Ahd=71vHmCT=DU6eor+Hwudh30b2f8cPyYH0eug@mail.gmail.com>
> This patch needs rebase.

Thank you. I'll rebase the following patch and repost as soon as
possible.

https://www.postgresql.org/message-id/20160407.211917.147996130.horiguchi.kyotaro@lab.ntt.co.jp

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, this is the new version of this patch. Rebased on the
current master.

At Tue, 06 Sep 2016 13:06:51 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160906.130651.171572544.horiguchi.kyotaro@lab.ntt.co.jp>
> Thank you. I'll rebase the following patch and repost as soon as
> possible.
> 
> https://www.postgresql.org/message-id/20160407.211917.147996130.horiguchi.kyotaro@lab.ntt.co.jp

This patch consists of the following files. Since these files are
splitted in strange criteria and order for historical reasons,
I'll reorganize this and post them later.

- 0001-Suggest-IF-NOT-EXISTS-for-tab-completion-of-psql.patch
 Add suggestion of IF (NOT) EXISTS on master. This should be the last patch in this patchset.

- 0002-Make-added-keywords-for-completion-queries-follow-to.patch
 Current suggestion mechanism doesn't distinguish object names and keywords, which should be differently handled in
determiningletter cases. This patch fixes that.
 

- 0003-Make-COMPLETE_WITH_ATTR-to-accept-additional-keyword.patch
 This patch apply the 0002 fix to COMPLET_WITH_ATTR.

- 0004-Refactoring-tab-complete-to-make-psql_completion-cod.patch
 By Tom's suggestion, in order to modify previous_words in more sane way, transforming the else-if sequence in
psql_completioninto a simple if sequence.
 

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Sat, 10 Sep 2016 07:40:16 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRC02=1AvrnsE+T++S_EB2zkwj3wp-6KhYi5pMR25=nwew@mail.gmail.com>
> 2016-09-06 15:00 GMT+02:00 Kyotaro HORIGUCHI <
> horiguchi.kyotaro@lab.ntt.co.jp>:
> 
> > Hello, this is the new version of this patch. Rebased on the
> > current master.
..
> > This patch consists of the following files. Since these files are
> > splitted in strange criteria and order for historical reasons,
> > I'll reorganize this and post them later.
> >
> 
> ok, can I start with testing and review with some from these files?

No problem, of couese. The reorganizing won't be a labor but will
change the spape so as to affect reviewing. Please wait for a
while.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, this is the new version.

At Tue, 13 Sep 2016 10:50:13 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160913.105013.65452566.horiguchi.kyotaro@lab.ntt.co.jp>
> > > This patch consists of the following files. Since these files are
> > > splitted in strange criteria and order for historical reasons,
> > > I'll reorganize this and post them later.

The focus of this patch has changed. The first motivation was
completing IF-EXISTS but the underlying issue was flexibility of
psql_completion. And as Pavel's suggestion, keywords suggested
along with database objects should follow the character case of
input.

For the purpose of resolving the issues, I reorganized the
confused patch set. The attached patches are organized as the
following.

1. Refactoring tab-complete to make psql_completion code
 Does two things. One is moving out the macros that has grown to be too large to stay in tab_completion.c to new file
tab-complete-macros.hThe other is separating out the else-if sequence in psql_completion() as a new function
psql_completion_internal().This allows us to the following things.
 
 - Exit from arbitrary place in the former-else-if sequence just   by return.
 - Do other than "if(matching) { completion }" in anywhere   convenient in the midst of the former-els... 
 - Recursively matching for sub syntaxes. EXPLAIN, RULE and   others are using this feature. (Needs the 4th patch to do
 this, though)
 

2. Make keywords' case follow to input
 Allow the keywords suggested along with databse objects to follow the input letter case. The core part of this patch
isa new function additional_kw_query(), which dynamically generates additional query string with specified keywords in
thedesired letter case. COMPLETE_WITH_* macros are modified to accept the function.
 

3. Fix suggested keywords to follow input in tab-completion session 2
 The 2nd patch above leaves some query string containing static keyword strings, which results in failure to follow
inputletter cases. Most of them are naturally removed but role names are a bother. This patch puts additional query
stringsfor several usage of roles but it might be overdone.
 

4. Introduce word shift and removal feature to psql-completion
 This is the second core for the flexibility of completion code. The word shift feature is the ability to omit first
severalwords in *MatchesN macros. For example this allows complete create-schema's schema elements in a natural code.
(Currentlythose syntaxes that can be a schema elements are using TailMatches instead of Matches, as the result
HeadMatchesare not available there). The words removing feature is the ability to (desructively) clip multiple
suceessivewords in the previous_words list. This feature allows suceeding completion code not to care about the removed
words,such like UNIQUE, CONCURRENTLY, VERBOSE and so on.
 

5. Add suggestion for IF (NOT) EXISTS for some syntaxes
 This adds IF (NOT) EXISTS suggestion, as a PoC.  This patch no loger covers all adoptable syntaces since the places
wheremore than boilerplating is required are omitted.
 

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Sep 19, 2016 at 6:11 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I am thinking so commit's description should be inside README

Horiguchi-san, your patch has some whitespace issues, you may want to
get a run with git diff --check. Here are some things I have spotted:
src/bin/psql/tab-complete.c:1074: trailing whitespace.
+        "MATERIALIZED VIEW",
src/bin/psql/tab-complete.c:2621: trailing whitespace.
+       COMPLETE_WITH_QUERY(Query_for_list_of_roles,

This set of patches is making psql tab completion move into a better
shape, particularly with 0001 that removes the legendary huge if-elif
and just the routine return immediately in case of a keyword match.
Things could be a little bit more shortened by for example not doing
the refactoring of the tab macros because they are just needed in
tab-complete.c. The other patches introduce further improvements for
the existing infrastructure, but that's a lot of things just for
adding IF [NOT] EXISTS to be honest.

Testing a bit, I have noticed that for example trying to after typing
"create table if", if I attempt to do a tab completion "not exists"
does not show up. I suspect that the other commands are failing at
that as well.
-- 
Michael

On Tue, Sep 20, 2016 at 3:50 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Sep 19, 2016 at 6:11 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> I am thinking so commit's description should be inside README
>
> Horiguchi-san, your patch has some whitespace issues, you may want to
> get a run with git diff --check. Here are some things I have spotted:
> src/bin/psql/tab-complete.c:1074: trailing whitespace.
> +        "MATERIALIZED VIEW",
> src/bin/psql/tab-complete.c:2621: trailing whitespace.
> +       COMPLETE_WITH_QUERY(Query_for_list_of_roles,
>
> This set of patches is making psql tab completion move into a better
> shape, particularly with 0001 that removes the legendary huge if-elif
> and just the routine return immediately in case of a keyword match.
> Things could be a little bit more shortened by for example not doing
> the refactoring of the tab macros because they are just needed in
> tab-complete.c. The other patches introduce further improvements for
> the existing infrastructure, but that's a lot of things just for
> adding IF [NOT] EXISTS to be honest.
>
> Testing a bit, I have noticed that for example trying to after typing
> "create table if", if I attempt to do a tab completion "not exists"
> does not show up. I suspect that the other commands are failing at
> that as well.

This patch hasn't been updated in over a week and we're just about out
of time for this CommitFest, so I've marked it "Returned with
Feedback" for now.  If it gets updated, it can be resubmitted for the
next CommitFest.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello,

At Wed, 28 Sep 2016 12:49:25 -0400, Robert Haas <robertmhaas@gmail.com> wrote in
<CA+TgmoargP0PpbUKZFGsbx0yR=6OH8iBp8SPFHMaDaGy1CWKOQ@mail.gmail.com>
> This patch hasn't been updated in over a week and we're just about out
> of time for this CommitFest, so I've marked it "Returned with
> Feedback" for now.  If it gets updated, it can be resubmitted for the
> next CommitFest.

Thanks, I will do it.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Thank you for reviewing!

At Mon, 19 Sep 2016 11:11:03 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRCpoYMoUzZ74p0JvX=orUxs7o88UR0z0-Lqt6W6bS9DaQ@mail.gmail.com>
> >> 2. Make keywords' case follow to input
> >>
> >>   Allow the keywords suggested along with databse objects to
> >>   follow the input letter case. The core part of this patch is a
> >>   new function additional_kw_query(), which dynamically generates
> >>   additional query string with specified keywords in the desired
> >>   letter case. COMPLETE_WITH_* macros are modified to accept the
> >>   function.
> >>
> >>
> second patch is working, but I don't think it is enough documented

Mmm. I should admit that. I will add comments in it.

> what is addon in COMPLETE_WITH_QUERY(query, addon)? semantics, usage?

Original COMPLETE_WITH_QUERY gets only query, but it is used in
the form when additional keywords are needed.

| COMPLETE_WITH_QUERY(Query_for_list_of_roles " UNION SELECT 'DEFAULT'");

This is a string literal concatenation which is available only on
compile time. Letter case modification needs this done in
runtime. So it should be given as a separate parameter from the
main query.

> in 99% the addon is "" when macro
> COMPLETE_WITH_SCHEMA_QUERY,COMPLETE_WITH_QUERY is used. Maybe a
> introduction of new macros with nonempty addon parameter should be better.

That looks better. I'll change the API as the following.

COMPLETE_WITH_QUERY(query);
COMPLETE_WITH_QUERY_KW(query, kwlist);
COMPLETE_WITH_SCHEMA_QUERY(squery);
COMPLETE_WITH_SCHEMA_QUERY_KW(squery, kwlist);


> > 3. Fix suggested keywords to follow input in tab-completion session 2
> >>
> >>   The 2nd patch above leaves some query string containing static
> >>   keyword strings, which results in failure to follow input
> >>   letter cases. Most of them are naturally removed but role names
> >>   are a bother. This patch puts additional query strings for
> >>   several usage of roles but it might be overdone.
> >>
> >
> this patch looks well
> 
> 
> >
> >> 4. Introduce word shift and removal feature to psql-completion
> >>
> >>   This is the second core for the flexibility of completion code.
> >>   The word shift feature is the ability to omit first several
> >>   words in *MatchesN macros. For example this allows complete
> >>   create-schema's schema elements in a natural code. (Currently
> >>   those syntaxes that can be a schema elements are using
> >>   TailMatches instead of Matches, as the result HeadMatches are
> >>   not available there). The words removing feature is the ability
> >>   to (desructively) clip multiple suceessive words in the
> >>   previous_words list. This feature allows suceeding completion
> >>   code not to care about the removed words, such like UNIQUE,
> >>   CONCURRENTLY, VERBOSE and so on.
> >>
> >
> I am thinking so commit's description should be inside README

Currently psql or tab-complete.c/psql_completion() have no such
document. If this should be written as README, perhaps I should
write about completion in general. On the other hand, per-macro
explanations are written in tab-complete-macros.h but the usages
are not. I'll try to write README.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Tue, 20 Sep 2016 16:50:29 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqRY1B++XJ26Mb+AUJxZQhS_1qWMi+MOWqJTDUBKXuuGTw@mail.gmail.com>
> On Mon, Sep 19, 2016 at 6:11 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > I am thinking so commit's description should be inside README
> 
> Horiguchi-san, your patch has some whitespace issues, you may want to
> get a run with git diff --check. Here are some things I have spotted:
> src/bin/psql/tab-complete.c:1074: trailing whitespace.
> +        "MATERIALIZED VIEW",
> src/bin/psql/tab-complete.c:2621: trailing whitespace.
> +       COMPLETE_WITH_QUERY(Query_for_list_of_roles,

Thank you very much for pointing it out. I put a pre-commit hook
to check that not to do such a mistake again.


http://stackoverflow.com/questions/591923/make-git-automatically-remove-trailing-whitespace-before-committing/22704385#22704385

> This set of patches is making psql tab completion move into a better
> shape, particularly with 0001 that removes the legendary huge if-elif
> and just the routine return immediately in case of a keyword match.
> Things could be a little bit more shortened by for example not doing
> the refactoring of the tab macros because they are just needed in
> tab-complete.c. The other patches introduce further improvements for
> the existing infrastructure, but that's a lot of things just for
> adding IF [NOT] EXISTS to be honest.

It was the motive for this, but even excluding it, some syntaxes
with optional keywords can be simplified or enriched with the new
macros. CREATE SCHEMA's schema elements, CREATE INDEX and some
other syntaxes are simplified using the feature.

> Testing a bit, I have noticed that for example trying to after typing
> "create table if", if I attempt to do a tab completion "not exists"
> does not show up. I suspect that the other commands are failing at
> that as well.

I suppose it is "create table if ", with a space at the tail. It
is a general issue on combined keywords(?) suggestion in the
whole tab-completion mechanism (or readline's limitation). Some
sytaxes have explicit complition for such cases. For examle,
"create foreign " gets a suggestion of "DATA WRAPPER" since it
has an explcit suggestion step.

> /* ALTER FOREIGN */
> if (Matches2("ALTER", "FOREIGN"))
>     COMPLETE_WITH_LIST2("DATA WRAPPER", "TABLE");

It is apparently solvable, but needs additional code to suggest
the rest words for every steps. It should be another issue.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 29 Sep 2016 16:16:00 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160929.161600.224338668.horiguchi.kyotaro@lab.ntt.co.jp>
> That looks better. I'll change the API as the following.
> 
> COMPLETE_WITH_QUERY(query);
> COMPLETE_WITH_QUERY_KW(query, kwlist);
> COMPLETE_WITH_SCHEMA_QUERY(squery);
> COMPLETE_WITH_SCHEMA_QUERY_KW(squery, kwlist);

Done on my environment.

> > >> 4. Introduce word shift and removal feature to psql-completion
> > >>
> > >>   This is the second core for the flexibility of completion code.
> > >>   The word shift feature is the ability to omit first several
> > >>   words in *MatchesN macros. For example this allows complete
> > >>   create-schema's schema elements in a natural code. (Currently
> > >>   those syntaxes that can be a schema elements are using
> > >>   TailMatches instead of Matches, as the result HeadMatches are
> > >>   not available there). The words removing feature is the ability
> > >>   to (desructively) clip multiple suceessive words in the
> > >>   previous_words list. This feature allows suceeding completion
> > >>   code not to care about the removed words, such like UNIQUE,
> > >>   CONCURRENTLY, VERBOSE and so on.
> > >>
> > >
> > I am thinking so commit's description should be inside README
> 
> Currently psql or tab-complete.c/psql_completion() have no such
> document. If this should be written as README, perhaps I should
> write about completion in general. On the other hand, per-macro
> explanations are written in tab-complete-macros.h but the usages
> are not. I'll try to write README.

Before proposing new patch including this. Since I'm totally
inconfident for my capability to write a documentation, I'd like
to ask anyone here of what shape we are to aim..

The following is the first part of the document I have written up
to now. Please help me by giving me any corrections, suggestions,
opinions, or anything else!

# The completion macro part would be better to be moved as
# comments for the macros in tab-complete-macros.h.

======================
src/bin/psql/README.completion

Word completion of interactive psql
===================================

psql supports word completion on interactive input. The core function
of the feature is psql_completion_internal in tab-complete.c. A bunch
of macros are provided in order to make it easier to read and maintain
the completion code. The console input to refer is stored in char **
previous_words in reverse order but maintaiers of
psql_completion_internal don't need to be aware of the detail of
it. Most of the operation can be described using the provided macros.

Basic structure of the completion code
--------------------------------------

The main part of the function is just a series of completion
definitions, where the first match wins. Each definition basically is
in the following shape.
  if (*matching expression*)     *corresponding completion, then return*

The matching expression checks against all input words before the word
currently being entered. Completion chooses the words prefixed by
letters already entered. For example, for "CREATE <tab>" the word list
to be matched is ["CREATE"] and the prefix for completion is
nothing. For "CREATE INDEX id", the list is ["CREATE", "INDEX"] and
the prefix is "id".


Matching expression macros
--------------------------
There are four types of matching expression macros.

- MatchesN(word1, word2 .. , wordN)
true iff the word list is exactly the same as the paremeter.

- HeadMatchesN(word1, word2 .., wordN)
true iff the first N words in the word list matches the parameter.

- TailMatchesN(word1, word2 .., wordN)
true iff the last N words in the word list matches the parameter.

- MidMatchesN(pos, word1, word2 .., wordN)
true iff N successive words starts from pos in the word list matchesthe parameter. The position is 1-based.


Completion macros
-----------------
There are N types of completion macros.

- COMPLETE_WITH_QUERY(query), COMPLETE_WITH_QUERY_KW(query, addon)
 Suggest completion words acquired using the given query. The details of the query is seen in the comment for
_complete_from_query().Word matching is case-sensitive.
 
 The latter takes an additional parameter, which should be a fragment of query starts with " UNION " followed by a
querystring which gives some additional words. This addon can be ADDLISTN() macro for case-sensitive suggestion.
 

- COMPLETE_WITH_SCHEMA_QUERY(squery), COMPLETE_WITH_SCHEMA_QUERY_KW(squery, addon)
 Suggest based on a "schema query", which is a struct containing parameters. You will see the details in the comment
for_complete_from_query(). Word maching is case-sensitive.
 
 Just same as COMPLETE_WITH_QUERY_KW, the latter form takes a fragment query same to that for COMPLETE_WITH_QUERY_KW.

- COMPLETE_WITH_LIST_CS(list)
 Suggest completion words given as an array of strings. Word matching is case-sensitive.

- COMPLETE_WITH_LIST_CSN(s1, s2.. ,sN)
 Shortcut for COMPLETE_WITH_LIST_CS.

- COMPLETE_WITH_LIST(list)
 Same as COMPLETE_WITH_LIST_CS except that word matching is case-insensitive and the letter case of suggestions is
decidedaccording to COMP_KEYWORD_CASE.
 

- COMPLETE_WITH_LISTN(s1, s2.. ,sN)
 Shortcut for COMPLETE_WITH_LIST.

- COMPLETE_WITH_CONST(string)
 Same as COMPLETE_WITH_LIST but with just one suggestion.

- COMPLETE_WITH_ATTR(relation, addon)
 Suggest completion attribute names for the given relation. Word matching is case-sensitve. 
- COMPLETE_WITH_FUNCTION_ARG(function)
 Suggest function name for the given SQL function. Word matching is case-sensitve.


Additional keywords for COMPLETE_WITH(_SCHEMA)_QUERY
----------------------------------------------------

(snip, or done till here..)

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Fri, 30 Sep 2016 14:43:03 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160930.144303.91443471.horiguchi.kyotaro@lab.ntt.co.jp>
> The following is the first part of the document I have written up
> to now. Please help me by giving me any corrections, suggestions,
> opinions, or anything else!

Anyway, I fixed space issues and addressed the
COMPLETE_WITH_QUERY()'s almost unused parameter problem. And
tried to write a README files.

Some incomplete suggestions are not of this patch and maybe
proposed as another patch.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, I rebased this patch on the current master.

At Mon, 31 Oct 2016 10:15:48 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20161031.101548.162143279.horiguchi.kyotaro@lab.ntt.co.jp>
> Anyway, I fixed space issues and addressed the
> COMPLETE_WITH_QUERY()'s almost unused parameter problem. And
> tried to write a README files.

tab-complete.c have gotten some improvements after this time.

577f0bdd2b8904cbdfde6c98f4bda6fd93a05ffc psql: Tab completion for renaming enum values.
927d7bb6b120a2ca09a164898f887eb850b7a329 Improve tab completion for CREATE TRIGGER.
1d15d0db50a5f39ab69c1fe60f2d5dcc7e2ddb9c psql: Tab-complete LOCK [TABLE] ... IN {ACCESS|ROW|SHARE}.

The attached patchset is rebsaed on the master including these
patches.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

Thank you for looking this long-and-bothersome patch.


At Wed, 23 Nov 2016 07:12:00 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRBxgUrg-6CKbVOy4VqwSFkrf--uCzj3q-vd9FgpSGV+qQ@mail.gmail.com>
> Hi
> 
> 2016-11-15 12:26 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.
> jp>:
> 
> > Hello, I rebased this patch on the current master.
> >
> > At Mon, 31 Oct 2016 10:15:48 +0900 (Tokyo Standard Time), Kyotaro
> > HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <
> > 20161031.101548.162143279.horiguchi.kyotaro@lab.ntt.co.jp>
> > > Anyway, I fixed space issues and addressed the
> > > COMPLETE_WITH_QUERY()'s almost unused parameter problem. And
> > > tried to write a README files.
> >
> > tab-complete.c have gotten some improvements after this time.
> >
> > 577f0bdd2b8904cbdfde6c98f4bda6fd93a05ffc psql: Tab completion for
> > renaming enum values.
> > 927d7bb6b120a2ca09a164898f887eb850b7a329 Improve tab completion for
> > CREATE TRIGGER.
> > 1d15d0db50a5f39ab69c1fe60f2d5dcc7e2ddb9c psql: Tab-complete LOCK [TABLE]
> > ... IN {ACCESS|ROW|SHARE}.
> >
> > The attached patchset is rebsaed on the master including these
> > patches.
> >
> 
> I checked patches 0001, 0002, 0003 patches
> 
> There are no any problems with patching, compiling, it is working as
> expected
> 
> These patches can be committed separately - they are long, but the code is
> almost mechanical

Thanks. 

You're right. I haven't consider about relations among them.


0001 (if-else refactoring) does not anyting functionally. It is
required by 0004(word-shift-and-removal) and 0005(if-not-exists).

0002 (keywords case improvement) is almost independent from all
other patches in this patch set. And it brings an obvious
improvement.

0003 (addition to 0002) is move embedded keywords out of defined
queries. Functionally can be united to 0002 but separated for
understandability

0004 (word-shift-and-removal) is quite arguable one. This
introduces an ability to modify (or destroy) previous_words
array. This reduces almost redundant matching predicates such as,

>  if (TailMatches3("CREATE|UNIQUE", "INDEX", MatchAny) ||
>      TailMatches4("CREATE|UNIQUE", "INDEX", "CONCURRENTLY", MatchAny))

into

>  if (Matches3("CREATE", "INDEX", MatchAnyExcept("ON")))

by removing "CONCURRENTLY". This obviously simplifies the
predicates literally but it the code implies history of
modification. The implied history might be worse than the
previous shape, especially for the simple cases like this. For a
complex case of CREATE TRIGGER, it seems worse than the original
shape... I'll consider this a bit more. Maybe match-and-collapse
template should be written a predicate.

0005 (if-not-exists). I have admit that this is arguable
feature...

0006 is the terder point:( but.

> The README is perfect

Thank you, I'm relieved by hearing that.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Fri, 25 Nov 2016 06:51:43 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRAm2CsafiH0CPxoWyTccJSm+y=TVgzq07gGS5ydS0qwCA@mail.gmail.com>
> I am sure about benefit of all patches - but it is lot of changes in one
> moment, and it is not necessary in this moment.
> 
> patches 0004 and 0005 does some bigger mental changes, and the work can be
> separated.

The patches are collestions of sporadic changes on the same
basis. I agree that the result is too big too look at. (And the
code itself is confused) Please wait for a while for separated
patches.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Thanks for reviewing but I ran out of time for this CF..

I'm going to move this to the next CF.

At Fri, 25 Nov 2016 15:14:27 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20161125.151427.53669441.horiguchi.kyotaro@lab.ntt.co.jp>
> Hello,
> 
> At Fri, 25 Nov 2016 06:51:43 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRAm2CsafiH0CPxoWyTccJSm+y=TVgzq07gGS5ydS0qwCA@mail.gmail.com>
> > I am sure about benefit of all patches - but it is lot of changes in one
> > moment, and it is not necessary in this moment.
> > 
> > patches 0004 and 0005 does some bigger mental changes, and the work can be
> > separated.
> 
> The patches are collestions of sporadic changes on the same
> basis. I agree that the result is too big too look at. (And the
> code itself is confused) Please wait for a while for separated
> patches.

I'm working on it but ran out of time. I'm going to be unplugged
until next Monday. So I move this to the next CF.

The attached pathes are an incomplete work of the
separation. Still contains some confusions among patches but far
easier to look at, I think.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

> Thanks for reviewing but I ran out of time for this CF..
> 
> I'm going to move this to the next CF.

I splitted the patch into small pieces. f3fd531 conflicted to
this so rebased onto the current master HEAD.

0001 is psql_completion refactoring.
0002-0003 are patches prividing new infrastructures.
0004 is README for the infrastructures.
0005 is letter-case correction of SET/RESET/SHOW using 0002.
0006-0008 are improvements of recursive syntaxes using 0001 and 0004.
0009-0016 are simplifying (maybe) completion code per syntax.

The last one (0017) is the IF(NOT)EXIST modifications. It
suggests if(not)exists for syntaxes already gets object
suggestion. So some kind of objects like operator, cast and so
don't get an if.. suggestion. Likewise, I intentionally didn't
modified siggestions for "TEXT SEARCH *".

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Mon, 26 Dec 2016 14:24:33 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRD2qq6v0jm6kqmWMwo-yNSvn8Vvf+m=DBy3ps=Y0_3yPA@mail.gmail.com>
pavel.stehule> 2016-12-26 9:40 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp
> >:
> 
> > > Thanks for reviewing but I ran out of time for this CF..
> > >
> > > I'm going to move this to the next CF.
> >
> > I splitted the patch into small pieces. f3fd531 conflicted to
> > this so rebased onto the current master HEAD.
> >
> > 0001 is psql_completion refactoring.
> > 0002-0003 are patches prividing new infrastructures.
> > 0004 is README for the infrastructures.
> > 0005 is letter-case correction of SET/RESET/SHOW using 0002.
> > 0006-0008 are improvements of recursive syntaxes using 0001 and 0004.
> > 0009-0016 are simplifying (maybe) completion code per syntax.
> >
> > The last one (0017) is the IF(NOT)EXIST modifications. It
> > suggests if(not)exists for syntaxes already gets object
> > suggestion. So some kind of objects like operator, cast and so
> > don't get an if.. suggestion. Likewise, I intentionally didn't
> > modified siggestions for "TEXT SEARCH *".
> >
> >
> lot of patches. I hope I look on these patches this week.

Thank you for looking this and sorry for the many files. But I
hople that they would be far easier to read.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Dec 27, 2016 at 12:23 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Hello,
>
> At Mon, 26 Dec 2016 14:24:33 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRD2qq6v0jm6kqmWMwo-yNSvn8Vvf+m=DBy3ps=Y0_3yPA@mail.gmail.com>
> pavel.stehule> 2016-12-26 9:40 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp
>> >:
>>
>> > > Thanks for reviewing but I ran out of time for this CF..
>> > >
>> > > I'm going to move this to the next CF.
>> >
>> > I splitted the patch into small pieces. f3fd531 conflicted to
>> > this so rebased onto the current master HEAD.
>> >
>> > 0001 is psql_completion refactoring.
>> > 0002-0003 are patches prividing new infrastructures.
>> > 0004 is README for the infrastructures.
>> > 0005 is letter-case correction of SET/RESET/SHOW using 0002.
>> > 0006-0008 are improvements of recursive syntaxes using 0001 and 0004.
>> > 0009-0016 are simplifying (maybe) completion code per syntax.
>> >
>> > The last one (0017) is the IF(NOT)EXIST modifications. It
>> > suggests if(not)exists for syntaxes already gets object
>> > suggestion. So some kind of objects like operator, cast and so
>> > don't get an if.. suggestion. Likewise, I intentionally didn't
>> > modified siggestions for "TEXT SEARCH *".
>> >
>> >
>> lot of patches. I hope I look on these patches this week.
>
> Thank you for looking this and sorry for the many files. But I
> hople that they would be far easier to read.

The current patch status was "waiting on author", but that's incorrect
as a new series of this patch has been sent. Please be careful with
the status of the CF app! I am moving it to next CF with "needs
review". Gerdan Santos has marked himself as a reviewer of the patch
but I saw no activity, so I have removed his name to not confuse
people looking for patches to review (that happens!).
-- 
Michael

On Tue, Jan 31, 2017 at 2:53 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I tested new set of these patches and I found some regressions there -
> mentioned in my last mail.
>
> Maybe I miss new update, bit I don't know about it.

The last update I am aware of is that saying: "lot of patches. I hope
I look on these patches this week.". Here is the message:
https://www.postgresql.org/message-id/CAFj8pRD2qq6v0jm6kqmWMwo-yNSvn8Vvf+m=DBy3ps=Y0_3yPA@mail.gmail.com

And there is no sign of reviews on the new versions.
-- 
Michael

On Tue, Jan 31, 2017 at 2:58 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I found a error - I sent mail only to author 2016-12-31 :( - It is my
> mistake. I am sorry

Ah... Thanks for the update. No problem.
-- 
Michael

At Tue, 31 Jan 2017 15:07:55 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqTCuK1qVQNwPvnxhA9trxp228v0X6A4gKQb2Uc=mc+adQ@mail.gmail.com>
> On Tue, Jan 31, 2017 at 2:58 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > I found a error - I sent mail only to author 2016-12-31 :( - It is my
> > mistake. I am sorry
> 
> Ah... Thanks for the update. No problem.

Ouch. Sorry for missing you commnet. I'll dig my mail box for that.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Thank you for reviewing.

At Tue, 31 Jan 2017 11:28:17 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRADxoycR=PqmtdJy0tBrYAWaQ0_xt6t-QBX_U8_ROGCwQ@mail.gmail.com>
> I am doing a review of this set of patches:
> 
> 1. There are no problem with patching, compiling - all regress tests passed

Oh! My new check script made from Michael's suggestion finds one
trailing space in the patch. And PUBLICATION/SUBSCRIPTION
conflicats with this so I'll post the reased version.

> 2. tab complete doesn't work well if I am manually writing "create index
> on" - second "ON" is appended - it is a regression

I'll fix it in the version.

> I didn't find any other issues -
> 
> note: not necessary to implement (nice to have) - I miss a support for OR
> REPLACE flag - it is related to LANGUAGE, TRANSFORMATION,  FUNCTION and
> RULE.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello. This is the new version of this patch. 

- Rebased to the current master (555494d) PUBLICATION/SUBSCRIPTION stuff conflicted.

- Fix a bug of CREATE INDEX(0012-Simplify-completion-for-CREATE-INDEX.patch). CREATE INDEX ON no longer gets a
suggestionof "ON".
 

- Added logging feature (0018-Debug-output-of-psql-completion.patch)
 This might be suitable to be a separate patch. psql completion code is defficult to debug when it is uncertain what
linedid a suggestion. This patch allows completion logs to psql log, which is activated by -L option.
 
 psql -L <logfile> <dbname>
 And the logs like the following will be printed.
 | completion performed at tab-complete.c:1146 for "do"

- OR REPLACE suggestion (0019-Add-suggestion-of-OR-REPLACE.patch)

At Wed, 1 Feb 2017 09:42:54 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRAHCwdwe+NRTQ9JrtMO2OdUWtp1demmv_jGBU2tRRs-CQ@mail.gmail.com>
> 2017-02-01 9:37 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp
> the content of my last mail is a copy my mail from end of December.
> Probably lot of changes there.

Thanks for reposting.

> > > 2. tab complete doesn't work well if I am manually writing "create index
> > > on" - second "ON" is appended - it is a regression
> >
> > I'll fix it in the version.
> >
> > > I didn't find any other issues -
> > >
> > > note: not necessary to implement (nice to have) - I miss a support for OR
> > > REPLACE flag - it is related to LANGUAGE, TRANSFORMATION,  FUNCTION and
> > > RULE.

Hmm. This patch perhaps should not 'add a feature' (how about the
logging..). Anyway the last 19th patch does that.  The word
removal framework works well for this case.

After all, this patch is so large that I'd like to attach them as
one compressed file. The content of the file is the following.


0001-Refactoring-tab-complete-to-make-psql_completion-cod.patch - Just a refactoring of psql_completion

0002-Make-keywords-case-follow-to-input.patch - The letter case of additional suggestions for   COMPLETION_WITH_XX
followsinput.
 

0003-Introduce-word-shift-and-removal-feature-to-psql-com.patch - A feature to ignore preceding words. And a feature to
remove  intermediate words.
 

0004-Add-README-for-tab-completion.patch - README

0005-Make-SET-RESET-SHOW-varialble-follow-input-letter-ca.patch
0006-Allow-complete-schema-elements-in-more-natural-way.patch
0007-Allow-CREATE-RULE-to-use-command-completion-recursiv.patch
0008-Allow-completing-the-body-of-EXPLAIN.patch
0009-Simpilfy-ALTER-TABLE-ALTER-COLUMN-completion.patch
0010-Simplify-completion-for-CLUSTER-VERBOSE.patch
0011-Simplify-completion-for-COPY.patch
0012-Simplify-completion-for-CREATE-INDEX.patch
0013-Simplify-completion-for-CREATE-SEQUENCE.patch
0014-Simplify-completion-for-DROP-INDEX.patch
0015-Add-CURRENT_USER-to-some-completions-of-role.patch
0016-Refactor-completion-for-ALTER-DEFAULT-PRIVILEGES.patch
0017-Add-suggestions-of-IF-NOT-EXISTS.patch - A kind of sample refctoring (or augmenting) suggestion code   based on
thenew infrastructure.
 

0018-Debug-output-of-psql-completion.patch - Debug logging for psql_completion (described above)

0019-Add-suggestion-of-OR-REPLACE.patch - Suggestion of CREATE OR REPLACE.


# I hear the footsteps of another conflict..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Thank you for the comment.

At Mon, 6 Feb 2017 17:10:43 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRD85cnxEEgLtOoqL-Bda2XpzvHB3a6Mr+bvf+OKpiq3Eg@mail.gmail.com>
> > 0001-Refactoring-tab-complete-to-make-psql_completion-cod.patch
> >   - Just a refactoring of psql_completion
> >
> > 0002-Make-keywords-case-follow-to-input.patch
> >   - The letter case of additional suggestions for
> >     COMPLETION_WITH_XX follows input.
> >
> > 0003-Introduce-word-shift-and-removal-feature-to-psql-com.patch
> >   - A feature to ignore preceding words. And a feature to remove
> >     intermediate words.
> >
> > 0004-Add-README-for-tab-completion.patch
> >   - README
> >
> > 0005-Make-SET-RESET-SHOW-varialble-follow-input-letter-ca.patch
> > 0006-Allow-complete-schema-elements-in-more-natural-way.patch
> > 0007-Allow-CREATE-RULE-to-use-command-completion-recursiv.patch
> > 0008-Allow-completing-the-body-of-EXPLAIN.patch
> > 0009-Simpilfy-ALTER-TABLE-ALTER-COLUMN-completion.patch
> > 0010-Simplify-completion-for-CLUSTER-VERBOSE.patch
> > 0011-Simplify-completion-for-COPY.patch
> > 0012-Simplify-completion-for-CREATE-INDEX.patch
> > 0013-Simplify-completion-for-CREATE-SEQUENCE.patch
> > 0014-Simplify-completion-for-DROP-INDEX.patch
> > 0015-Add-CURRENT_USER-to-some-completions-of-role.patch
> > 0016-Refactor-completion-for-ALTER-DEFAULT-PRIVILEGES.patch
> > 0017-Add-suggestions-of-IF-NOT-EXISTS.patch
> >   - A kind of sample refctoring (or augmenting) suggestion code
> >     based on the new infrastructure.
> >
> > 0018-Debug-output-of-psql-completion.patch
> >   - Debug logging for psql_completion (described above)
> >
> > 0019-Add-suggestion-of-OR-REPLACE.patch
> >   - Suggestion of CREATE OR REPLACE.
> >
> >
> > # I hear the footsteps of another conflict..
> >
> The patch 0018 was not be applied.

The fear came true. fd6cd69 conflicts with it but on a
comment. The attached patch set applies on top of the current
master head (ae0e550).

> Few other notes from testing - probably these notes should not be related
> to your patch set
> 
> 1. When we have set of keywords, then the upper or lower chars should to
> follow previous keyword. Is it possible? It should to have impact only on
> keywords.

It sounds reasonable, more flexible than "upper"/"lower" of
COMP_KEYWORD_CASE. The additional 20th(!) patch does that. It
adds a new value 'follow-first' to COMP_KEYWORD_CASE. All
keywords in a command line will be in the case of the first
letter of the first word. ("CREATE" in the following case. I
think it is enogh for the purpose.)

postgres=# \set COMP_KEYWORD_CASE follow-first
postgres=# CREATE in<tab>      =># CREATE INDEX hoge <tab>      =># CREATE INDEX hoge ON emp<tab>      =># CREATE INDEX
hogeON employee ..
 
postgres=# create IN<tab>      =># create index

Typing tab at the first in a command line shows all available
keywords in upper case.

> 2. the list of possible functions after EXECUTE PROCEDURE in CREATE TRIGGER
> statement should be reduced to trigger returns function only.

Actually Query_for_list_of_trigger_functions returns too many
candidates. The suggested restriction reduces them to a
reasonable number. The 21th patch does that.

> CREATE OR REPLACE FUNCTIONS works great, thank you!

Thanks. It was easier than expected.

As the result, 21 paches are attached to this message. 1 - 19th
are described above and others are described below.

0020-New-COMP_KEYWORD_CASE-mode-follow-first.patch
 - Add new COMP_KEYWORD_CASE mode "follow-first". The completion   works with the case of the first word. (This doesn't
relyon   this patchset but works in more cases with 0002)
 

0021-Suggest-only-trigger-functions-for-CREAET-TRIGGER.EX.patch - Restrict suggestion for the syntax to ones acutually
usable  there. (This relies on none of this patchset, though..)
 

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Feb 22, 2017 at 12:38 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Now first patch is broken :(
>
> It is pretty sensitive to any changes. Isn't possible to commit first four
> patches first and separately maybe out of commitfest window?

Yeah, maybe, but we'd need a committer to take more of an interest in
this patch series.  Personally, I'm wondering why we need a series of
19 patches to add tab completion support for IF NOT EXISTS.  The
feature which is the subject of this thread arrives in patch 0017, and
a lot of the patches which come before that seem to change a lot of
stuff without actually improving much that would really benefit users.

0001 seems like a lot of churn for no real benefit that I can immediately see.
0002 is a real feature, and probably a good one, though unrelated to
the subject of this thread.  In the process, it changes many lines of
code in fairly mechanical ways; does it need to do that?
0003 is infrastructure.
0004 adds a README.  Do we really need that?  It seems to be
explaining things which are mostly fairly clear from just looking at
the code.  If we add a README, we have to update it when we change
things.  That's worthwhile if it helps people write code better, I'm
not sure if it will do that.
0005 extends 0002.
0006 prevents incorrect completions in obscure circumstances.
0007 adds some kind of tab completion for CREATE RULE; I'm fuzzy on the details.
0008 improves tab completion after EXPLAIN.
0009-0014 uses the infrastructure from 0003 to improve tab completion
for various commands.  They say they're merely simplifying tab
completion for those things, but actually they're extending it to some
obscure situations that aren't currently covered.
0015 adds completion for magic keywords like CURRENT_USER when role
commands are used.
0016 refactors tab completion for ALTER DEFAULT PRIVILEGES, possibly
improving it somehow.
0017 implements the titular feature.
0018 adds optional debugging output.
0019 improves things for CREATE OR REPLACE completion.

Phew.  That's a lot of work for relatively obscure improvements to tab
completion.  I grant that the result is probably better, but it's a
lot of code change for what we get out of it.  I'm not saying we
should reject it on that basis, but it may be the reason why nobody's
jumped in to work on getting this committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> Yeah, maybe, but we'd need a committer to take more of an interest in
> this patch series.  Personally, I'm wondering why we need a series of
> 19 patches to add tab completion support for IF NOT EXISTS.  The
> feature which is the subject of this thread arrives in patch 0017, and
> a lot of the patches which come before that seem to change a lot of
> stuff without actually improving much that would really benefit users.

FWIW, one reason this committer hasn't jumped in is that we already
rewrote tab-complete.c pretty completely in 9.6.  If we accept a patch
that completely rewrites it again, we're going to be faced with
maintaining three fundamentally different implementations for the next
three-plus years (until 9.5 dies).  Admittedly, we don't back-patch
fixes in tab-complete.c every week, but a look at the git history says
we do need to do that several times a year.

Also, the nature of the primary refactoring (changing the big else-chain
into standalone ifs, if I read it correctly) is particularly bad from a
back-patching standpoint because all you have to do is insert an "else",
or fail to insert one, to silently and almost completely break either
one or the other branch.  And I don't really understand why that's a good
idea anyway: surely we can return at most one set of completions, so how
is turning the function into a lot of independent actions a win?

So I'd be a whole lot happier if it didn't do that.  Can we really not
add the desired features in a more localized fashion?
        regards, tom lane

On Mon, Feb 27, 2017 at 5:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Yeah, maybe, but we'd need a committer to take more of an interest in
>> this patch series.  Personally, I'm wondering why we need a series of
>> 19 patches to add tab completion support for IF NOT EXISTS.  The
>> feature which is the subject of this thread arrives in patch 0017, and
>> a lot of the patches which come before that seem to change a lot of
>> stuff without actually improving much that would really benefit users.
>
> FWIW, one reason this committer hasn't jumped in is that we already
> rewrote tab-complete.c pretty completely in 9.6.  If we accept a patch
> that completely rewrites it again, we're going to be faced with
> maintaining three fundamentally different implementations for the next
> three-plus years (until 9.5 dies).  Admittedly, we don't back-patch
> fixes in tab-complete.c every week, but a look at the git history says
> we do need to do that several times a year.

Indeed, having worked on the 9.6 refactoring a bit as well... I'll
vote for not doing this again as HEAD is in a more readable shape
compared to the pre-9.5 area, and I am not convinced that it is worth
the trouble. There are a couple of things that can be extracted from
this set of patches, but I would vote for not doing the same level of
refactoring.

> Also, the nature of the primary refactoring (changing the big else-chain
> into standalone ifs, if I read it correctly) is particularly bad from a
> back-patching standpoint because all you have to do is insert an "else",
> or fail to insert one, to silently and almost completely break either
> one or the other branch.  And I don't really understand why that's a good
> idea anyway: surely we can return at most one set of completions, so how
> is turning the function into a lot of independent actions a win?
>
> So I'd be a whole lot happier if it didn't do that.  Can we really not
> add the desired features in a more localized fashion?

As "if not exists" is defined after the object type if would not be
that complicated to add completion for IE/INE after the object type
with a set of THING_* flags in words_after_create. One missing piece
would be to add completion for the objects themselves after IE or INE
have been entered by the user, but I would think that tweaking the
checks on words_after_create[i] would be doable as well. And that
would be localized.
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Mon, Feb 27, 2017 at 5:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So I'd be a whole lot happier if it didn't do that.  Can we really not
>> add the desired features in a more localized fashion?

> As "if not exists" is defined after the object type if would not be
> that complicated to add completion for IE/INE after the object type
> with a set of THING_* flags in words_after_create. One missing piece
> would be to add completion for the objects themselves after IE or INE
> have been entered by the user, but I would think that tweaking the
> checks on words_after_create[i] would be doable as well. And that
> would be localized.

BTW ... can anyone explain to me the reason why we offer to complete
CREATE OBJECT with the names of existing objects of that kind?
That seems pretty darn stupid.  I can see offering the names of existing
schemas there, if the object type is one that has schema-qualified names,
but completing with an existing object name is just setting up to fail
isn't it?

If we dropped that behavior, seems like it would become much easier
to plug in IF NOT EXISTS at those spots.
        regards, tom lane

On Mon, Feb 27, 2017 at 10:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Mon, Feb 27, 2017 at 5:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> So I'd be a whole lot happier if it didn't do that.  Can we really not
>>> add the desired features in a more localized fashion?
>
>> As "if not exists" is defined after the object type if would not be
>> that complicated to add completion for IE/INE after the object type
>> with a set of THING_* flags in words_after_create. One missing piece
>> would be to add completion for the objects themselves after IE or INE
>> have been entered by the user, but I would think that tweaking the
>> checks on words_after_create[i] would be doable as well. And that
>> would be localized.
>
> BTW ... can anyone explain to me the reason why we offer to complete
> CREATE OBJECT with the names of existing objects of that kind?
> That seems pretty darn stupid.  I can see offering the names of existing
> schemas there, if the object type is one that has schema-qualified names,
> but completing with an existing object name is just setting up to fail
> isn't it?

Isn't that to facilitate commands appended after CREATE SCHEMA? Say
table foo is in schema1, and creating it in schema2 gets easier with
tab completion?
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Mon, Feb 27, 2017 at 10:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW ... can anyone explain to me the reason why we offer to complete
>> CREATE OBJECT with the names of existing objects of that kind?

> Isn't that to facilitate commands appended after CREATE SCHEMA? Say
> table foo is in schema1, and creating it in schema2 gets easier with
> tab completion?

Seems like pretty much of a stretch.  I've never done anything like
that, have you?
        regards, tom lane

On Mon, Feb 27, 2017 at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Mon, Feb 27, 2017 at 10:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> BTW ... can anyone explain to me the reason why we offer to complete
>>> CREATE OBJECT with the names of existing objects of that kind?
>
>> Isn't that to facilitate commands appended after CREATE SCHEMA? Say
>> table foo is in schema1, and creating it in schema2 gets easier with
>> tab completion?
>
> Seems like pretty much of a stretch.  I've never done anything like
> that, have you?

Never, but that was the only reason I could think about. I recall
reading something else on -hackers but I cannot put my finger on it,
nor does a lookup at the archives help... Perhaps that's the one I
just mentioned as well.
-- 
Michael

At Mon, 27 Feb 2017 10:43:39 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqQr7apg8W+p41W1azTjy7LSasSEvWvKePTU4knnxWCZkw@mail.gmail.com>
> On Mon, Feb 27, 2017 at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Michael Paquier <michael.paquier@gmail.com> writes:
> >> On Mon, Feb 27, 2017 at 10:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> BTW ... can anyone explain to me the reason why we offer to complete
> >>> CREATE OBJECT with the names of existing objects of that kind?
> >
> >> Isn't that to facilitate commands appended after CREATE SCHEMA? Say
> >> table foo is in schema1, and creating it in schema2 gets easier with
> >> tab completion?
> >
> > Seems like pretty much of a stretch.  I've never done anything like
> > that, have you?
> 
> Never, but that was the only reason I could think about. I recall
> reading something else on -hackers but I cannot put my finger on it,
> nor does a lookup at the archives help... Perhaps that's the one I
> just mentioned as well.

I suppose it is for suggesting what kind of word should come
there, or avoiding silence for a tab. Or for symmetry with other
types of manipulation, like DROP. Another possibility is creating
multiple objects with similar names, say CREATE TABLE
employee_x1, CREATE TABLE employee_x2. Just trying to complete
existing *schema* is one more another possible objective.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

* Kyotaro HORIGUCHI (horiguchi.kyotaro@lab.ntt.co.jp) wrote:
> I suppose it is for suggesting what kind of word should come
> there, or avoiding silence for a tab. Or for symmetry with other
> types of manipulation, like DROP. Another possibility is creating
> multiple objects with similar names, say CREATE TABLE
> employee_x1, CREATE TABLE employee_x2. Just trying to complete
> existing *schema* is one more another possible objective.

I don't buy any of these arguments either.  I *really* don't want us
going down some road where we try to make sure that hitting 'tab' never
fails...

Thanks!

Stephen

On Mon, Feb 27, 2017 at 11:53:17PM -0500, Stephen Frost wrote:
> * Kyotaro HORIGUCHI (horiguchi.kyotaro@lab.ntt.co.jp) wrote:
> > I suppose it is for suggesting what kind of word should come
> > there, or avoiding silence for a tab. Or for symmetry with other
> > types of manipulation, like DROP. Another possibility is creating
> > multiple objects with similar names, say CREATE TABLE employee_x1,
> > CREATE TABLE employee_x2. Just trying to complete existing
> > *schema* is one more another possible objective.
> 
> I don't buy any of these arguments either.  I *really* don't want us
> going down some road where we try to make sure that hitting 'tab'
> never fails...

Wouldn't that just be a correct, grammar-aware implementation of tab
completion?  Why wouldn't you want that?

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

* David Fetter (david@fetter.org) wrote:
> On Mon, Feb 27, 2017 at 11:53:17PM -0500, Stephen Frost wrote:
> > * Kyotaro HORIGUCHI (horiguchi.kyotaro@lab.ntt.co.jp) wrote:
> > > I suppose it is for suggesting what kind of word should come
> > > there, or avoiding silence for a tab. Or for symmetry with other
> > > types of manipulation, like DROP. Another possibility is creating
> > > multiple objects with similar names, say CREATE TABLE employee_x1,
> > > CREATE TABLE employee_x2. Just trying to complete existing
> > > *schema* is one more another possible objective.
> >
> > I don't buy any of these arguments either.  I *really* don't want us
> > going down some road where we try to make sure that hitting 'tab'
> > never fails...
>
> Wouldn't that just be a correct, grammar-aware implementation of tab
> completion?  Why wouldn't you want that?

No, it wouldn't, it would mean we have to provide something for cases
where it doesn't make sense to try and provide an answer, as being
discussed here for CREATE TABLE.

We can't provide an answer based on tab-completion to what you want to
call your new table.

Thanks!

Stephen

Hello,

At Tue, 28 Feb 2017 10:39:01 -0500, Stephen Frost <sfrost@snowman.net> wrote in
<20170228153901.GH9812@tamriel.snowman.net>
> * David Fetter (david@fetter.org) wrote:
> > On Mon, Feb 27, 2017 at 11:53:17PM -0500, Stephen Frost wrote:
> > > * Kyotaro HORIGUCHI (horiguchi.kyotaro@lab.ntt.co.jp) wrote:
> > > > I suppose it is for suggesting what kind of word should come
> > > > there, or avoiding silence for a tab. Or for symmetry with other
> > > > types of manipulation, like DROP. Another possibility is creating
> > > > multiple objects with similar names, say CREATE TABLE employee_x1,
> > > > CREATE TABLE employee_x2. Just trying to complete existing
> > > > *schema* is one more another possible objective.
> > > 
> > > I don't buy any of these arguments either.  I *really* don't want us
> > > going down some road where we try to make sure that hitting 'tab'
> > > never fails...

These suggestions exist before this patch. Whether to remove them
would be another discussion. I was going to add some but finally
I believe I have added no such things in this patchset.

> > Wouldn't that just be a correct, grammar-aware implementation of tab
> > completion?  Why wouldn't you want that?
> 
> No, it wouldn't, it would mean we have to provide something for cases
> where it doesn't make sense to try and provide an answer, as being
> discussed here for CREATE TABLE.
> 
> We can't provide an answer based on tab-completion to what you want to
> call your new table.

The difference seems to be that what we take this feature to
be. If we see it as just a fast-path of entering a word where we
know what words should come, silence is not a problem. If we see
it as safety-wheels to guide users to the right way to go,
silence would be bad. A silence during word completion suggests
something wrong in the preceding words to me so it is a bit
annoying.

As an analogous operation, mkdir on bash suggests existing
directories. We can suggest existing tables for CREATE TABLE with
the same basis.

Another possible way to go would be showing a 'suggestion' not a
list of possibilities. If readline allows such operation, I
imagine the following. But this is a quite diferrent discussion.

=# CREATE TABLE <tab>
<<a table name to create>>
=# CREATE TABLE table1 <tab>

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

On 3/1/17 9:38 PM, Kyotaro HORIGUCHI wrote:

> At Tue, 28 Feb 2017 10:39:01 -0500, Stephen Frost <sfrost@snowman.net> wrote in
<20170228153901.GH9812@tamriel.snowman.net>
>> * David Fetter (david@fetter.org) wrote:
>>> On Mon, Feb 27, 2017 at 11:53:17PM -0500, Stephen Frost wrote:
>>>> * Kyotaro HORIGUCHI (horiguchi.kyotaro@lab.ntt.co.jp) wrote:
>>>>> I suppose it is for suggesting what kind of word should come
>>>>> there, or avoiding silence for a tab. Or for symmetry with other
>>>>> types of manipulation, like DROP. Another possibility is creating
>>>>> multiple objects with similar names, say CREATE TABLE employee_x1,
>>>>> CREATE TABLE employee_x2. Just trying to complete existing
>>>>> *schema* is one more another possible objective.
>>>>
>>>> I don't buy any of these arguments either.  I *really* don't want us
>>>> going down some road where we try to make sure that hitting 'tab'
>>>> never fails...
> 
> These suggestions exist before this patch. Whether to remove them
> would be another discussion. I was going to add some but finally
> I believe I have added no such things in this patchset.
> 
>>> Wouldn't that just be a correct, grammar-aware implementation of tab
>>> completion?  Why wouldn't you want that?
>>
>> No, it wouldn't, it would mean we have to provide something for cases
>> where it doesn't make sense to try and provide an answer, as being
>> discussed here for CREATE TABLE.
>>
>> We can't provide an answer based on tab-completion to what you want to
>> call your new table.
> 
> The difference seems to be that what we take this feature to
> be. If we see it as just a fast-path of entering a word where we
> know what words should come, silence is not a problem. If we see
> it as safety-wheels to guide users to the right way to go,
> silence would be bad. A silence during word completion suggests
> something wrong in the preceding words to me so it is a bit
> annoying.
> 
> As an analogous operation, mkdir on bash suggests existing
> directories. We can suggest existing tables for CREATE TABLE with
> the same basis.
> 
> Another possible way to go would be showing a 'suggestion' not a
> list of possibilities. If readline allows such operation, I
> imagine the following. But this is a quite diferrent discussion.
> 
> =# CREATE TABLE <tab>
> <<a table name to create>>
> =# CREATE TABLE table1 <tab>
> 
> regards,

It has been a while since this thread has received any comments or a new
patch.  The general consensus seems to be that this feature is too large
a rewrite of tab completion considering a major rewrite was done for 9.6.

Are you considering writing a localized patch for this feature as Tom
suggested?  If so, please post that by 2017-03-16 AoE.

If no new patch is submitted by that date I will mark this submission
"Returned with Feedback".

Thanks,
-- 
-David
david@pgmasters.net

At Mon, 13 Mar 2017 10:42:05 -0400, David Steele <david@pgmasters.net> wrote in
<1e8297fd-f7f2-feab-848d-5121e45c8cba@pgmasters.net>
> It has been a while since this thread has received any comments or a new
> patch.  The general consensus seems to be that this feature is too large
> a rewrite of tab completion considering a major rewrite was done for 9.6.
> 
> Are you considering writing a localized patch for this feature as Tom
> suggested?  If so, please post that by 2017-03-16 AoE.
> 
> If no new patch is submitted by that date I will mark this submission
> "Returned with Feedback".

It's a pity. I had to take a week's leave..

I understood that the 'localized fashion' means "without removing
eles's", that is the core of this patch. So, if it is not
acceptable this should be abandoned. I'll try put the inidividual
enahancements other than refactoring in other shape, in the next
commitfest.

Thanks.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 3/17/17 3:58 AM, Kyotaro HORIGUCHI wrote:
> At Mon, 13 Mar 2017 10:42:05 -0400, David Steele <david@pgmasters.net> wrote in
<1e8297fd-f7f2-feab-848d-5121e45c8cba@pgmasters.net>
>> It has been a while since this thread has received any comments or a new
>> patch.  The general consensus seems to be that this feature is too large
>> a rewrite of tab completion considering a major rewrite was done for 9.6.
>>
>> Are you considering writing a localized patch for this feature as Tom
>> suggested?  If so, please post that by 2017-03-16 AoE.
>>
>> If no new patch is submitted by that date I will mark this submission
>> "Returned with Feedback".
> 
> It's a pity. I had to take a week's leave..
> 
> I understood that the 'localized fashion' means "without removing
> eles's", that is the core of this patch. So, if it is not
> acceptable this should be abandoned. I'll try put the inidividual
> enahancements other than refactoring in other shape, in the next
> commitfest.

I have marked this submission "Returned with Feedback".  Please feel
free to resubmit when you have a new version.

-- 
-David
david@pgmasters.net