Subject: Re: [HACKERS] PATCH: psql tab completion for SELECT
To: Edmund Horner <ejrh00@gmail.com>
Cc: pgsql-hackers@postgresql.org
References: 
 <CAMyN-kB_xrU4iYdcF1j=tijgO1DSyjtb3j96O4UEj91XZrZcMg@mail.gmail.com>
 <034f9a0a-d426-251a-5f81-6083d730ee9e@2ndquadrant.com>
 <CAMyN-kAFsW_2fv1J3iYzvNAtr00hp_GWcZdaxV6dkdGzw9_-FQ@mail.gmail.com>
From: Vik Fearing <vik.fearing@2ndquadrant.com>
Organization: 2ndQuadrant France
Message-ID: <2f1079b8-4c0d-b89e-e1d2-ab83e7dff978@2ndquadrant.com>
Date: Wed, 10 Jan 2018 15:28:27 +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-kAFsW_2fv1J3iYzvNAtr00hp_GWcZdaxV6dkdGzw9_-FQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 01/10/2018 06:38 AM, Edmund Horner wrote:
> Hi Vik, thanks so much for the comments and the offer to review!
> 
> I kept a low profile after my first message as there was already a
> commitfest in progress, but now I'm working on a V2 patch.
> 
> I will include aggregate and window functions as you suggest.  And
> I'll exclude trigger functions.

Thanks.

> I'd like to exclude more if we could, because there are already over
> 1000 completions on an empty database.  I had thought of filtering out
> functions with an argument of type internal but couldn't figure out
> how to interrogate the proargtypes oidvector in a nice way.

If you just want to do internal,

    WHERE regtype 'internal' <> ALL (proargtypes)

but if you'll want to add more banned types, then

    WHERE NOT (proargtypes::regtype[] && array['internal',
'unknown']::regtype[])

This is a very good test to add.

> Regarding support for older versions, psql fails silently if a tab
> completion query fails.

No it doesn't, see below.

> We could just let it do this, which is what
> happens with, for example, ALTER PUBLICATION against a 9.6 server.  I
> can't see any existing completions that check the server version --
> but completions that don't work against older versions, like ALTER
> PUBLICATION, also aren't useful for older versions.  SELECT is a bit
> different as it can be useful against older versions that don't have
> the pg_aggregate.aggcombinefn that my query uses for filtering out
> aggregation support functions.

That's a bug in my opinion, but not one that needs to be addressed by
this patch.

There are no completions that cater to the server version (yet) but all
the \d stuff certainly does.  You can see that in action in
src/bin/psql/describe.c as well as all over the place in pg_dump and the
like.

> There's also the small irritation that when a completion query fails,
> it aborts the user's current transaction to provide an incentive for
> handling older versions gracefully.

That is actively hostile and not at all what I would consider "silently
failing".  If you don't want to do the multiple versions thing, you
should at least check if you're on 9.6+ before issuing the query.

> Regarding multiple columns, I have an idea that if we check that:
> 
> a) the last of any SELECT/WHERE/GROUP BY/etc.-level keyword is a
> SELECT (i.e. that we're in the SELECT clause of the query), and
> b) the last word was a comma (or ended in a comma).
> we can then proffer the column/function completions.
> 
> There may be other completions that could benefit from such a check,
> e.g. table names after a comma in the FROM clause, but I just want to
> get SELECT working first.

I would like to see this as a separate patch.  Let's keep this one
simple and just do like the other things currently do.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support