Обсуждение: CREATE tab completion

Hi hackers,

I noticed that there are some tab completions missing for the following 
commands:
-CREATE CONVERSION : missing FOR, TO, FROM
-CREATE DOMAIN : missing after AS
-CREATE LANGUAGE : missing after HANDLER
-CREATE SCHEMA : missing AUTHORIZATION, IF NOT EXISTS
-CREATE SEQUENCE : missing AS
-CREATE TRANSFORM : missing after FOR

I made a patch for this, so please have a look.

Best wishes,

-- 
Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Nov 17, 2021 at 10:44:44AM +0900, Ken Kato wrote:
> I made a patch for this, so please have a look.

+       else if (Matches("CREATE", "TRANSFORM", "FOR", MatchAny)
+               COMPLETE_WITH("LANGUAGE")
+       else if (Matches("CREATE", "TRANSFORM", "FOR", MatchAny, "LANGUAGE")

Those three lines are wrong, for two different reasons and three
mistakes.  You may want to compile your code before sending it :)

"CREATE [TEMP|TEMPORARY] SEQUENCE name AS" could be completed with the
supported types.  There are three of them.

+               COMPLETE_WITH_QUERY(Query_for_list_of_schemas
+                                                       " UNION SELECT 'AUTORIZATION'");
Incorrect completion here, s/AUTORIZATION/AUTHORIZATION/.

+       else if (Matches("CREATE", "CONVERSION", MatchAny))
+               COMPLETE_WITH("FOR");
Why didn't you consider DEFAULT for the set of changes with
conversions?

+       else if (Matches("CREATE", "SCHEMA"))
+               COMPLETE_WITH("AUTHORIZATION", "IF NOT EXISTS");
+       else if (Matches("CREATE", "SCHEMA", "IF", "NOT", "EXISTS"))
We don't do any completion for INE or IE in the other object types.

+       /* CREATE LANGUAGE */
+       else if (Matches("CREATE", "LANGUAGE", MatchAny))
+               COMPLETE_WITH("HANDLER");
+       else if (Matches("CREATE", "LANGUAGE", MatchAny, "HANDLER", MatchAny))
+               COMPLETE_WITH("INLINE", "VALIDATOR");
It looks like you forgot the case of "OR REPLACE" here?  This is done
for triggers, for example.
--
Michael

> +       else if (Matches("CREATE", "TRANSFORM", "FOR", MatchAny)
> +               COMPLETE_WITH("LANGUAGE")
> +       else if (Matches("CREATE", "TRANSFORM", "FOR", MatchAny, 
> "LANGUAGE")
> 
> Those three lines are wrong, for two different reasons and three
> mistakes.  You may want to compile your code before sending it :)
> 
> +               COMPLETE_WITH_QUERY(Query_for_list_of_schemas
> +                                                       " UNION SELECT
> 'AUTORIZATION'");
> Incorrect completion here, s/AUTORIZATION/AUTHORIZATION/.

Thank you for the comments.

I am sorry for the compile error and a typo. I will make sure to compile 
before sending it and double check typos.


> "CREATE [TEMP|TEMPORARY] SEQUENCE name AS" could be completed with the
> supported types.  There are three of them.

For this part, I did the following:
+    else if (TailMatches("CREATE", "SEQUENCE", MatchAny, "AS") ||
+             TailMatches("CREATE", "TEMP|TEMPORARY", "SEQUENCE", MatchAny, 
"AS"))
+        COMPLETE_WITH("smallint", "integer", "bigint");

Am I doing this right? or Are there better ways to do it?


Best wishes,

-- 
Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Nov 18, 2021 at 04:25:30PM +0900, Ken Kato wrote:
> For this part, I did the following:
> +    else if (TailMatches("CREATE", "SEQUENCE", MatchAny, "AS") ||
> +             TailMatches("CREATE", "TEMP|TEMPORARY", "SEQUENCE", MatchAny, "AS"))
> +        COMPLETE_WITH("smallint", "integer", "bigint");
>
> Am I doing this right? or Are there better ways to do it?

That looks fine per se.

> +/* CREATE SCHEMA */
> +    else if (Matches("CREATE", "SCHEMA"))
> +        COMPLETE_WITH("AUTHORIZATION");
> +    else if (Matches("CREATE", "SCHEMA") && TailMatches("AUTHORIZATION"))
> +        COMPLETE_WITH_QUERY(Query_for_list_of_roles);

The part about CREATE SCHEMA was itching me a bit, until I recalled
this recent thread which has a more complete logic:
https://www.postgresql.org/message-id/87im0efqhp.fsf@wibble.ilmari.org

The rest looks good, I'll take care of that in a bit.
--
Michael

At Thu, 18 Nov 2021 17:17:25 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Thu, Nov 18, 2021 at 04:25:30PM +0900, Ken Kato wrote:
> > For this part, I did the following:
> > +    else if (TailMatches("CREATE", "SEQUENCE", MatchAny, "AS") ||
> > +             TailMatches("CREATE", "TEMP|TEMPORARY", "SEQUENCE", MatchAny, "AS"))
> > +        COMPLETE_WITH("smallint", "integer", "bigint");
> > 
> > Am I doing this right? or Are there better ways to do it?
> 
> That looks fine per se.

FWIW, I would be a bit perplexed to see type names suggested in
upper-cases, even if it is acceptable by the parser.  If you type-in
the following phrase:

=# type boo<tab>

it is completed to "boolean" but,

=# type BOO<tab>

doesn't respond.

So we could use COMPLETE_WITH_CS instead so that CREATE SEQUENCE
behavess the same way with the existing behavior of TYPE.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Nov 18, 2021 at 05:20:58PM +0900, Kyotaro Horiguchi wrote:
> So we could use COMPLETE_WITH_CS instead so that CREATE SEQUENCE
> behavess the same way with the existing behavior of TYPE.

Makes sense.

Another issue I have noticed with the patch is that it forgets to
apply quotes for the FROM/TO clauses of CREATE CONVERSION, making the
queries fail.  \encoding does not care about that but we do for this
set of DDLs.  So I have discarded this part.

One extra issue was in CREATE TRANSFORM, where we should handle "OR
REPLACE" like the others.  And a last issue I had was with CREATE
LANGUAGE where we would miss TRUSTED.  This last one cannot be
completed, but we'd better allow its completion if we can.  That makes
things a bit tense if you add on top of that the handling of "OR
REPLACE".

I have fixed (or discarded) that, and the parts for sequences, domains
and transforms remained.  That looked like good enough on its own, so
applied those parts of the patch.
--
Michael

> I have fixed (or discarded) that, and the parts for sequences, domains
> and transforms remained.  That looked like good enough on its own, so
> applied those parts of the patch.
> --
> Michael
Thank you very much!

I assume Michael has committed the modified version of the patch.
Therefore, I changed the status to"committed" in Commitfest 2022-01.
https://commitfest.postgresql.org/36/3418/

Best wishes,

-- 
Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION