[PATCH] Improve tab completion for CREATE TABLE

Hi hackers,

Please find attached a patch that adds the following tab completions for
CREATE TABLE:

- ( or PARTITION OF after the name
- options after the column list
- ON COMMIT actions for temp tables

Regards,

ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:

> Hi hackers,
>
> Please find attached a patch that adds the following tab completions for
> CREATE TABLE:

Added to the 2019-01 commitfest: https://commitfest.postgresql.org/21/1895/

- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen

On Fri, Nov 30, 2018 at 03:44:38PM +0000, Dagfinn Ilmari Mannsåker wrote:
> ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:
>> Please find attached a patch that adds the following tab completions for
>> CREATE TABLE:
>
> Added to the 2019-01 commitfest: https://commitfest.postgresql.org/21/1895/

+ else if (TailMatches("CREATE", "TABLE", MatchAny) ||
+ TailMatches("CREATE", "TEMP|TEMPORARY|UNLOGGED", "TABLE", MatchAny))
+ COMPLETE_WITH("(", "PARTITION OF");
This is missing the completion of "OF TYPE" (no need to print the type
matches).

+ else if (TailMatches("CREATE", "TABLE", MatchAny, "(*)") ||
+ TailMatches("CREATE", "UNLOGGED", "TABLE", MatchAny, "(*)"))
+ COMPLETE_WITH("INHERITS (", "PARTITION BY", "WITH (", "TABLESPACE")
Temporary tables can be part of a partition tree, with the parent being
temporary or permanent.

+ /* Complete ON COMMIT actions for temp tables */
+ else if (TailMatches("ON", "COMMIT"))
+ COMPLETE_WITH("PRESERVE ROWS", "DELETE ROWS", "DROP");
This causes ON COMMIT to show up for permanent and unlogged tables.
--
Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:

> On Fri, Nov 30, 2018 at 03:44:38PM +0000, Dagfinn Ilmari Mannsåker wrote:
>> ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:
>>> Please find attached a patch that adds the following tab completions for
>>> CREATE TABLE:
>>
>> Added to the 2019-01 commitfest: https://commitfest.postgresql.org/21/1895/
>
> + else if (TailMatches("CREATE", "TABLE", MatchAny) ||
> + TailMatches("CREATE", "TEMP|TEMPORARY|UNLOGGED", "TABLE", MatchAny))
> + COMPLETE_WITH("(", "PARTITION OF");
>
> This is missing the completion of "OF TYPE" (no need to print the type
> matches).

Good catch. I've added that, and separate completion of composite type
names after it in the attached v2 patch.

> + else if (TailMatches("CREATE", "TABLE", MatchAny, "(*)") ||
> + TailMatches("CREATE", "UNLOGGED", "TABLE", MatchAny, "(*)"))
> + COMPLETE_WITH("INHERITS (", "PARTITION BY", "WITH (", "TABLESPACE")
>
> Temporary tables can be part of a partition tree, with the parent being
> temporary or permanent.

Yes, the next clause completes all the same options as non-temporary
tables, plus "ON COMMIT":

+ else if (TailMatches("CREATE", "TEMP|TEMPORARY", "TABLE", MatchAny, "(*)"))
+ COMPLETE_WITH("INHERITS (", "PARTITION BY", "WITH (", "ON COMMIT", "TABLESPACE");

> + /* Complete ON COMMIT actions for temp tables */
> + else if (TailMatches("ON", "COMMIT"))
> + COMPLETE_WITH("PRESERVE ROWS", "DELETE ROWS", "DROP");
>
> This causes ON COMMIT to show up for permanent and unlogged tables.

No, this is for completing _after_ ON COMMIT, which is only suggested by
the clause for temporary tables above.

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:

> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>
>> On Fri, Nov 30, 2018 at 03:44:38PM +0000, Dagfinn Ilmari Mannsåker wrote:
>>> ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:
>>>> Please find attached a patch that adds the following tab completions for
>>>> CREATE TABLE:
>>>
>>> Added to the 2019-01 commitfest: https://commitfest.postgresql.org/21/1895/
>>
>> + else if (TailMatches("CREATE", "TABLE", MatchAny) ||
>> + TailMatches("CREATE", "TEMP|TEMPORARY|UNLOGGED", "TABLE", MatchAny))
>> + COMPLETE_WITH("(", "PARTITION OF");
>>
>> This is missing the completion of "OF TYPE" (no need to print the type
>> matches).
>
> Good catch. I've added that, and separate completion of composite type
> names after it in the attached v2 patch.

Oops, that had a typo after I fiddled with the formatting of the
selcondition in the query and forgot to compile check it. Fixed v3
patch attached.

Another omission I just realised of is that it doesn't complete the list
of table storage options after after "WITH (". That should be fairly
easy to add (we already have the list for completing after ALTER TABLE
<name> SET|RESET), but it's getting late here now.

Also, when there are multiple things that can all appear in any order in
a given place in the grammar, it completes them all there, but it
doesn't complete the rest after you've typed one one. E.g. it won't
complete WITH or any of the other options after CREATE TABLE <name> (
... ) TABLESPACE <name>. This seems like a more involved change, and
probably a more widespread problem, so I'm tempted to leave that for
another patch.

- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen

On Wed, Dec 19, 2018 at 11:22:29PM +0000, Dagfinn Ilmari Mannsåker wrote:
> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>> + /* Complete ON COMMIT actions for temp tables */
>> + else if (TailMatches("ON", "COMMIT"))
>> + COMPLETE_WITH("PRESERVE ROWS", "DELETE ROWS", "DROP");
>>
>> This causes ON COMMIT to show up for permanent and unlogged tables.
>
> No, this is for completing _after_ ON COMMIT, which is only suggested by
> the clause for temporary tables above.

If a user types "CREATE TABLE foo () ON COMMIT" by himself and then asks
for tab completion then he would get the suggestion, which is incorrect?
--
Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:

> On Wed, Dec 19, 2018 at 11:22:29PM +0000, Dagfinn Ilmari Mannsåker wrote:
>> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>>> + /* Complete ON COMMIT actions for temp tables */
>>> + else if (TailMatches("ON", "COMMIT"))
>>> + COMPLETE_WITH("PRESERVE ROWS", "DELETE ROWS", "DROP");
>>>
>>> This causes ON COMMIT to show up for permanent and unlogged tables.
>>
>> No, this is for completing _after_ ON COMMIT, which is only suggested by
>> the clause for temporary tables above.
>
> If a user types "CREATE TABLE foo () ON COMMIT" by himself and then asks
> for tab completion then he would get the suggestion, which is incorrect?

Point, fixed in the attached v4. OTOH, as I mentioned in my other
email, that runs into the problem that it won't complete the actions
after e.g. "CREATE TEMP TABLE FOO () WITH () ON COMMIT".

--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl

On Thu, Dec 20, 2018 at 12:02:52AM +0000, Dagfinn Ilmari Mannsåker wrote:
> Point, fixed in the attached v4. OTOH, as I mentioned in my other
> email, that runs into the problem that it won't complete the actions
> after e.g. "CREATE TEMP TABLE FOO () WITH () ON COMMIT".

I am fine to do that later on if that's adapted, one complication being
that the completion is dependent on the order of the clauses for CREATE
TABLE as we need something compatible with CREATE TABLE commands bundled
with CREATE SCHEMA calls so only tail checks can be done.

So committed your last version after some comment tweaks and reordering
the entries in alphabetical order.
--
Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:

> On Thu, Dec 20, 2018 at 12:02:52AM +0000, Dagfinn Ilmari Mannsåker wrote:
>> Point, fixed in the attached v4. OTOH, as I mentioned in my other
>> email, that runs into the problem that it won't complete the actions
>> after e.g. "CREATE TEMP TABLE FOO () WITH () ON COMMIT".
>
> I am fine to do that later on if that's adapted, one complication being
> that the completion is dependent on the order of the clauses for CREATE
> TABLE as we need something compatible with CREATE TABLE commands bundled
> with CREATE SCHEMA calls so only tail checks can be done.

Yeah, because of that we can't do the obvious HeadMatches("CREATE",
"TABLE") && (TailMatches(...) || TailMatches(...) || ...). I believe
this would require extending the match syntax with regex-like grouping,
alternation and repetition operators, which I'm not volunteering to do.

> So committed your last version after some comment tweaks and reordering
> the entries in alphabetical order.

Thanks!

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

I wrote:
> Another omission I just realised of is that it doesn't complete the list
> of table storage options after after "WITH (". That should be fairly
> easy to add (we already have the list for completing after ALTER TABLE
> <name> SET|RESET), but it's getting late here now.

Here's a patch that does this (and in passing alphabetises the list of
options).

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl

On Fri, Dec 21, 2018 at 01:57:40PM +0000, Dagfinn Ilmari Mannsåker wrote:
> Yeah, because of that we can't do the obvious HeadMatches("CREATE",
> "TABLE") && (TailMatches(...) || TailMatches(...) || ...). I believe
> this would require extending the match syntax with regex-like grouping,
> alternation and repetition operators, which I'm not volunteering to
> do.

That seems to be a lot of work for little benefit as few queries are
able to work within CREATE SCHEMA, so I would not take this road.
--
Michael

On Fri, Dec 21, 2018 at 03:14:36PM +0000, Dagfinn Ilmari Mannsåker wrote:
> Here's a patch that does this (and in passing alphabetises the list of
> options).

Cool, thanks. The position of the option list is fine. However
list_TABLEOPTIONS is not a name consistent with the surroundings. So
we could just use table_level_option? Reordering them is a good idea,
log_autovacuum_min_duration being the bad entry.
--
Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:

> On Fri, Dec 21, 2018 at 03:14:36PM +0000, Dagfinn Ilmari Mannsåker wrote:
>> Here's a patch that does this (and in passing alphabetises the list of
>> options).
>
> Cool, thanks. The position of the option list is fine. However
> list_TABLEOPTIONS is not a name consistent with the surroundings. So
> we could just use table_level_option?

The CREATE and ALTER TABLE documentation calls them storage parameters,
so I've gone for table_storage_parameters in the attached v2 patch.

> Reordering them is a good idea, log_autovacuum_min_duration being the
> bad entry.

toast_tuple_target was also on the wrong side of the toast.* options.

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl

On Sat, Dec 22, 2018 at 01:33:23PM +0000, Dagfinn Ilmari Mannsåker wrote:
> The CREATE and ALTER TABLE documentation calls them storage parameters,
> so I've gone for table_storage_parameters in the attached v2 patch.

Sold. And committed.

>> Reordering them is a good idea, log_autovacuum_min_duration being the
>> bad entry.
>
> toast_tuple_target was also on the wrong side of the toast.*
> options.

Indeed.
--
Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:

> On Sat, Dec 22, 2018 at 01:33:23PM +0000, Dagfinn Ilmari Mannsåker wrote:
>> The CREATE and ALTER TABLE documentation calls them storage parameters,
>> so I've gone for table_storage_parameters in the attached v2 patch.
>
> Sold. And committed.

Thanks again!

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law