Re: PATCH: psql tab completion for SELECT

From: Vik Fearing <vik(dot)fearing(at)2ndquadrant(dot)com>
To: Edmund Horner <ejrh00(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PATCH: psql tab completion for SELECT
Date: 2018-01-22 18:41:20
Message-ID: 326b26f4-e1d4-c2f1-9110-aae396193f15@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 01/12/2018 06:59 AM, Edmund Horner wrote:
> Hi, here's a new patch.
>
> This one makes some changes to the criteria for which functions to
> include; namely filtering out trigger functions and those that take or
> return values of type "internal"; and including aggregate and window
> functions. Some of the other checks could be removed as they were
> covered by the "internal" check.

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.

> 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.

> 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.

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)
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Petr Jelinek 2018-01-22 18:41:53 Re: [PATCH] session_replication_role = replica with TRUNCATE
Previous Message Peter Eisentraut 2018-01-22 18:19:28 Re: Add %r substitution for psql prompts to show recovery status