Re: Improve tab completion for ALTER DEFAULT PRIVILEGE and ALTER TABLE

On Thu, 28 Mar 2024 at 13:05, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Hi,
>
> Thank you for the patch!
>
> On Mon, Jul 3, 2023 at 12:12 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > Improved tab completion for "ALTER DEFAULT PRIVILEGE" and "ALTER TABLE":
> > 1) GRANT, REVOKE and FOR USER keyword was not displayed in tab
> > completion of alter default privileges like the below statement:
> > ALTER DEFAULT PRIVILEGES GRANT INSERT ON tables TO PUBLIC;
> > ALTER DEFAULT PRIVILEGES REVOKE INSERT ON tables FROM PUBLIC;
> > ALTER DEFAULT PRIVILEGES FOR USER vignesh revoke INSERT ON tables FROM dba1;
>
> +1
>
> >
> > 2) USER was not displayed for "ALTER DEFAULT PRIVILEGES IN SCHEMA
> > public FOR " like in below statement:
> > ALTER DEFAULT PRIVILEGES IN SCHEMA public FOR USER dba1 GRANT INSERT
> > ON TABLES TO PUBLIC;
>
> Since there is no difference FOR USER and FOR ROLE, I'm not sure we
> really want to support both in tab-completion.

I have removed this change

> >
> > 3) "FOR GRANT OPTION" was not display for "ALTER DEFAULT PRIVILEGES
> > REVOKE " like in below statement:
> > alter default privileges revoke grant option for select ON tables FROM dba1;
>
> +1. But the v3 patch doesn't cover the following case:
>
> =# alter default privileges for role masahiko revoke [tab]
> ALL         CREATE      DELETE      EXECUTE     INSERT      MAINTAIN
>  REFERENCES  SELECT      TRIGGER     TRUNCATE    UPDATE      USAGE

Modified in the updated patch

> And it doesn't cover MAINTAIN neither:
>
> =# alter default privileges revoke [tab]
> ALL               DELETE            GRANT OPTION FOR  REFERENCES
>  TRIGGER           UPDATE
> CREATE            EXECUTE           INSERT            SELECT
>  TRUNCATE          USAGE

Modified in the updated patch

> The patch adds the completions for ALTER DEFAULT PRIVILEGES REVOKE,
> but we handle such case in GRANT and REVOKE part:
>
> (around L3958)
>         /*
>          * With ALTER DEFAULT PRIVILEGES, restrict completion to grantable
>          * privileges (can't grant roles)
>          */
>         if (HeadMatches("ALTER", "DEFAULT", "PRIVILEGES"))
>             COMPLETE_WITH("SELECT", "INSERT", "UPDATE",
>                           "DELETE", "TRUNCATE", "REFERENCES", "TRIGGER",
>                           "CREATE", "EXECUTE", "USAGE", "MAINTAIN", "ALL");

The current patch handles the fix from here now.

> Also, I think we can support WITH GRANT OPTION too. For example,
>
> =# alter default privileges for role masahiko grant all on tables to
> public [tab]

I have handled this in the updated patch

> It's already supported in the GRANT statement.
>
> >
> > 4) "DATA TYPE" was missing in "ALTER TABLE table-name ALTER COLUMN
> > column-name SET" like in:
> > ALTER TABLE t1 ALTER COLUMN c1 SET DATA TYPE text;
> >
>
> +1. The patch looks good to me, so pushed.

Thanks for committing this.

The updated patch has the changes for the above comments.

Regards,
Vignesh