Re: PATCH: psql tab completion for SELECT

Hi Vik, thanks for the feedback!

On 23 January 2018 at 07:41, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> This looks better.  One minor quibble I have is your use of != (which
> PostgreSQL accepts) instead of <> (the SQL standard).  I'll let the
> committer worry about that.

There are both forms in the existing queries, but if <> is more
standard, we should use that.

>> It also uses the server version to determine which query to run.  I
>> have not written a custom query for every version from 7.1!  I've
>> split up the server history into:
>>
>> pre-7.3 - does not even have pg_function_is_visible.  Not supported.
>> pre-9.0 - does not have several support functions in types, languages,
>> etc.  We don't bother filtering using columns in other tables.
>> pre-9.6 - does not have various aggregate support functions.
>> 9.6 or later - the full query
>
> I'm not sure how overboard we need to go with this going backwards so
> what you did is probably fine.

What I did might be considered overboard, too. :)

>> I was able to test against 9.2, 9.6, and 10 servers, but compiling and
>> testing the older releases was a bit much for a Friday evening.  I'm
>> not sure there's much value in support for old servers, as long as we
>> can make completion queries fail a bit more gracefully.
>
> pg_dump stops at 8.0, we can surely do the same.

Ok.  I'll try to do a bit more testing against servers in that range.

> Now for some other points:
>
> Your use of Matches1 is wrong, you should use TailMatches1 instead.
> SELECT is a fully reserved keyword, so just checking if it's the
> previous token is sufficient.  By using Matches1, you miss cases like
> this: SELECT (SELECT <tab>
>
> It also occurred to me that SELECT isn't actually a complete command (or
> whatever you want to call it), it's an abbreviation for SELECT ALL.  So
> you should check for SELECT, SELECT ALL, and SELECT DISTINCT. (The FROM
> tab completion is missing this trick but that's a different patch)

Right.  TailMatches it is.

(I was thinking we could preprocess the input to parts extraneous to
the current query for tab completion purposes, such as:
  - Input up to and including "(", to tab complete a sub-query.
  - Input inside "()", to ignore complete subqueries that might contain keywords
  - Everything inside quotes.
etc.
Which would be a step to support things like comma-separated SELECT,
FROM, GROUP BY items.  But that's all way too complicated for this
patch.)

I'll make another patch version, with these few differences.