PATCH: psql tab completion for SELECT

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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:
/messages/by-id/1328820579.11241.4.camel@vanquo.pezone.net
And also you should be careful about things like WITH clauses...
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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.

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

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.

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

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

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.

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

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.

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?

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

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

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

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

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

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.

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.