RE: Fix some newly modified tab-complete changes

On Wed, Sep 28, 2022 1:49 PM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith
> <smithpb2250(at)gmail(dot)com> wrote in
> > On Tue, Sep 27, 2022 at 8:28 PM shiy(dot)fnst(at)fujitsu(dot)com
> > <shiy(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > Hi hackers,
> > >
> > > I saw a problem when using tab-complete for "GRANT", "TABLES IN
> SCHEMA" should
> > > be "ALL TABLES IN SCHEMA" in the following case.
> > >
> > > postgres=# grant all on
> > > ALL FUNCTIONS IN SCHEMA DATABASE FUNCTION
> PARAMETER SCHEMA TABLESPACE
> > > ALL PROCEDURES IN SCHEMA DOMAIN information_schema.
> PROCEDURE SEQUENCE tbl
> > > ALL ROUTINES IN SCHEMA FOREIGN DATA WRAPPER LANGUAGE
> public. TABLE TYPE
> > > ALL SEQUENCES IN SCHEMA FOREIGN SERVER LARGE OBJECT
> ROUTINE TABLES IN SCHEMA
> > >
> > > I found that it is related to the recent commit 790bf615dd, and maybe it's
> > > better to fix it. I also noticed that some comments should be modified
> according
> > > to this new syntax. Attach a patch to fix them.
> > >
> >
> > Thanks for the patch! Below are my review comments.
> >
> > The patch looks good to me but I did find some other tab-completion
> > anomalies. IIUC these are unrelated to your work, but since I found
> > them while testing your patch I am reporting them here.
>
> Looks fine to me, too.
>

Thanks for reviewing it.

> > Perhaps you want to fix them in the same patch, or just raise them
> > again separately?
> >
> > ======
> >
> > 1. tab complete for CREATE PUBLICATION
> >
> > I donʼt think this is any new bug, but I found that it is possible to do this...
> >
> > test_pub=# create publication p for ALL TABLES IN SCHEMA <tab>
> > information_schema pg_catalog pg_toast public
> >
> > or, even this...
> >
> > test_pub=# create publication p for XXX TABLES IN SCHEMA <tab>
> > information_schema pg_catalog pg_toast public
>
> Completion is responding to "IN SCHEMA" in these cases. However, I
> don't reach this state only by completion becuase it doesn't suggest
> "IN SCHEMA" after "TABLES" nor "ALL TABLES". I don't see a reason to
> change that behavior unless that fix doesn't cause any additional
> complexity.
>

+1

> > ======
> >
> > 2. tab complete for GRANT
> >
> > test_pub=# grant <tab>
> > ALL EXECUTE
> > pg_execute_server_program pg_read_server_files postgres
> > TRIGGER
> > ALTER SYSTEM GRANT pg_monitor
> > pg_signal_backend REFERENCES
> > TRUNCATE
> > CONNECT INSERT pg_read_all_data
> > pg_stat_scan_tables SELECT UPDATE
> > CREATE pg_checkpoint
> > pg_read_all_settings pg_write_all_data SET
> > USAGE
> > DELETE pg_database_owner
> > pg_read_all_stats pg_write_server_files TEMPORARY
> >
> > 2a.
> > grant "GRANT" ??
>
> Yeah, for the mement I thought that might a kind of admin option but
> there's no such a privilege. REVOKE gets the same suggestion.
>

Maybe that's for "REVOKE GRANT OPTION FOR". But it is used by both GRANT and
REVOKE. I think it's a separate problem, I have tried to fix it in the attached
0002 patch.

> > 2b.
> > grant "TEMPORARY" but not "TEMP" ??
>
> TEMP is an alternative spelling so that's fine.
>

Agreed.

>
> I found the following suggestion.
>
> CREATE PUBLICATION p FOR TABLES <tab> -> ["IN SCHEMA", "WITH ("]
>
> I believe "WITH (" doesn't come there.
>

Fixed.

Attach the updated patch.

Regards,
Shi yu