Horton

Thread

Commits

  1. In psql, restore old behavior of Query_for_list_of_functions.

  2. Add infrastructure to support server-version-dependent tab completion.

  1. PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2017-11-06T04:28:55Z

    Hi pgsql-hackers,

Here's a little draft patch to add *some* tab completion ability for
SELECT in psql.  I have often missed the ability, especially with
invocations of utility functions.

It would be nice to be able to suggest column names from the relevant
tables in the query, but, as the SQL language puts the column list
before the FROM clause, we have to use columns from all tables; I'm
certainly not the first to be frustrated by this language feature, but
we can't do much about it.



What my patch does:

For a command line with the single word SELECT, column names and
function names will be used for completions of the next word.
Function names have a "(" suffix added, largely because it makes them
easier to distinguish in the completion list.

Only the first select-list item can be tab-completed; i.e. SELECT foo,
b<tab> won't find column bar.

Examples:

postgres=# select rel<tab>
relacl               relchecks            relhasindex
relhassubclass       (etc.)

postgres=# select str<tab>
string_to_array(  strip(            strpos(



How it works:

The implementation uses a normal non-schema query, because columns
don't have schemas.

The eligible columns are normal, visible columns.

I have tried to filter out the various functions which aren't likely
to be directly used in queries.

The eligible functions are those which are:
  - visible
  - not aggregate or window functions
  - not RI_FKey_* functions
  - not support functions for types, aggregates, operators, languages,
casts, access methods.

Completions for FROM, WHERE, etc. still work, since the additional
completions are only used immediately after the single word SELECT.



Is this likely to be a useful addition to PostgreSQL?

If so, I'll try to get the patch to a good standard.  I am not aiming
for complete support for the SELECT grammar, but just the low-hanging
fruit.

I'm not aware of existing tests for psql tab completion.  But I ran
"make check" anyway, with no problems.

Some other questions about how it should be done:

  - Are my criteria for the columns and function names appropriate?

  - Should I try to use a SchemaQuery so that function schema names can be used?

  - Should I try to support simple cases of multiple columns?  (How?
We can use TailMatches1(",") but how do we tell we aren't into the
FROM-clause or later?)

  - How do we make it work with older servers, e.g. those that predate
some of the newer columns used in the function criteria?

Cheers,
Edmund Horner

  2. Re: PATCH: psql tab completion for SELECT

    David Fetter <david@fetter.org> — 2017-11-12T20:13:22Z

    On Mon, Nov 06, 2017 at 05:28:55PM +1300, Edmund Horner wrote:
> Hi pgsql-hackers,
> 
> Here's a little draft patch to add *some* tab completion ability for
> SELECT in psql.  I have often missed the ability, especially with
> invocations of utility functions.
> 
> It would be nice to be able to suggest column names from the
> relevant tables in the query, but, as the SQL language puts the
> column list before the FROM clause, we have to use columns from all
> tables; I'm certainly not the first to be frustrated by this
> language feature, but we can't do much about it.
> 
> What my patch does:
> 
> For a command line with the single word SELECT, column names and
> function names will be used for completions of the next word.
> Function names have a "(" suffix added, largely because it makes
> them easier to distinguish in the completion list.
> 
> Only the first select-list item can be tab-completed; i.e. SELECT
> foo, b<tab> won't find column bar.

That can be fixed later.

> Examples:
> 
> postgres=# select rel<tab>
> relacl               relchecks            relhasindex
> relhassubclass       (etc.)
> 
> postgres=# select str<tab>
> string_to_array(  strip(            strpos(

Neat!

Please add this to the upcoming (2018-01) commitfest at
https://commitfest.postgresql.org/

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

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

  3. Re: PATCH: psql tab completion for SELECT

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-13T00:55:49Z

    On Mon, Nov 13, 2017 at 5:13 AM, David Fetter <david@fetter.org> wrote:
> Please add this to the upcoming (2018-01) commitfest at
> https://commitfest.postgresql.org/

You may want to scan the following thread as well:
https://www.postgresql.org/message-id/1328820579.11241.4.camel%40vanquo.pezone.net
And also you should be careful about things like WITH clauses...
-- 
Michael

  4. Re: [HACKERS] PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-01-09T15:45:41Z

    On 11/06/2017 05:28 AM, Edmund Horner wrote:
> Hi pgsql-hackers,
> 
> Here's a little draft patch to add *some* tab completion ability for
> SELECT in psql.  I have often missed the ability, especially with
> invocations of utility functions.

Yes, indeed!  The vast majority of what I use interactive psql for is to
call functions.  The fact that it does columns as well is nice, and not
doing anything after the first isn't a problem as we have many other
places where that's the case.  It's annoying, but consistent with
current annoying behavior.  So no points off for that.

> What my patch does:
> 
> For a command line with the single word SELECT, column names and
> function names will be used for completions of the next word.
> Function names have a "(" suffix added, largely because it makes them
> easier to distinguish in the completion list.

And because the user is going to have to type one anyway.  The
completion mechanism adds a space after the paren, which is a needle in
the eye of my OCD, but that's not this patch's fault.

> Only the first select-list item can be tab-completed; i.e. SELECT foo,
> b<tab> won't find column bar.

That's not a problem for me.

> How it works:
> 
> The implementation uses a normal non-schema query, because columns
> don't have schemas.
> 
> The eligible columns are normal, visible columns.

At first I thought this was too broad, but of course it's not because
the column will be visible if the user schema-qualifies the table it's in.

> I have tried to filter out the various functions which aren't likely
> to be directly used in queries.
> 
> The eligible functions are those which are:
>   - visible

Yes.

>   - not aggregate or window functions

Why?  I think this is a big mistake.

>   - not RI_FKey_* functions

Agreed.

>   - not support functions for types, aggregates, operators, languages,
> casts, access methods.

That list looks good to me.  But you forgot to exclude trigger functions.

> Is this likely to be a useful addition to PostgreSQL?

Yes, very much so.

> Some other questions about how it should be done:
> 
>   - Are my criteria for the columns and function names appropriate?

No, I don't think so, as mentioned above.

>   - Should I try to use a SchemaQuery so that function schema names can be used?
> 
>   - Should I try to support simple cases of multiple columns?  (How?
> We can use TailMatches1(",") but how do we tell we aren't into the
> FROM-clause or later?)

I think these two can be pushed to a later patch.  I'd rather have what
you've got now than nothing at all.

>   - How do we make it work with older servers, e.g. those that predate
> some of the newer columns used in the function criteria?

This is something that's going to have to be addressed if this patch is
to be committed.  The way to do this is by writing a query for each
different version you need to support and then call the right one
depending on the pset.sversion global variable.

Marking as Waiting on Author.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  5. Re: [HACKERS] PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-01-10T05:38:57Z

    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.

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.

Regarding support for older versions, psql fails silently if a tab
completion query fails.  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.

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.

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.

  6. Re: [HACKERS] PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-01-10T14:28:27Z

    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

  7. Re: [HACKERS] PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-01-10T23:31:58Z

    On 11 January 2018 at 03:28, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> On 01/10/2018 06:38 AM, Edmund Horner wrote:
>> 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.

There's also the less-critical problem that you can't complete
anything from an already-failed transaction.

Let's just talk about a separate patch that might improve this.

I can think of two options:

1. Use a separate connection for completions.  The big problem with
this is people want to complete on objects created in their current
transaction.  Maybe there's a way to use SET TRANSACTION SNAPSHOT to
access the user's transaction but this seems far too intrusive just
for a bit of tab completion.

2. Use savepoints.  In exec_query you'd have:

    SAVEPOINT _psql_tab_completion;
    run the query
    ROLLBACK TO _psql_tab_completion;   // Just in case there was an
error, but safe to do anyway.
    RELEASE SAVEPOINT _psql_tab_completion;    // This may not be worth doing.

It doesn't help with tab completion in already-failed transactions.
But would it be a reasonable way to make tab completion safer?  I
don't know whether savepoint creation/rollback/release has some
cumulative cost that we'd want to avoid incurring.

  8. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-01-12T05:59:30Z

    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.

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

Edmund

  9. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-01-15T01:12:37Z

    And here's a patch to add savepoint protection for tab completion.

It could definitely use some scrutiny to make sure it's not messing up
the user's transaction.

I added some error checking for the savepoint creation and the
rollback, and then wrapped it in #ifdef NOT_USED (just as the query
error checking is) as I wasn't sure how useful it is for normal use.

But I do feel that if psql unexpectedly messes up the transaction, it
should report it somehow.  How can we tell that we've messed it up for
the user?

Cheers,
Edmund

  10. Re: PATCH: psql tab completion for SELECT

    Andres Freund <andres@anarazel.de> — 2018-01-15T01:20:08Z

    
On January 14, 2018 5:12:37 PM PST, Edmund Horner <ejrh00@gmail.com> wrote:
>And here's a patch to add savepoint protection for tab completion.

It'd be good to explain what that means, so people don't have to read the patch to be able to discuss whether this is a good idea.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  11. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-01-15T01:44:01Z

    On 15 January 2018 at 14:20, Andres Freund <andres@anarazel.de> wrote:
> On January 14, 2018 5:12:37 PM PST, Edmund Horner <ejrh00@gmail.com> wrote:
>>And here's a patch to add savepoint protection for tab completion.
>
> It'd be good to explain what that means, so people don't have to read the patch to be able to discuss whether this is a good idea.


Good idea.

In psql if you have readline support and press TAB, psql will often
run a DB query to get a list of possible completions to type on your
current command line.

It uses the current DB connection for this, which means that if the
tab completion query fails (e.g. because psql is querying catalog
objects that doesn't exist in your server), the current transaction
(if any) fails.  An example of this happening is:

    $ psql -h old_database_server
    psql (10.1, server 9.2.24)
    Type "help" for help.

    postgres=# begin;
    BEGIN
    postgres=# create table foo (id int);
    CREATE TABLE
    postgres=# alter subscription <TAB>

    (no tab completions because the pg_subscription table doesn't
exist on 9.2!  User realises their mistake and types a different
command)

    postgres=# select  * from foo;
    ERROR:  current transaction is aborted, commands ignored until end
of transaction block


This patch:
  - checks that the server supports savepoints (version 8.0 and later)
and that the user is currently idle in a transaction
  - if so, creates a savepoint just before running a tab-completion query
  - rolls back to that savepoint just after running the query

The result is that on an 8.0 or later server, the user's transaction
is still ok:

    $ psql -h old_database_server
    psql (11devel, server 9.2.24)
    Type "help" for help.

    postgres=# begin;
    BEGIN
    postgres=# create table foo (id int);
    CREATE TABLE
    postgres=# alter subscription <TAB>

    (again, no tab completions)

    postgres=# select * from p;
     id
    ----
    (0 rows)

    postgres=# commit;
    COMMIT


Note that only the automatic tab-completion query is protected; the
user can still fail their transaction by typing an invalid command
like ALTER SUBSCRIPTION ;.

  12. Re: PATCH: psql tab completion for SELECT

    Andres Freund <andres@anarazel.de> — 2018-01-15T02:45:18Z

    
On January 14, 2018 5:44:01 PM PST, Edmund Horner <ejrh00@gmail.com> wrote:
>On 15 January 2018 at 14:20, Andres Freund <andres@anarazel.de> wrote:
>> On January 14, 2018 5:12:37 PM PST, Edmund Horner <ejrh00@gmail.com>
>wrote:
>>>And here's a patch to add savepoint protection for tab completion.
>>
>> It'd be good to explain what that means, so people don't have to read
>the patch to be able to discuss whether this is a good idea.
>
>
>Good idea.
>
>In psql if you have readline support and press TAB, psql will often
>run a DB query to get a list of possible completions to type on your
>current command line.
>
>It uses the current DB connection for this, which means that if the
>tab completion query fails (e.g. because psql is querying catalog
>objects that doesn't exist in your server), the current transaction
>(if any) fails.  An example of this happening is:

Ah, that's what I thought. I don't think this is the right fix.


> pg_subscription table doesn't
>exist on 9.2!  User realises their mistake and types a different
>command)
>
>    postgres=# select  * from foo;
>    ERROR:  current transaction is aborted, commands ignored until end
>of transaction block

All worries like this are supposed to check the server version.


Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  13. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-01-17T21:47:11Z

    On 15 January 2018 at 15:45, Andres Freund <andres@anarazel.de> wrote:
> On January 14, 2018 5:44:01 PM PST, Edmund Horner <ejrh00@gmail.com> wrote:
>>In psql if you have readline support and press TAB, psql will often
>>run a DB query to get a list of possible completions to type on your
>>current command line.
>>
>>It uses the current DB connection for this, which means that if the
>>tab completion query fails (e.g. because psql is querying catalog
>>objects that doesn't exist in your server), the current transaction
>>(if any) fails.  An example of this happening is:
>
> Ah, that's what I thought. I don't think this is the right fix.
>
>
>> pg_subscription table doesn't
>>exist on 9.2!  User realises their mistake and types a different
>>command)
>>
>>    postgres=# select  * from foo;
>>    ERROR:  current transaction is aborted, commands ignored until end
>>of transaction block
>
> All worries like this are supposed to check the server version.

In psql there are around 200 such tab completion queries, none of
which checks the server version.  Many would cause the user's
transaction to abort if invoked on an older server.  Identifying the
appropriate server versions for each one would be quite a bit of work.

Is there a better way to make this more robust?

  14. Re: PATCH: psql tab completion for SELECT

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-18T00:07:18Z

    Edmund Horner <ejrh00@gmail.com> writes:
> On 15 January 2018 at 15:45, Andres Freund <andres@anarazel.de> wrote:
>> All worries like this are supposed to check the server version.

> In psql there are around 200 such tab completion queries, none of
> which checks the server version.  Many would cause the user's
> transaction to abort if invoked on an older server.  Identifying the
> appropriate server versions for each one would be quite a bit of work.

> Is there a better way to make this more robust?

Maybe it'd be worth the effort to wrap tab completion queries in
SAVEPOINT/RELEASE SAVEPOINT if we're inside a user transaction
(which we could detect from libpq's state, I believe).

That seems like an independent patch, but it'd be a prerequisite
if you want to issue tab completion queries with version dependencies.

A bigger point here is: do you really want SELECT tab completion
to work only against the latest and greatest server version?
That would become an argument against committing the feature at all;
maybe not enough to tip the scales against it, but still a demerit.

			regards, tom lane

  15. Re: PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-01-18T16:37:30Z

    On 01/18/2018 01:07 AM, Tom Lane wrote:
> Edmund Horner <ejrh00@gmail.com> writes:
>> On 15 January 2018 at 15:45, Andres Freund <andres@anarazel.de> wrote:
>>> All worries like this are supposed to check the server version.
> 
>> In psql there are around 200 such tab completion queries, none of
>> which checks the server version.  Many would cause the user's
>> transaction to abort if invoked on an older server.  Identifying the
>> appropriate server versions for each one would be quite a bit of work.
> 
>> Is there a better way to make this more robust?
> 
> Maybe it'd be worth the effort to wrap tab completion queries in
> SAVEPOINT/RELEASE SAVEPOINT if we're inside a user transaction
> (which we could detect from libpq's state, I believe).
> 
> That seems like an independent patch, but it'd be a prerequisite
> if you want to issue tab completion queries with version dependencies.
> 
> A bigger point here is: do you really want SELECT tab completion
> to work only against the latest and greatest server version?
> That would become an argument against committing the feature at all;
> maybe not enough to tip the scales against it, but still a demerit.

I don't really want such a patch.  I use new psql on old servers all the
time.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  16. Re: PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-01-18T16:38:47Z

    On 01/12/2018 06:59 AM, Edmund Horner wrote:
> Hi, here's a new patch.

Thanks, and sorry for the delay.  I have reviewed this patch, but
haven't had time to put my thoughts together in a coherent message yet.
I will be able to do that tomorrow.

Thank you for your patience.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  17. Re: PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-01-22T18:41:20Z

    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

  18. Re: PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-01-23T08:47:06Z

    On Tue, Jan 23, 2018 at 4:17 AM, Edmund Horner <ejrh00@gmail.com> wrote:

> Hi Vik, thanks for the feedback!
>

Don't forget to include the list :-)
I'm quoting the entirety of the message---which I would normally trim---for
the archives.


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

If this were my patch, I'd have one query for 8.0, and then queries for all
currently supported versions if needed.  If 9.2 only gets 8.0-level
features, that's fine by me.  That limits us to a maximum of six queries.
Just because we keep support for dead versions doesn't mean we have to
retroactively develop for them.  Au contraire.


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

Great!
-- 

Vik Fearing                                          +33 6 46 75 15
36http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et
Support

  19. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-01-26T00:20:07Z

    On 23 January 2018 at 21:47, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> Don't forget to include the list :-)
> I'm quoting the entirety of the message---which I would normally trim---for
> the archives.

Thanks for spotting that.  Sorry list!

> If this were my patch, I'd have one query for 8.0, and then queries for all
> currently supported versions if needed.  If 9.2 only gets 8.0-level
> features, that's fine by me.  That limits us to a maximum of six queries.
> Just because we keep support for dead versions doesn't mean we have to
> retroactively develop for them.  Au contraire.
>> I'll make another patch version, with these few differences.

I managed to do testing against servers on all the versions >= 8.0 and
I discovered that prior to version 8.1 they don't support ALL/ANY on
oidvectors; so I added another query form.

But I don't like having so many different queries, so I am in favour
of trimming it down.  Can I leave that up to the committer to remove
some cases or do we need another patch?

> Great!

Here's another.

  20. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-01-26T00:28:15Z

    On 19 January 2018 at 05:37, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> On 01/18/2018 01:07 AM, Tom Lane wrote:
>> Edmund Horner <ejrh00@gmail.com> writes:
>>> On 15 January 2018 at 15:45, Andres Freund <andres@anarazel.de> wrote:
>>>> All worries like this are supposed to check the server version.
>>
>>> In psql there are around 200 such tab completion queries, none of
>>> which checks the server version.  Many would cause the user's
>>> transaction to abort if invoked on an older server.  Identifying the
>>> appropriate server versions for each one would be quite a bit of work.
>>
>>> Is there a better way to make this more robust?
>>
>> Maybe it'd be worth the effort to wrap tab completion queries in
>> SAVEPOINT/RELEASE SAVEPOINT if we're inside a user transaction
>> (which we could detect from libpq's state, I believe).
>>
>> That seems like an independent patch, but it'd be a prerequisite
>> if you want to issue tab completion queries with version dependencies.
>>
>> A bigger point here is: do you really want SELECT tab completion
>> to work only against the latest and greatest server version?
>> That would become an argument against committing the feature at all;
>> maybe not enough to tip the scales against it, but still a demerit.
>
> I don't really want such a patch.  I use new psql on old servers all the
> time.

I'm not sure where we got with this.  I really should have put this
patch in a separate thread, since it's an independent feature.

The patch mentioned attempts to put savepoints around the tab
completion query where appropriate.

  21. Re: PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-01-26T00:44:12Z

    On 01/26/2018 01:28 AM, Edmund Horner wrote:
> On 19 January 2018 at 05:37, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
>> On 01/18/2018 01:07 AM, Tom Lane wrote:
>>> Edmund Horner <ejrh00@gmail.com> writes:
>>>> On 15 January 2018 at 15:45, Andres Freund <andres@anarazel.de> wrote:
>>>>> All worries like this are supposed to check the server version.
>>>
>>>> In psql there are around 200 such tab completion queries, none of
>>>> which checks the server version.  Many would cause the user's
>>>> transaction to abort if invoked on an older server.  Identifying the
>>>> appropriate server versions for each one would be quite a bit of work.
>>>
>>>> Is there a better way to make this more robust?
>>>
>>> Maybe it'd be worth the effort to wrap tab completion queries in
>>> SAVEPOINT/RELEASE SAVEPOINT if we're inside a user transaction
>>> (which we could detect from libpq's state, I believe).
>>>
>>> That seems like an independent patch, but it'd be a prerequisite
>>> if you want to issue tab completion queries with version dependencies.
>>>
>>> A bigger point here is: do you really want SELECT tab completion
>>> to work only against the latest and greatest server version?
>>> That would become an argument against committing the feature at all;
>>> maybe not enough to tip the scales against it, but still a demerit.
>>
>> I don't really want such a patch.  I use new psql on old servers all the
>> time.
> 
> I'm not sure where we got with this.  I really should have put this
> patch in a separate thread, since it's an independent feature.

Yes, it should have been a separate thread.

> The patch mentioned attempts to put savepoints around the tab
> completion query where appropriate.

I am -1 on this idea.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  22. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-01-26T01:09:40Z

    On 26 January 2018 at 13:44, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> On 01/26/2018 01:28 AM, Edmund Horner wrote:
>> The patch mentioned attempts to put savepoints around the tab
>> completion query where appropriate.
>
> I am -1 on this idea.

May I ask why?  It doesn't stop psql working against older versions,
as it checks that the server supports savepoints.

  23. Re: PATCH: psql tab completion for SELECT

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-04T19:06:48Z

    Edmund Horner <ejrh00@gmail.com> writes:
> On 26 January 2018 at 13:44, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
>> On 01/26/2018 01:28 AM, Edmund Horner wrote:
>>> The patch mentioned attempts to put savepoints around the tab
>>> completion query where appropriate.

>> I am -1 on this idea.

> May I ask why?  It doesn't stop psql working against older versions,
> as it checks that the server supports savepoints.

I looked into this patch and was disappointed to discover that it had
only a very ad-hoc solution to the problem of version-dependent tab
completion queries.  We need something better --- in particular, the
recent prokind changes mean that there needs to be a way to make
SchemaQuery queries version-dependent.

So ... here is a modest proposal.  It invents a VersionedQuery concept
and also extends the SchemaQuery infrastructure to allow those to be
versioned.  I have not taken this nearly as far as it could be taken,
since it's mostly just proposing mechanism.  To illustrate the
VersionedQuery infrastructure, I fixed it so it wouldn't send
publication/subscription queries to pre-v10 servers, and to illustrate
the versioned SchemaQuery infrastructure, I fixed the prokind problems.

If people like this approach, I propose to commit this more or less
as-is.  The select-tab-completion patch would then need to be rewritten
to use this infrastructure, but I think that should be straightforward.
As a separate line of work, the infrastructure could be applied to fix
the pre-existing places where tab completion fails against old servers.
But that's probably work for v12 or beyond, unless somebody's really
motivated to do it right now.

			regards, tom lane

  24. Re: PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-03-05T08:46:44Z

    On 03/04/2018 08:06 PM, Tom Lane wrote:
> Edmund Horner <ejrh00@gmail.com> writes:
>> On 26 January 2018 at 13:44, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
>>> On 01/26/2018 01:28 AM, Edmund Horner wrote:
>>>> The patch mentioned attempts to put savepoints around the tab
>>>> completion query where appropriate.
> 
>>> I am -1 on this idea.
> 
>> May I ask why?  It doesn't stop psql working against older versions,
>> as it checks that the server supports savepoints.
> 
> I looked into this patch and was disappointed to discover that it had
> only a very ad-hoc solution to the problem of version-dependent tab
> completion queries.  We need something better --- in particular, the
> recent prokind changes mean that there needs to be a way to make
> SchemaQuery queries version-dependent.
> 
> So ... here is a modest proposal.  It invents a VersionedQuery concept
> and also extends the SchemaQuery infrastructure to allow those to be
> versioned.  I have not taken this nearly as far as it could be taken,
> since it's mostly just proposing mechanism.  To illustrate the
> VersionedQuery infrastructure, I fixed it so it wouldn't send
> publication/subscription queries to pre-v10 servers, and to illustrate
> the versioned SchemaQuery infrastructure, I fixed the prokind problems.
> 
> If people like this approach, I propose to commit this more or less
> as-is.  The select-tab-completion patch would then need to be rewritten
> to use this infrastructure, but I think that should be straightforward.
> As a separate line of work, the infrastructure could be applied to fix
> the pre-existing places where tab completion fails against old servers.
> But that's probably work for v12 or beyond, unless somebody's really
> motivated to do it right now.

Tab completion on functions in SELECT (a subset of this thread's patch)
is quite important to me personally.  I will work on this in the coming
days.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  25. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-03-05T10:21:28Z

    On 5 March 2018 at 08:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I looked into this patch and was disappointed to discover that it had
> only a very ad-hoc solution to the problem of version-dependent tab
> completion queries.  We need something better --- in particular, the
> recent prokind changes mean that there needs to be a way to make
> SchemaQuery queries version-dependent.

Thanks for the review Tom.

I was avoiding changing things that already worked and hence ended up
with the ad-hoc code.  It's much better have a unified approach to
handling this, though.

> So ... here is a modest proposal.  It invents a VersionedQuery concept
> and also extends the SchemaQuery infrastructure to allow those to be
> versioned.  I have not taken this nearly as far as it could be taken,
> since it's mostly just proposing mechanism.  To illustrate the
> VersionedQuery infrastructure, I fixed it so it wouldn't send
> publication/subscription queries to pre-v10 servers, and to illustrate
> the versioned SchemaQuery infrastructure, I fixed the prokind problems.

(Hmm, I guess the new prokind column may be germane to my query for
callable functions.)

> If people like this approach, I propose to commit this more or less
> as-is.  The select-tab-completion patch would then need to be rewritten
> to use this infrastructure, but I think that should be straightforward.

The SELECT completion patch was definitely ugly.  I think the
VersionedQuery is a nice way of making it less ad-hoc, but the work of
writing the numerous query versions, where necessary, remains.

My patch had 4 versions: PRE_81, PRE_90, PRE_96, and current.  This
could be reformulated as

    static const VersionedQuery
Query_for_list_of_selectable_functions_or_attributes[] = {
        {90600, ... },
        {90000, ... },
        {80100, ... },
        {70300, ... },
        {0, NULL}
    };

I do think the version indicators are nicer when they mean "supported
as of this server version" rather than my "fallback if the server
version is prior to this".

Is it overkill to have so many variations?

I am still just slightly unclear on where we are in relation to the
SAVEPOINT patch -- is that redundant now?

For SELECT support, I would be happy with applying:

  a. Your patch
  b. A reworked simple SELECT patch (possibly with fewer query versions)
  c. A SAVEPOINT patch *if* it's deemed useful [1]

And perhaps later,
  d. A cleverer SELECT patch (supporting additional items after the
first comma, for instance)

> As a separate line of work, the infrastructure could be applied to fix
> the pre-existing places where tab completion fails against old servers.
> But that's probably work for v12 or beyond, unless somebody's really
> motivated to do it right now.
>
>                         regards, tom lane

Edmund

[1] There is more complexity with tab completions and transactions
than I want to tackle just for SELECT; see
https://www.postgresql.org/message-id/flat/CAMyN-kAyFTC4Xavp%2BD6XYOoAOZQW2%3Dc79htji06DXF%2BuF6StOQ%40mail.gmail.com#CAMyN-kAyFTC4Xavp+D6XYOoAOZQW2=c79htji06DXF+uF6StOQ@mail.gmail.com

  26. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-03-05T10:33:28Z

    On 5 March 2018 at 21:46, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> Tab completion on functions in SELECT (a subset of this thread's patch)
> is quite important to me personally.  I will work on this in the coming
> days.

It's something I've missed numerous times in the last few months at
work.  I guess I should really be running a psql with my own
preliminary patches applied; but I'm only a novice pgsql-hacker, and
easily default to using the official one.

If you are going to work on a patch just for functions, you should
probably make it a SchemaQuery.  I did not find a way to support
schema-qualified functions in a query that also returned column names.

Cheers,
Edmund

  27. Re: PATCH: psql tab completion for SELECT

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-05T14:41:43Z

    Edmund Horner <ejrh00@gmail.com> writes:
> On 5 March 2018 at 08:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If people like this approach, I propose to commit this more or less
>> as-is.  The select-tab-completion patch would then need to be rewritten
>> to use this infrastructure, but I think that should be straightforward.

> My patch had 4 versions: PRE_81, PRE_90, PRE_96, and current.  This
> could be reformulated as
>     static const VersionedQuery
> Query_for_list_of_selectable_functions_or_attributes[] = {
>         {90600, ... },
>         {90000, ... },
>         {80100, ... },
>         {70300, ... },
>         {0, NULL}
>     };

Right.

> Is it overkill to have so many variations?

Well, it's whatever you need for the purpose.  We could discuss what the
support cutoff is, but I doubt we'd make it any later than 8.0, so some
types of catalog queries are going to need a lot of variations.

> I am still just slightly unclear on where we are in relation to the
> SAVEPOINT patch -- is that redundant now?

I'm inclined to think it's a bit pointless, if the direction we're
heading is to make the queries actually work on every server version.
Issuing a savepoint would just mask failures.

What would be actually useful is to be able to tab-complete even in
the midst of a failed transaction block ... but savepoints as such
won't get us there, and I have no good ideas about what would.

			regards, tom lane

  28. Re: PATCH: psql tab completion for SELECT

    David G. Johnston <david.g.johnston@gmail.com> — 2018-03-05T14:47:37Z

    On Mon, Mar 5, 2018 at 7:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> What would be actually useful is to be able to tab-complete even in
> the midst of a failed transaction block ... but savepoints as such
> won't get us there, and I have no good ideas about what would.
>

​Why not have psql open two sessions to the backend, one with
application_name 'psql_user' and one with application name "psql_​meta" (or
some such) and have all these queries executed on the psql_meta connection?

David J.

  29. Re: PATCH: psql tab completion for SELECT

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-05T15:04:30Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Mon, Mar 5, 2018 at 7:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What would be actually useful is to be able to tab-complete even in
>> the midst of a failed transaction block ... but savepoints as such
>> won't get us there, and I have no good ideas about what would.

> ​Why not have psql open two sessions to the backend, one with
> application_name 'psql_user' and one with application name "psql_​meta" (or
> some such) and have all these queries executed on the psql_meta connection?

If we did it like that, tab completion would fail to see the session's
temp tables, or objects created in the current open transaction.

People might bitch about using twice as many connections, too, although
likely you could finesse that by only opening the second connection if
tab completion actually happens (so that only interactive sessions have
one).  Still, the local-objects problem seems like a fatal objection.

			regards, tom lane

  30. Re: PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-03-05T15:48:38Z

    On 03/05/2018 11:33 AM, Edmund Horner wrote:
> On 5 March 2018 at 21:46, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
>> Tab completion on functions in SELECT (a subset of this thread's patch)
>> is quite important to me personally.  I will work on this in the coming
>> days.
> 
> It's something I've missed numerous times in the last few months at
> work.  I guess I should really be running a psql with my own
> preliminary patches applied; but I'm only a novice pgsql-hacker, and
> easily default to using the official one.
> 
> If you are going to work on a patch just for functions, you should
> probably make it a SchemaQuery.  I did not find a way to support
> schema-qualified functions in a query that also returned column names.

I meant that I was going to work on your patch, with you, to quickly get
this in v11.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  31. Re: PATCH: psql tab completion for SELECT

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-03-05T15:59:56Z

    On 03/05/2018 11:21 AM, Edmund Horner wrote:
> I am still just slightly unclear on where we are in relation to the
> SAVEPOINT patch -- is that redundant now?

My vote is to reject that patch.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  32. Re: PATCH: psql tab completion for SELECT

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-05T20:52:06Z

    I wrote:
> If people like this approach, I propose to commit this more or less
> as-is.  The select-tab-completion patch would then need to be rewritten
> to use this infrastructure, but I think that should be straightforward.
> As a separate line of work, the infrastructure could be applied to fix
> the pre-existing places where tab completion fails against old servers.
> But that's probably work for v12 or beyond, unless somebody's really
> motivated to do it right now.

Pushed, but while looking it over a second time, I noticed a discrepancy
that ought to be resolved.  In Query_for_list_of_functions, we now have
this for server version >= 11

		/* selcondition */
		"p.prokind IN ('f', 'w')",

and this for prior versions:

		/* selcondition */
		NULL,

The prokind variant is as Peter had it, and NULL is what we were using
here in v10 and earlier.  But it strikes me that these are inconsistent,
in that the prokind variant rejects aggregates while the other variant
doesn't.  We should decide which behavior we want and make that happen
consistently regardless of server version.

I believe the primary reason why the old code didn't reject aggregates
is that there is no GRANT ON AGGREGATE syntax, so that we really need to
include aggregates when completing GRANT ... ON FUNCTION.  Also, other
commands such as DROP FUNCTION will accept an aggregate, although that's
arguably just historical laxity.

If we wanted to tighten this up, one way would be to create a separate
Query_for_list_of_functions_or_aggregates that allows both, and use it
for (only) the GRANT/REVOKE case.  I'm not sure it's worth the trouble
though; I do not recall hearing field complaints about the laxity of
the existing completion rules.  So my inclination is to change the
v11 code to be "p.prokind != 'p'" and leave it at that.

Thoughts?

			regards, tom lane

  33. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-03-06T07:31:31Z

    I've reworked the SELECT completion patch to use the VersionedQuery
infrastructure.

I've also made it a schema query (for the functions), with an addon
for the attributes.  This provides schema-aware completion.

Previously, addons to schema queries were appended verbatim; I've
changed this to use the same interpolation just as in the simple_query
case, so that the attributes can be filtered in the query.  Otherwise,
relevant items can be omitted when they don't make it into the first
1000 rows in the query result, even when only a small number of items
are ultimately presented for completion.

Edmund

  34. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-03-08T04:23:21Z

    New patch that fixes a little bug with appending NULL addons to schema queries.

  35. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-03-08T07:05:41Z

    Some updates on patch status:
  - "version-dependent-tab-completion-1.patch" by Tom Lane was committed in 722408bcd.
  - "psql-tab-completion-savepoint-v1.patch" by Edmund Horner is probably not needed.
  - "psql-select-tab-completion-v6.patch" (the latest) is still under development/review.

  36. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-03-21T06:51:52Z

    On 8 March 2018 at 17:23, Edmund Horner <ejrh00@gmail.com> wrote:
> New patch that fixes a little bug with appending NULL addons to schema queries.

Hi all, I haven't heard anything for a while and so assume you're
beavering away on real features. :)

I've been dogfooding this patch at work, and I am personally pretty
happy with it.

I still think the number of completions on an empty string is a bit
too big, but I don't know what to omit.  There are around 1700
completions on the empty "postgres" database in my testing, and we
show the first 1000 (arbitrarily limited, as the generated SQL has
LIMIT 1000 but no ORDER BY).

Should we just leave it as is?

Whether we improve the filtering or not, I'm optimistic the feature
will be committed in this CF or the next.

I've also been working on adding support for completions after commas,
but I really want to see the current feature finished first.

Cheers,
Edmund

  37. Re: PATCH: psql tab completion for SELECT

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-04-06T01:29:06Z

    On 3/21/18 02:51, Edmund Horner wrote:
> I still think the number of completions on an empty string is a bit
> too big, but I don't know what to omit.  There are around 1700
> completions on the empty "postgres" database in my testing, and we
> show the first 1000 (arbitrarily limited, as the generated SQL has
> LIMIT 1000 but no ORDER BY).
> 
> Should we just leave it as is?
> 
> Whether we improve the filtering or not, I'm optimistic the feature
> will be committed in this CF or the next.

I looked at this a bit now.  I think it still needs some work.

Some of the queries for older versions contain syntax errors that causes
them not to work.

For example, in 80100:

"'internal'::regtype != ALL ([.proargtypes) "

The query definition structures are missing a comma between selcondition
and viscondition.  This causes all the following fields to be
misassigned.  I'm not quite sure how it actually works at all some of
the time.  There might be another bug that offsets this one.

I'd like to see a short comment what is different between each of the
version queries.  You had a list earlier in the thread.

The selection of which functions to show can be further refined.

I don't think the contents of pg_amproc and pg_cast should be excluded.
Some of those functions are useful.  Similarly for pg_operator.oprcode.

Things like oprrest and oprjoin will already be excluded because they
have "internal" arguments.  Similarly for some columns in pg_aggregate.

There are also additional pseudo-types that should be excluded.  See
pg_type.typtype = 'p', except some of those should not be excluded.
Needs more thought.

Considering these issues, I think it would be appropriate to move this
patch to the next commitfest.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  38. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-04-06T03:01:43Z

    On 6 April 2018 at 13:29, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> I looked at this a bit now.  I think it still needs some work.

Hi Peter, thanks for the feedback.

> Some of the queries for older versions contain syntax errors that causes
> them not to work.
>
> For example, in 80100:
>
> "'internal'::regtype != ALL ([.proargtypes) "
>
> The query definition structures are missing a comma between selcondition
> and viscondition.  This causes all the following fields to be
> misassigned.  I'm not quite sure how it actually works at all some of
> the time.  There might be another bug that offsets this one.


One of the problems was a missing comma before the viscondition value
in the struct!

    /* selcondition */
    "p.prorettype NOT IN ('trigger'::regtype, 'internal'::regtype) "
    ...
    "AND p.oid NOT IN (SELECT amproc FROM pg_amproc) "         <--
Should be a comma here!
    /* viscondition */
    "pg_catalog.pg_function_is_visible(p.oid)",

I did some fairly thorough testing against older servers, but that was
before I rewrote it to fit in Tom's versioned SchemaQuery.  I'll fix
this and repeat the testing.

> I'd like to see a short comment what is different between each of the
> version queries.  You had a list earlier in the thread.

Ok, I'll see if I can add that.

> The selection of which functions to show can be further refined.
>
> I don't think the contents of pg_amproc and pg_cast should be excluded.
> Some of those functions are useful.  Similarly for pg_operator.oprcode.
>
> Things like oprrest and oprjoin will already be excluded because they
> have "internal" arguments.  Similarly for some columns in pg_aggregate.
>
> There are also additional pseudo-types that should be excluded.  See
> pg_type.typtype = 'p', except some of those should not be excluded.
> Needs more thought.

Ok I'll play with these.

Selection of which functions and attributes to show is something with
probably no right answer, so we might have to settle for "close
enough" at some point.

> Considering these issues, I think it would be appropriate to move this
> patch to the next commitfest.

  39. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-04-08T01:59:15Z

    On 6 April 2018 at 13:29, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> The selection of which functions to show can be further refined.
>
> I don't think the contents of pg_amproc and pg_cast should be excluded.
> Some of those functions are useful.  Similarly for pg_operator.oprcode.
>
> Things like oprrest and oprjoin will already be excluded because they
> have "internal" arguments.  Similarly for some columns in pg_aggregate.

You're right.  There were lots of useful functions being excluded by
the pg_amproc, pg_cast, and oprcode checks.  And all the oprrest and
oprjoin functions are already excluded, so I can remove that check.

Perhaps we should remove the "appears in this catalog table" exclusion
checks, and just use argument and return type?

> There are also additional pseudo-types that should be excluded.  See
> pg_type.typtype = 'p', except some of those should not be excluded.
> Needs more thought.

I don't know much about some of the pseudotypes but this is what I propose:

any*, record, _record, cstring should NOT be excluded
void should NOT be excluded for return type (and perhaps in general;
void_out and void_send are callable, if not hugely useful in psql)

trigger, event_trigger should be excluded
internal, opaque, unknown, pg_ddl_command should be excluded
language_handler, tsm_handler, index_am_handler, fdw_handler should be excluded

For modern servers, our query can be simplified to something like:

SELECT distinct pg_catalog.quote_ident(p.proname)
FROM pg_catalog.pg_proc p
WHERE NOT arrayoverlap(ARRAY['internal', 'event_trigger', 'internal',
'opaque', 'unknown', 'pg_ddl_command', 'language_handler',
'tsm_handler', 'index_am_handler', 'fdw_handler']::regtype[]::oid[],
    ARRAY(SELECT p.prorettype UNION SELECT unnest(proargtypes)));

What do you think?

  40. Re: PATCH: psql tab completion for SELECT

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-07-16T12:00:24Z

    (trimmed CC list to evade gmail's spam filter, sorry)

On 21/03/18 08:51, Edmund Horner wrote:
> Hi all, I haven't heard anything for a while and so assume you're
> beavering away on real features. :)
> 
> I've been dogfooding this patch at work, and I am personally pretty
> happy with it.
> 
> I still think the number of completions on an empty string is a bit
> too big, but I don't know what to omit.  There are around 1700
> completions on the empty "postgres" database in my testing, and we
> show the first 1000 (arbitrarily limited, as the generated SQL has
> LIMIT 1000 but no ORDER BY).
> 
> Should we just leave it as is?

> Whether we improve the filtering or not, I'm optimistic the feature
> will be committed in this CF or the next.
> 
> I've also been working on adding support for completions after commas,
> but I really want to see the current feature finished first.

Playing around with this a little bit, I'm not very satisfied with the 
completions. Sometimes this completes too much, and sometimes too 
little. All of this has been mentioned in this and the other thread [1] 
already, this just my opinion on all this.

Too much:

postgres=# \d lp
                    Table "public.lp"
    Column  |  Type   | Collation | Nullable | Default
----------+---------+-----------+----------+---------
   id       | integer |           |          |
   partkey  | text    |           |          |
   partcol1 | text    |           |          |
   partcol2 | text    |           |          |
Partition key: LIST (partkey)
Number of partitions: 1000 (Use \d+ to list them.)

postgres=# select pa[TAB]
pad_attribute      parameter_default  parameter_style    partattrs 
partcol2           partexprs          partrelid
page               parameter_mode     parameter_types    partclass 
partcollation      partkey            partstrat
pageno             parameter_name     parse_ident(       partcol1 
partdefid          partnatts          passwd

Too little:

postgres=# select partkey, p[TAB]
[no completions]


Then there's the multi-column case, which seems weird (to a user - I 
know the implementation and understand why):

postgres=# select partkey, partc[TAB]
[no completions]

And I'd love this case, where go back to edit the SELECT list, after 
already typing the FROM part, to be smarter:

postgres=# select p[TAB] FROM lp;
Display all 370 possibilities? (y or n)

There's something weird going on with system columns, from a user's 
point of view:

SELECT oi[TAB]
oid              oidvectortypes(

postgres=# select xm[TAB]
xmin                          xmlcomment( xml_is_well_formed_content( 
xmlvalidate(
xmlagg(                       xml_is_well_formed( 
xml_is_well_formed_document(

So oid and xmin are completed. But "xmax" and "ctid" are not. I think 
this is because in fact none of the system columns are completed, but 
there happen to be some tables with regular columns named "oid" and 
"xmin". So it makes sense once you know that, but it's pretty confusing 
to a user. Tab-completion is a great way for a user to explore what's 
available, so it's weird that some system columns are effectively 
completed while others are not.

I'd like to not include set-returning functions from the list. Although 
you can do "SELECT generate_series(1,10)", I'd like to discourage people 
from doing that, since using set-returning functions in the target list 
is a PostgreSQL extension to the SQL standard, and IMHO the "SELECT * 
FROM generate_series(1,10)" syntax is easier to understand and works 
more sanely with joins etc. Conversely, it would be really nice to 
include set-returning function in the completions after FROM.


I understand that there isn't much you can do about some of those 
things, short of adding a ton of new context information about previous 
queries and heuristics. I think the completion needs to be smarter to be 
acceptable. I don't know what exactly to do, but perhaps someone else 
has ideas.

I'm also worried about performance, especially of the query to get all 
the column names. It's pretty much guaranteed to do perform a 
SeqScan+Sort+Unique on pg_attribute. pg_attribute can be very large. In 
my little test database, with the above 1000-partition table, hitting 
tab after "SELECT " takes about 1 second, until you get the "Display all 
1000 possibilities" prompt. And that's not a particularly large schema.

- Heikki


PS. All the references to "pg_attribute" and other system tables, and 
functions, need to be schema-qualified, as "pg_catalog.pg_attribute" and 
so forth.

[1] 
https://www.postgresql.org/message-id/1328820579.11241.4.camel@vanquo.pezone.net

  41. Re: PATCH: psql tab completion for SELECT

    Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> — 2018-07-16T15:27:27Z

    Hello,

postgres=# select partkey, partc[TAB]
> [no completions]
>

From the thread, I believe that this feature will be implemented in a after
patch.

>
> And I'd love this case, where go back to edit the SELECT list, after
> already typing the FROM part, to be smarter:
>
> postgres=# select p[TAB] FROM lp;
> Display all 370 possibilities? (y or n)
>

I believe this would be a very interesting feature indeed.


After playing alittle bit around with the patch I noticed that a comma was
missing in line 1214
+ 1202                 /* min_server_version */
+ 1203                 90000,
+ 1204                 /* catname */
+ 1205                 "pg_catalog.pg_proc p",
+ 1206                 /* selcondition */
+ 1207                 "p.prorettype NOT IN ('trigger'::regtype,
'internal'::regtype) "
+ 1208                 "AND 'internal'::regtype != ALL (p.proargtypes) "
+ 1209                 "AND p.oid NOT IN (SELECT
unnest(array[typinput,typoutput,typreceive,typsend,typmodin,typmodout,typanalyze])
FROM pg_type) "
+ 1210                 "AND p.oid NOT IN (SELECT
unnest(array[aggtransfn,aggfinalfn]) FROM pg_aggregate) "
+ 1211                 "AND p.oid NOT IN (SELECT
unnest(array[oprcode,oprrest,oprjoin]) FROM pg_operator) "
+ 1212                 "AND p.oid NOT IN (SELECT
unnest(array[lanplcallfoid,laninline,lanvalidator]) FROM pg_language) "
+ 1213                 "AND p.oid NOT IN (SELECT castfunc FROM pg_cast) "
+ 1214                 "AND p.oid NOT IN (SELECT amproc FROM pg_amproc) "
+ 1215                 /* viscondition */
+ 1216                 "pg_catalog.pg_function_is_visible(p.oid)",

To catch these typos would be good if we could get some testing around
psql.
(Newbie question: do we have any kind of testing around tools like psql?)

Thanks
Joao

  42. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-07-17T01:31:20Z

    On 17 July 2018 at 00:00, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Playing around with this a little bit, I'm not very satisfied with the
> completions. Sometimes this completes too much, and sometimes too little.
> All of this has been mentioned in this and the other thread [1] already,
> this just my opinion on all this.

Hi Heikki, thanks for getting this thread active again.

I do actually have another patch, which I didn't post yet because
everyone was busy with v11, and I wanted to wait for more feedback
about inclusions/exclusions.  Holding it back now seems like a mistake
(especially since the last patch had a bug) so here it is.

There's also a patch, to be applied on top, that adds completion after commas.

> Too much:
>
> postgres=# \d lp
>                    Table "public.lp"
>    Column  |  Type   | Collation | Nullable | Default
> ----------+---------+-----------+----------+---------
>   id       | integer |           |          |
>   partkey  | text    |           |          |
>   partcol1 | text    |           |          |
>   partcol2 | text    |           |          |
> Partition key: LIST (partkey)
> Number of partitions: 1000 (Use \d+ to list them.)
>
> postgres=# select pa[TAB]
> pad_attribute      parameter_default  parameter_style    partattrs partcol2
> partexprs          partrelid
> page               parameter_mode     parameter_types    partclass
> partcollation      partkey            partstrat
> pageno             parameter_name     parse_ident(       partcol1 partdefid
> partnatts          passwd

I agree that there is too much.  I don't know what the best way to
reduce it is, short of specifically excluding certain things.  In
theory, I think the query could be sensitive to how much text is
entered, for example, let pa[TAB] not show partrelid and other columns
from pg_partition, but part[TAB] show them; the general rule could be
"only show attributes for system tables if 3 or more letters entered".
But I'm wary of overcomplicating a patch that is (IMHO) a useful
improvement even if simple.

> Too little:
>
> postgres=# select partkey, p[TAB]
> [no completions]
>
>
> Then there's the multi-column case, which seems weird (to a user - I know
> the implementation and understand why):
>
> postgres=# select partkey, partc[TAB]
> [no completions]

Yep, there was no completion after commas in the previous patches.

> And I'd love this case, where go back to edit the SELECT list, after already
> typing the FROM part, to be smarter:
>
> postgres=# select p[TAB] FROM lp;
> Display all 370 possibilities? (y or n)

I don't know enough about readline to estimate how easy this would be.
I think all our current completions use only the text up to the
cursor.

> There's something weird going on with system columns, from a user's point of
> view:
>
> SELECT oi[TAB]
> oid              oidvectortypes(
>
> postgres=# select xm[TAB]
> xmin                          xmlcomment( xml_is_well_formed_content(
> xmlvalidate(
> xmlagg(                       xml_is_well_formed(
> xml_is_well_formed_document(
>
> So oid and xmin are completed. But "xmax" and "ctid" are not. I think this
> is because in fact none of the system columns are completed, but there
> happen to be some tables with regular columns named "oid" and "xmin". So it
> makes sense once you know that, but it's pretty confusing to a user.
> Tab-completion is a great way for a user to explore what's available, so
> it's weird that some system columns are effectively completed while others
> are not.

You are correct, system columns are excluded, but of course there are
always the same column names in other tables.  Do you think we should
just include all columns?

> I'd like to not include set-returning functions from the list. Although you
> can do "SELECT generate_series(1,10)", I'd like to discourage people from
> doing that, since using set-returning functions in the target list is a
> PostgreSQL extension to the SQL standard, and IMHO the "SELECT * FROM
> generate_series(1,10)" syntax is easier to understand and works more sanely
> with joins etc. Conversely, it would be really nice to include set-returning
> function in the completions after FROM.

I'm happy to exclude SRFs after SELECT if others agree.  Including
them after FROM seems worthwhile, but best left for a different patch?

> I understand that there isn't much you can do about some of those things,
> short of adding a ton of new context information about previous queries and
> heuristics. I think the completion needs to be smarter to be acceptable. I
> don't know what exactly to do, but perhaps someone else has ideas.
>
> I'm also worried about performance, especially of the query to get all the
> column names. It's pretty much guaranteed to do perform a
> SeqScan+Sort+Unique on pg_attribute. pg_attribute can be very large. In my
> little test database, with the above 1000-partition table, hitting tab after
> "SELECT " takes about 1 second, until you get the "Display all 1000
> possibilities" prompt. And that's not a particularly large schema.

The indexes on pg_attribute both start with attrelid; unless we know
what small set of relations we want attributes for, I don't think we
can avoid a SeqScan.  Maybe there's a way to extract the attribute
name part from the text entered and use it in a "WHERE attname LIKE
'%s' " qualifier, to encourage an IndexOnlyScan for some cases.

> PS. All the references to "pg_attribute" and other system tables, and
> functions, need to be schema-qualified, as "pg_catalog.pg_attribute" and so
> forth.

(Thanks for the advice.  I managed to sneak this into the updated patch.)

  43. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-07-17T01:44:59Z

    On 17 July 2018 at 03:27, Joao De Almeida Pereira
<jdealmeidapereira@pivotal.io> wrote:
> After playing alittle bit around with the patch I noticed that a comma was
> missing in line 1214
> + 1202                 /* min_server_version */
> + 1203                 90000,
> + 1204                 /* catname */
> + 1205                 "pg_catalog.pg_proc p",
> + 1206                 /* selcondition */
> + 1207                 "p.prorettype NOT IN ('trigger'::regtype,
> 'internal'::regtype) "
> + 1208                 "AND 'internal'::regtype != ALL (p.proargtypes) "
> + 1209                 "AND p.oid NOT IN (SELECT
> unnest(array[typinput,typoutput,typreceive,typsend,typmodin,typmodout,typanalyze])
> FROM pg_type) "
> + 1210                 "AND p.oid NOT IN (SELECT
> unnest(array[aggtransfn,aggfinalfn]) FROM pg_aggregate) "
> + 1211                 "AND p.oid NOT IN (SELECT
> unnest(array[oprcode,oprrest,oprjoin]) FROM pg_operator) "
> + 1212                 "AND p.oid NOT IN (SELECT
> unnest(array[lanplcallfoid,laninline,lanvalidator]) FROM pg_language) "
> + 1213                 "AND p.oid NOT IN (SELECT castfunc FROM pg_cast) "
> + 1214                 "AND p.oid NOT IN (SELECT amproc FROM pg_amproc) "
> + 1215                 /* viscondition */
> + 1216                 "pg_catalog.pg_function_is_visible(p.oid)",
>
> To catch these typos would be good if we could get some testing around psql.
> (Newbie question: do we have any kind of testing around tools like psql?)
>
> Thanks
> Joao

Hi Joao,

Ah yes, the embarrassing missing comma.  I kind of wish my IDE or
compiler had pointed it out to me, but how was it to know that I
foolishly combining two struct fields?  Sadly, I think the best
defence right now is to have others scrutinise the code.

I attached a new version of the patch in reply to Heikki.  Would you
care to take a careful look for me?

I think some automated test framework for the tab completion queries
is possible, by calling psql_completion and examining the results.
Maybe someone will volunteer...

Edmund

  44. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2018-09-22T06:53:55Z

    Hi all,

Here are some rebased versions of the last two patches.  No changes in
functionality, except a minor case sensitivity fix in the "completion
after commas" patch.

Edmund

  45. Re: PATCH: psql tab completion for SELECT

    David Fetter <david@fetter.org> — 2021-10-08T07:01:23Z

    On Sat, Sep 22, 2018 at 06:53:55PM +1200, Edmund Horner wrote:
> Hi all,
> 
> Here are some rebased versions of the last two patches.  No changes in
> functionality, except a minor case sensitivity fix in the "completion
> after commas" patch.
> 
> Edmund

I've rebased and updated these to be more principled about what
functions could be tab completed.

Still missing: tests.

What precisely is this supposed to do?

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

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

  46. Re: PATCH: psql tab completion for SELECT

    Edmund Horner <ejrh00@gmail.com> — 2021-10-14T03:53:37Z

    On Fri, 8 Oct 2021 at 20:01, David Fetter <david@fetter.org> wrote:

> On Sat, Sep 22, 2018 at 06:53:55PM +1200, Edmund Horner wrote:
> > Hi all,
> >
> > Here are some rebased versions of the last two patches.  No changes in
> > functionality, except a minor case sensitivity fix in the "completion
> > after commas" patch.
> >
> > Edmund
>
> I've rebased and updated these to be more principled about what
> functions could be tab completed.
>
> Still missing: tests.
>
> What precisely is this supposed to do?


Hi David,

The main patch 0001 was to add completion for "SELECT <tab>" using
attributes, functions, and a couple of hard-coded options.

The changes to _complete_from_query were so that simple_query (when used in
addon mode, as for this feature) could have the current word interpolated
into it.  This feature uses a schema query to get the function names, as
they can be schema qualified, and an addon to get the column names, as they
can't (they could be table-qualified, but that's hard when you don't know
what the table aliases will be).  Previously existing queries using addons
did not need to interpolate into them, but this one did, hence the change.

The second patch 0002 was to build on 0001 and add support for pressing
<tab> after a comma while in the column list, i.e. when SELECT was the most
recent major keyword (major being defined by the list of keywords in
CurrentQueryPartMatches).

There's a bit of trickery around completing "," by adding a single space.
Without the space, the next <tab> will try to complete using the whole word
including the part before the comma.

Regarding testing, I can't see any automated tests for tab completion.
Though it seems to me we could do it with a readline driver program of some
sorts, if the current regression test scripts don't work interactively.

Cheers,
Edmund

  47. Re: PATCH: psql tab completion for SELECT

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-14T04:19:06Z

    Edmund Horner <ejrh00@gmail.com> writes:
> On Fri, 8 Oct 2021 at 20:01, David Fetter <david@fetter.org> wrote:
>> What precisely is this supposed to do?

> The main patch 0001 was to add completion for "SELECT <tab>" using
> attributes, functions, and a couple of hard-coded options.

This sort of thing has been proposed a few times, but never made it to
commit, because there's basically no way to limit the completions to a
usefully small set of possibilities.  SQL wasn't designed to make this
easy :-(

> Regarding testing, I can't see any automated tests for tab completion.

src/bin/psql/t/010_tab_completion.pl

			regards, tom lane