RE: Fix some newly modified tab-complete changes - Mailing list pgsql-hackers

On Wed, Sep 28, 2022 1:49 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> 
> At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith
> <smithpb2250@gmail.com> wrote in
> > On Tue, Sep 27, 2022 at 8:28 PM shiy.fnst@fujitsu.com
> > <shiy.fnst@fujitsu.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