Horton

Thread

Commits

  1. psql: Tab completion for CREATE MATERIALIZED VIEW ... USING

  2. psql: Tab completion for CREATE TEMP TABLE ... USING

  3. psql: Tab completion for ALTER TYPE ... CASCADE/RESTRICT

  4. psql: Tab completion for ALTER TYPE ... ADD ATTRIBUTE

  5. Add more tab completion for CREATE TABLE in psql

  1. Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-01T10:43:55Z

    Hi hackers!

I have been working on cloudberry/greenplum tab completion features,
and I spotted that the postgres version of psql does not tab-complete
ALTER TABLE ADD COLUMN IF NOT EXISTS pattern.
So, I propose to support this.

-- 
Best regards,
Kirill Reshke

  2. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-01T18:06:35Z

    On Fri, 1 Nov 2024 at 15:43, Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> Hi hackers!
>
> I have been working on cloudberry/greenplum tab completion features,
> and I spotted that the postgres version of psql does not tab-complete
> ALTER TABLE ADD COLUMN IF NOT EXISTS pattern.
> So, I propose to support this.
>
> --
> Best regards,
> Kirill Reshke

Hi!
I found another pattern which has no tab competition.

-- 
Best regards,
Kirill Reshke

  3. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-04T11:50:50Z

    On Fri, 1 Nov 2024 at 15:43, Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> Hi hackers!
>
> I have been working on cloudberry/greenplum tab completion features,
> and I spotted that the postgres version of psql does not tab-complete
> ALTER TABLE ADD COLUMN IF NOT EXISTS pattern.
> So, I propose to support this.
>
> --
> Best regards,
> Kirill Reshke
Hi, I spotted another missing option:

CREATE TEMP TABLE <name> (...) does not complete with USING.
I suspect the reason is that commit 4cba9c2, which added this tab
completion, was before TAM support.

So, sending this enhancement (v3-0003) along with two other patches.


-- 
Best regards,
Kirill Reshke

  4. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Karina Litskevich <litskevichkarina@gmail.com> — 2024-11-06T15:09:25Z

    Hi Kirill,

I looked through your patches. First of all, please note that TAB
completion doesn't have to complete all valid grammatical
constructions. See the comment on the top of tab-complete.in.c:

 * This file implements a somewhat more sophisticated readline "TAB
 * completion" in psql. It is not intended to be AI, to replace
 * learning SQL, or to relieve you from thinking about what you're
 * doing. Also it does not always give you all the syntactically legal
 * completions, only those that are the most common or the ones that
 * the programmer felt most like implementing.

Considering this, I don't think 0001 should be accepted as is. Neither
"IF NOT EXIST" nor "IF EXISTS" appears anywhere in tab-complete.in.c.
Adding it for the only "ALTER TABLE ... ADD COLUMN" case doesn't seem
right. Adding it everywhere looks like lots of work of questionable
utility.

0003 looks good to me. Considering that we already have a completion of
"CREATE TEMP TABLE ... USING" with table access methods, completion of
"CREATE TABLE ..." with USING seems reasonable. It’s strange we didn’t
have this before. Good catch!

I can't judge if 0002 adds useful completion cases, but I noticed that
you add completion of "DROP/ADD ATTRIBUTE ..." with CASCADE/RESRTICT
while ignoring "ALTER/RENAME ATTRIBUTE ..." that also can be followed
by CASCADE/RESRTICT according to the documentation. Is it intentional?

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/

  5. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-07T11:42:30Z

    Hi, thank you for your feedback.

On Wed, 6 Nov 2024 at 20:09, Karina Litskevich
<litskevichkarina@gmail.com> wrote:
>
> Hi Kirill,
>
> I looked through your patches. First of all, please note that TAB
> completion doesn't have to complete all valid grammatical
> constructions. See the comment on the top of tab-complete.in.c:
>
>  * This file implements a somewhat more sophisticated readline "TAB
>  * completion" in psql. It is not intended to be AI, to replace
>  * learning SQL, or to relieve you from thinking about what you're
>  * doing. Also it does not always give you all the syntactically legal
>  * completions, only those that are the most common or the ones that
>  * the programmer felt most like implementing.
>
> Considering this, I don't think 0001 should be accepted as is. Neither
> "IF NOT EXIST" nor "IF EXISTS" appears anywhere in tab-complete.in.c.
> Adding it for the only "ALTER TABLE ... ADD COLUMN" case doesn't seem
> right. Adding it everywhere looks like lots of work of questionable
> utility.

Ok. I dropped this patch from v4.

> 0003 looks good to me. Considering that we already have a completion of
> "CREATE TEMP TABLE ... USING" with table access methods, completion of
> "CREATE TABLE ..." with USING seems reasonable. It’s strange we didn’t
> have this before. Good catch!

Cool.


> I can't judge if 0002 adds useful completion cases, but I noticed that
> you add completion of "DROP/ADD ATTRIBUTE ..." with CASCADE/RESRTICT
> while ignoring "ALTER/RENAME ATTRIBUTE ..." that also can be followed
> by CASCADE/RESRTICT according to the documentation. Is it intentional?


v3-0002 patch actually mixes two types of completion. First one, which
adds a data type completion for ADD ATTRIBUTE, is pretty useful.
similar to existing tab completion for `ALTER TABLE xxx ADD [IF NOT
EXISTS] [COLUMN] yyy `.
The second one (CASCADE/RESTRICT) is dubious, and I'm also not sure if
we really need it, so perhaps we should wait for some other views..
For this reason, I split v3-0002 into two separate patches.


I also want to propose an Access method completion for create M.V.
<foo> using. This is a patch I forgot to attach in my last email.
Please, see v4-0004

-- 
Best regards,
Kirill Reshke

  6. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Karina Litskevich <litskevichkarina@gmail.com> — 2024-11-07T17:33:47Z

    I looked through the new set of patches.

On Thu, Nov 7, 2024 at 2:42 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> v3-0002 patch actually mixes two types of completion. First one, which
> adds a data type completion for ADD ATTRIBUTE, is pretty useful.
> similar to existing tab completion for `ALTER TABLE xxx ADD [IF NOT
> EXISTS] [COLUMN] yyy `.

As I understand, it's v4-0001. Looks fine, I agree that it's a useful
completion and I can't see any arguments against that.

> The second one (CASCADE/RESTRICT) is dubious, and I'm also not sure if
> we really need it, so perhaps we should wait for some other views..
> For this reason, I split v3-0002 into two separate patches.

And this is v4-0002. Agreed that we should wait for another opinion to
see if it's needed. Anyway, I have a few remarks here:
1) "ALTER TYPE xxx RENAME VALUE yyy TO zzz" can't be followed by
CASCADE/RESTRICT at least according to the docs [1].
2) I still can't see why you don't complete "ALTER TYPE ... ALTER
ATTRIBUTE ... TYPE ..." with CASCADE/RESTRICT.

By the way, I suggest adding a completion of "ALTER TYPE ... ALTER
ATTRIBUTE ... TYPE" with the list of types while we are here. It should
probably go together with v4-0001.

Next, v4-0003 is the same as v3-0003, and we already agreed on this.
(Just a note for myself.)

> I also want to propose an Access method completion for create M.V.
> <foo> using. This is a patch I forgot to attach in my last email.
> Please, see v4-0004

In v4-0004 I like the idea to add this completion, but I have some
remarks to implementation:
1) Try to keep comments identical, at least in one piece of code. Right
now you have "CREATE MATERIALIZED VIEW <name>" and "CREATE MATERIALIZED
VIEW <sth>" within three consecutive lines. I can see there was the
same problem before your changes, so it's not exactly your fault. Let's
correct it, though.
2) The comment
    /* Complete CREATE MATERIALIZED VIEW <name> with AS */
is no longer correct since you've changed
-       COMPLETE_WITH("AS");
to
+       COMPLETE_WITH("AS", "USING");
Please fix it.

I hope to look more thoroughly into tab-complete.in.c tomorrow or on
Monday to see if there are any other problems I can't see at first
glance. I'll send another mail when I get to do this.


[1] https://www.postgresql.org/docs/current/sql-altertype.html


Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/

  7. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Karina Litskevich <litskevichkarina@gmail.com> — 2024-11-08T08:14:13Z

    I did a quick testing.

As expected, both "ALTER TYPE xxx RENAME VALUE yyy TO zzz CASCADE" and
"ALTER TYPE xxx RENAME VALUE yyy TO zzz RESTRICT" in v4-0002 cause
syntax errors.

In v4-0004 you might also want to complete
"CREATE MATERIALIZED VIEW... USING ..." with "AS".

Other completions work fine.

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/

  8. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-10T12:43:21Z

    On Thu, 7 Nov 2024 at 22:33, Karina Litskevich
<litskevichkarina@gmail.com> wrote:
>
> I looked through the new set of patches.
>
> On Thu, Nov 7, 2024 at 2:42 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> > v3-0002 patch actually mixes two types of completion. First one, which
> > adds a data type completion for ADD ATTRIBUTE, is pretty useful.
> > similar to existing tab completion for `ALTER TABLE xxx ADD [IF NOT
> > EXISTS] [COLUMN] yyy `.
>
> As I understand, it's v4-0001. Looks fine, I agree that it's a useful
> completion and I can't see any arguments against that.
>
> > The second one (CASCADE/RESTRICT) is dubious, and I'm also not sure if
> > we really need it, so perhaps we should wait for some other views..
> > For this reason, I split v3-0002 into two separate patches.
>
> And this is v4-0002. Agreed that we should wait for another opinion to
> see if it's needed. Anyway, I have a few remarks here:
> 1) "ALTER TYPE xxx RENAME VALUE yyy TO zzz" can't be followed by
> CASCADE/RESTRICT at least according to the docs [1].

Yes, my bad. I removed "VALUE".

> 2) I still can't see why you don't complete "ALTER TYPE ... ALTER
> ATTRIBUTE ... TYPE ..." with CASCADE/RESTRICT.

My bad, fixed this in v5. Thank you.

> By the way, I suggest adding a completion of "ALTER TYPE ... ALTER
> ATTRIBUTE ... TYPE" with the list of types while we are here. It should
> probably go together with v4-0001.

Surprisingly this is already supported in an interesting manner.  psql
will tab-complete everything that end with token "TYPE" with list of
datatypes:

```
reshke=# FOO BAR BAZ
<tab> *nothing*
reshke=# FOO BAR BAZ TYPE
<tab>
Display all 119 possibilities? (y or n)
```

> Next, v4-0003 is the same as v3-0003, and we already agreed on this.
> (Just a note for myself.)
>
> > I also want to propose an Access method completion for create M.V.
> > <foo> using. This is a patch I forgot to attach in my last email.
> > Please, see v4-0004
>
> In v4-0004 I like the idea to add this completion, but I have some
> remarks to implementation:
> 1) Try to keep comments identical, at least in one piece of code. Right
> now you have "CREATE MATERIALIZED VIEW <name>" and "CREATE MATERIALIZED
> VIEW <sth>" within three consecutive lines. I can see there was the
> same problem before your changes, so it's not exactly your fault. Let's
> correct it, though.

Ok, sure. I did the correction.

> 2) The comment
>     /* Complete CREATE MATERIALIZED VIEW <name> with AS */
> is no longer correct since you've changed
> -       COMPLETE_WITH("AS");
> to
> +       COMPLETE_WITH("AS", "USING");
> Please fix it.


Nice catch! Fixed.


> I hope to look more thoroughly into tab-complete.in.c tomorrow or on
> Monday to see if there are any other problems I can't see at first
> glance. I'll send another mail when I get to do this.

Your effort is much appreciated.

>
> [1] https://www.postgresql.org/docs/current/sql-altertype.html
>
>
> Best regards,
> Karina Litskevich
> Postgres Professional: http://postgrespro.com/


PFA v5.

-- 
Best regards,
Kirill Reshke

  9. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Karina Litskevich <litskevichkarina@gmail.com> — 2024-11-11T11:48:25Z

    On Sun, Nov 10, 2024 at 3:43 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> > By the way, I suggest adding a completion of "ALTER TYPE ... ALTER
> > ATTRIBUTE ... TYPE" with the list of types while we are here. It should
> > probably go together with v4-0001.
>
> Surprisingly this is already supported in an interesting manner.  psql
> will tab-complete everything that end with token "TYPE" with list of
> datatypes:
>
> ```
> reshke=# FOO BAR BAZ
> <tab> *nothing*
> reshke=# FOO BAR BAZ TYPE
> <tab>
> Display all 119 possibilities? (y or n)
> ```

Agreed.

I wrote:
> I hope to look more thoroughly into tab-complete.in.c tomorrow or on
> Monday to see if there are any other problems I can't see at first
> glance. I'll send another mail when I get to do this.

As promised, I looked into the new set of patches more closely. Can't
see any other significant problems. However, you still need to do a
couple of cosmetic changes.

On Sun, Nov 10, 2024 at 3:43 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> > 1) Try to keep comments identical, at least in one piece of code. Right
> > now you have "CREATE MATERIALIZED VIEW <name>" and "CREATE MATERIALIZED
> > VIEW <sth>" within three consecutive lines. I can see there was the
> > same problem before your changes, so it's not exactly your fault. Let's
> > correct it, though.
>
> Ok, sure. I did the correction.

You now have the same problem with "USING <access method>" and
"USING <am_name>" in v5-0004.

Also, make sure you ran pgindent before creating patches. In v5-0004
there are comments that are too long for one line, and there is a line
with a trailing space:

+ else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "AS") ||

Other than that, everything looks fine to me.

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/

  10. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Tomas Vondra <tomas@vondra.me> — 2024-12-07T22:35:25Z

    Hi,

On 11/11/24 12:48, Karina Litskevich wrote:
> ...
>
> I wrote:
>> I hope to look more thoroughly into tab-complete.in.c tomorrow or on
>> Monday to see if there are any other problems I can't see at first
>> glance. I'll send another mail when I get to do this.
> 
> As promised, I looked into the new set of patches more closely. Can't
> see any other significant problems. However, you still need to do a
> couple of cosmetic changes.
> 
> On Sun, Nov 10, 2024 at 3:43 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
>>> 1) Try to keep comments identical, at least in one piece of code. Right
>>> now you have "CREATE MATERIALIZED VIEW <name>" and "CREATE MATERIALIZED
>>> VIEW <sth>" within three consecutive lines. I can see there was the
>>> same problem before your changes, so it's not exactly your fault. Let's
>>> correct it, though.
>>
>> Ok, sure. I did the correction.
> 
> You now have the same problem with "USING <access method>" and
> "USING <am_name>" in v5-0004.
> 
> Also, make sure you ran pgindent before creating patches. In v5-0004
> there are comments that are too long for one line, and there is a line
> with a trailing space:
> 
> + else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "AS") ||
> 
> Other than that, everything looks fine to me.
> 

I took a quick look at this patch series, and it looks generally fine to
me. Barring objections, I'll get it committed. Yes, there's a couple
cosmetic details, and it needs a pgindent run, but I think I can take
care of that ...


regards

-- 
Tomas Vondra

  11. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-08T04:22:53Z

    On Sun, 8 Dec 2024 at 03:35, Tomas Vondra <tomas@vondra.me> wrote:
>
> Hi,
> I took a quick look at this patch series, and it looks generally fine to
> me. Barring objections, I'll get it committed. Yes, there's a couple
> cosmetic details, and it needs a pgindent run, but I think I can take
> care of that ...
>
Thank you for taking a look.

PFA v6 with last Karina's review comments addressed.

-- 
Best regards,
Kirill Reshke

  12. Re: Add missing tab completion for ALTER TABLE ADD COLUMN IF NOT EXISTS

    Tomas Vondra <tomas@vondra.me> — 2024-12-16T16:40:59Z

    On 12/8/24 05:22, Kirill Reshke wrote:
> On Sun, 8 Dec 2024 at 03:35, Tomas Vondra <tomas@vondra.me> wrote:
>>
>> Hi,
>> I took a quick look at this patch series, and it looks generally fine to
>> me. Barring objections, I'll get it committed. Yes, there's a couple
>> cosmetic details, and it needs a pgindent run, but I think I can take
>> care of that ...
>>
> Thank you for taking a look.
> 
> PFA v6 with last Karina's review comments addressed.
> 

Thanks. Pushed.

I went through the patches, and pushed them with some pretty minor
tweaks. Most of the work was about writing the commit messages. I
considered squashing the commits, but decided not to do that - each of
the 4 patches does tab completion for different command.


regards

-- 
Tomas Vondra