Re: Improved tab completion for FDW DDL

Hi,

Here is a patch which adds the below missing tab completions for the FDW
DDL commands. I noticed these were missing while playing around with
FDWs a while ago.

"ALTER SERVER <name>" with "RENAME TO"
"CREATE SERVER <name> FOREIGN DATA WRAPPER" with fdw name
"CREATE|ALTER USER MAPPING FOR <name> SERVER <name>" with "OPTIONS ("

Another completion which is currently missing but I am not sure if we
should add or not is completing "ALTER|CREATE|DROP USER" with "MAPPING
FOR", but since it might interfere with completing to username for
"ALTER|DROP USER" I am not sure we want it. What do you think?

Andreas

On Wed, Dec 30, 2015 at 01:21:06PM +0100, Andreas Karlsson wrote:
> Hi,
>
> Here is a patch which adds the below missing tab completions for the FDW DDL
> commands. I noticed these were missing while playing around with FDWs a
> while ago.
>
> "ALTER SERVER <name>" with "RENAME TO"
> "CREATE SERVER <name> FOREIGN DATA WRAPPER" with fdw name
> "CREATE|ALTER USER MAPPING FOR <name> SERVER <name>" with "OPTIONS ("
>
> Another completion which is currently missing but I am not sure if we should
> add or not is completing "ALTER|CREATE|DROP USER" with "MAPPING FOR", but
> since it might interfere with completing to username for "ALTER|DROP USER" I
> am not sure we want it. What do you think?

Is there a way to require some reasonable chunk--say, one that's
disambiguated from the name any known ROLE with LOGIN--of MAPPING
before completing with MAPPING FOR? I confess to not knowing how the
new system works in enough detail to know that off the top of my head.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Dec 30, 2015 at 9:21 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Hi,
>
> Here is a patch which adds the below missing tab completions for the FDW DDL
> commands. I noticed these were missing while playing around with FDWs a
> while ago.
>
> "ALTER SERVER <name>" with "RENAME TO"
> "CREATE SERVER <name> FOREIGN DATA WRAPPER" with fdw name
> "CREATE|ALTER USER MAPPING FOR <name> SERVER <name>" with "OPTIONS ("
>
> Another completion which is currently missing but I am not sure if we should
> add or not is completing "ALTER|CREATE|DROP USER" with "MAPPING FOR", but
> since it might interfere with completing to username for "ALTER|DROP USER" I
> am not sure we want it. What do you think?

You may want to use Matches() instead of TailMatches() for performance reasons.
--
Michael

On 01/04/2016 07:26 AM, Michael Paquier wrote:
> You may want to use Matches() instead of TailMatches() for performance reasons.

Probably, but if so should we not also change the surrounding rules to
also use Matches()? I imitated the surrounding code to keep code
consistency and avoid errors.

I thought I saw some patch from you changing many of the rules from
TailMatches() to Matches(). Perhaps I should just update my patch after
your patch has been applied.

Andreas

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> I thought I saw some patch from you changing many of the rules from
> TailMatches() to Matches(). Perhaps I should just update my patch after
> your patch has been applied.

Yes, I think it's important to get that set of patches from Michael
into the tree ASAP so that they can be precedent for the other tab
completion patches that are floating around. I am under the gun for
9.5.0 release today but will review and hopefully push Michael's
changes soon.

regards, tom lane

On 01/04/2016 07:26 AM, Michael Paquier wrote:
> You may want to use Matches() instead of TailMatches() for performance reasons.

Here is an updated version which uses Matches().

Andreas

On 01/04/2016 01:09 AM, David Fetter wrote:
> On Wed, Dec 30, 2015 at 01:21:06PM +0100, Andreas Karlsson wrote:
>> Another completion which is currently missing but I am not sure if we should
>> add or not is completing "ALTER|CREATE|DROP USER" with "MAPPING FOR", but
>> since it might interfere with completing to username for "ALTER|DROP USER" I
>> am not sure we want it. What do you think?
>
> Is there a way to require some reasonable chunk--say, one that's
> disambiguated from the name any known ROLE with LOGIN--of MAPPING
> before completing with MAPPING FOR? I confess to not knowing how the
> new system works in enough detail to know that off the top of my head.

No, and while it would not be too hard to build it would not be worth
doing just for this use case.

Andreas

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> On 01/04/2016 01:09 AM, David Fetter wrote:
>> On Wed, Dec 30, 2015 at 01:21:06PM +0100, Andreas Karlsson wrote:
>>> Another completion which is currently missing but I am not sure if we should
>>> add or not is completing "ALTER|CREATE|DROP USER" with "MAPPING FOR", but
>>> since it might interfere with completing to username for "ALTER|DROP USER" I
>>> am not sure we want it. What do you think?

>> Is there a way to require some reasonable chunk--say, one that's
>> disambiguated from the name any known ROLE with LOGIN--of MAPPING
>> before completing with MAPPING FOR? I confess to not knowing how the
>> new system works in enough detail to know that off the top of my head.

> No, and while it would not be too hard to build it would not be worth
> doing just for this use case.

The way we've solved other similar cases would translate like this:
instead of the "query for user names" just returning user names, add
on "UNION 'MAPPING FOR'". So if you do

# alter user <TAB>

where you're now offered

alice joe postgres

you'd instead get

alice joe postgres MAPPING FOR

Dunno if it's worth the trouble though.

regards, tom lane

The second part is not necessary, because there is already code that
completes FDW names after "FOREIGN DATA WRAPPER". So this already works.

The other two parts are valid improvements, but they should be done
consistently across commands. So please

- Also complete RENAME TO in ALTER FOREIGN DATA WRAPPER.

- Also complete OPTIONS in FOREIGN DATA WRAPPER and SERVER commands.

On 01/11/2016 02:39 AM, Peter Eisentraut wrote:
> The second part is not necessary, because there is already code that
> completes FDW names after "FOREIGN DATA WRAPPER". So this already works.

Good spot, thanks. I have no idea why I did not think it worked. Maybe
it just did not work in 9.2 and I failed when trying to reproduce it on
master.

> - Also complete RENAME TO in ALTER FOREIGN DATA WRAPPER.

Done.

> - Also complete OPTIONS in FOREIGN DATA WRAPPER and SERVER commands.

Done.

Andreas

On 1/18/16 8:36 PM, Andreas Karlsson wrote:
> On 01/11/2016 02:39 AM, Peter Eisentraut wrote:
>> The second part is not necessary, because there is already code that
>> completes FDW names after "FOREIGN DATA WRAPPER". So this already works.
>
> Good spot, thanks. I have no idea why I did not think it worked. Maybe
> it just did not work in 9.2 and I failed when trying to reproduce it on
> master.
>
>> - Also complete RENAME TO in ALTER FOREIGN DATA WRAPPER.
>
> Done.
>
>> - Also complete OPTIONS in FOREIGN DATA WRAPPER and SERVER commands.
>
> Done.

committed

On 01/23/2016 01:03 PM, Peter Eisentraut wrote:
> committed

Thanks!

Andreas