Subject: Re: PATCH: psql tab completion for SELECT
To: Edmund Horner <ejrh00@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: 
 <CAMyN-kB_xrU4iYdcF1j=tijgO1DSyjtb3j96O4UEj91XZrZcMg@mail.gmail.com>
 <CAMyN-kCyeh2E7k1Dhb3YQKa-bSp_wHvqZsWoGRHKh_ukfLm9WQ@mail.gmail.com>
From: Vik Fearing <vik.fearing@2ndquadrant.com>
Organization: 2ndQuadrant France
Message-ID: <326b26f4-e1d4-c2f1-9110-aae396193f15@2ndquadrant.com>
Date: Mon, 22 Jan 2018 19:41:20 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: 
 <CAMyN-kCyeh2E7k1Dhb3YQKa-bSp_wHvqZsWoGRHKh_ukfLm9WQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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