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

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/

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/

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/

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/

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

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