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/