Horton

Thread

Commits

  1. Add psql completion for [ NO ] DEPENDS ON EXTENSION

  2. doc: Mention NO DEPENDS ON EXTENSION in its supported ALTER commands

  1. [DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses

    Ian Lawrence Barwick <barwick@gmail.com> — 2021-02-12T01:32:14Z

    Greetings

We have following syntax:

    ALTER THING name [ NO ] DEPENDS ON EXTENSION name

for the following THINGs:

- ALTER TRIGGER
- ALTER FUNCTION
- ALTER PROCEDURE
- ALTER ROUTINE
- ALTER MATERIALIZED VIEW
- ALTER INDEX

In the documentation, the "[ NO ]" option is listed in the synopsis for
ALTER TRIGGER and ALTER FUNCTION, but not the others.
Trivial patch attached.

Will add to next CF.

Regards

Ian Barwick


-- 
EnterpriseDB: https://www.enterprisedb.com

  2. Re: [DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses

    Michael Paquier <michael@paquier.xyz> — 2021-02-13T02:52:51Z

    On Fri, Feb 12, 2021 at 10:32:14AM +0900, Ian Lawrence Barwick wrote:
> In the documentation, the "[ NO ]" option is listed in the synopsis for
> ALTER TRIGGER and ALTER FUNCTION, but not the others.
> Trivial patch attached.

There are two flavors to cover for 6 commands per gram.y, and you are
covering all of them.  So this looks good to me.  I'll apply and
backpatch in a bit.  It is worth noting that tab-complete.c does a bad
job in completing those clauses.
--
Michael

  3. Re: [DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses

    Ian Lawrence Barwick <barwick@gmail.com> — 2021-02-15T06:52:31Z

    2021年2月13日(土) 11:52 Michael Paquier <michael@paquier.xyz>:

> On Fri, Feb 12, 2021 at 10:32:14AM +0900, Ian Lawrence Barwick wrote:
> > In the documentation, the "[ NO ]" option is listed in the synopsis for
> > ALTER TRIGGER and ALTER FUNCTION, but not the others.
> > Trivial patch attached.
>
> There are two flavors to cover for 6 commands per gram.y, and you are
> covering all of them.  So this looks good to me.  I'll apply and
> backpatch in a bit.  It is worth noting that tab-complete.c does a bad
> job in completing those clauses.
> --
> Michael
>


-- 
EnterpriseDB: https://www.enterprisedb.com

  4. Re: [DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses

    Ian Lawrence Barwick <barwick@gmail.com> — 2021-02-15T06:57:04Z

    2021年2月13日(土) 11:52 Michael Paquier <michael@paquier.xyz>:

> On Fri, Feb 12, 2021 at 10:32:14AM +0900, Ian Lawrence Barwick wrote:
> > In the documentation, the "[ NO ]" option is listed in the synopsis for
> > ALTER TRIGGER and ALTER FUNCTION, but not the others.
> > Trivial patch attached.
>
> There are two flavors to cover for 6 commands per gram.y, and you are
> covering all of them.  So this looks good to me.  I'll apply and
> backpatch in a bit.


Thanks! (Apologies for the preceding blank mail).

It is worth noting that tab-complete.c does a bad
> job in completing those clauses.
>

Indeed it does. Not the most exciting of use cases, though I imagine it
might come in handy for anyone developing an extension, and the
existing implementation is inconsistent (in place for ALTER INDEX,
and partially for ALTER MATERIALIZED VIEW, but not the others).
Patch suggestion attached.

Regards

Ian Barwick


-- 
EnterpriseDB: https://www.enterprisedb.com

  5. Re: [DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses

    Michael Paquier <michael@paquier.xyz> — 2021-02-16T01:20:45Z

    On Mon, Feb 15, 2021 at 03:57:04PM +0900, Ian Lawrence Barwick wrote:
> Indeed it does. Not the most exciting of use cases, though I imagine it
> might come in handy for anyone developing an extension, and the
> existing implementation is inconsistent (in place for ALTER INDEX,
> and partially for ALTER MATERIALIZED VIEW, but not the others).
> Patch suggestion attached.

Thanks.

-   else if (Matches("ALTER", "INDEX", MatchAny, "NO", "DEPENDS"))
-       COMPLETE_WITH("ON EXTENSION");
-   else if (Matches("ALTER", "INDEX", MatchAny, "DEPENDS"))
-       COMPLETE_WITH("ON EXTENSION");
The part, if removed, means that typing "alter index my_index no " is
not able to complete with "DEPENDS ON EXTENSION" anymore.  So it seems
to me that ALTER INDEX got that right, and that the other commands had
better do the same.
--
Michael

  6. Re: [DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses

    Ian Lawrence Barwick <barwick@gmail.com> — 2021-02-16T02:18:47Z

    2021年2月16日(火) 10:20 Michael Paquier <michael@paquier.xyz>:

> On Mon, Feb 15, 2021 at 03:57:04PM +0900, Ian Lawrence Barwick wrote:
> > Indeed it does. Not the most exciting of use cases, though I imagine it
> > might come in handy for anyone developing an extension, and the
> > existing implementation is inconsistent (in place for ALTER INDEX,
> > and partially for ALTER MATERIALIZED VIEW, but not the others).
> > Patch suggestion attached.
>
> Thanks.
>
> -   else if (Matches("ALTER", "INDEX", MatchAny, "NO", "DEPENDS"))
> -       COMPLETE_WITH("ON EXTENSION");
> -   else if (Matches("ALTER", "INDEX", MatchAny, "DEPENDS"))
> -       COMPLETE_WITH("ON EXTENSION");
> The part, if removed, means that typing "alter index my_index no " is
> not able to complete with "DEPENDS ON EXTENSION" anymore.  So it seems
> to me that ALTER INDEX got that right, and that the other commands had
> better do the same.
>

Hmm, with the current implementation "alter index my_index no <TAB>"
doesn't work
anyway; you'd need to add this before the above lines:

+       else if (Matches("ALTER", "INDEX", MatchAny, "NO"))
+               COMPLETE_WITH("DEPENDS");

so AFAICT the patch doesn't change that behaviour. It does mean "alter index
my_index no depends <TAB>" no longer completes to "ON EXTENSION", but if
you've
typed one of "NO" or "DEPENDS" in that context, "ON EXTENSION" is the only
completion so I'm not sure what's gained by forcing the user to hit TAB
twice.

There are quite a few tab completions consisting of more than one word
(e.g. "MATERIALIZED VIEW", "FORCE ROW LEVEL SECURITY") where tab completion
is
ineffective after the first word followed by a space, e.g. "alter
materialized
<TAB>" doesn't result in any expansion either. I suppose we could go
through all
those and handle each word individually, but presumably there's a reason why
that hasn't been done already (maybe no-one has complained?).

Regards

Ian Barwick

-- 
EnterpriseDB: https://www.enterprisedb.com

  7. Re: [DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses

    Michael Paquier <michael@paquier.xyz> — 2021-02-17T03:00:49Z

    On Tue, Feb 16, 2021 at 11:18:47AM +0900, Ian Lawrence Barwick wrote:
> Hmm, with the current implementation "alter index my_index no <TAB>"
> doesn't work
> anyway; you'd need to add this before the above lines:
> 
> +       else if (Matches("ALTER", "INDEX", MatchAny, "NO"))
> +               COMPLETE_WITH("DEPENDS");
> 
> so AFAICT the patch doesn't change that behaviour. It does mean "alter index
> my_index no depends <TAB>" no longer completes to "ON EXTENSION", but if
> you've
> typed one of "NO" or "DEPENDS" in that context, "ON EXTENSION" is the only
> completion so I'm not sure what's gained by forcing the user to hit TAB
> twice.

You are right.  It looks like I have tested without a whitespace after
the "NO".  With a whitespace it does not work, so that looks like a
complication for little gain.  Another problem with the code on HEAD
is that you would not complete properly "NO DEPENDS ON", so that feels
half-completed.

> There are quite a few tab completions consisting of more than one word
> (e.g. "MATERIALIZED VIEW", "FORCE ROW LEVEL SECURITY") where tab completion
> is
> ineffective after the first word followed by a space, e.g. "alter
> materialized
> <TAB>" doesn't result in any expansion either. I suppose we could go
> through all
> those and handle each word individually, but presumably there's a reason why
> that hasn't been done already (maybe no-one has complained?).

Because that's just extra maintenance as most people will just
complete after typing the first set of characters?  This part got
discussed as of 1e324cb:
https://www.postgresql.org/message-id/CALtqXTcogrFEVP9uou5vFtnGsn+vHZUu9+9a0inarfYVOHScYQ@mail.gmail.com

Anyway, after sleeping on it, I have just applied your original patch
as that's simpler, and will cover the cases people would care for.
--
Michael