Re: PATCH: psql tab completion for SELECT

[Tom Lane]

Edmund Horner  writes:
> On Fri, 8 Oct 2021 at 20:01, David Fetter  wrote:
>> What precisely is this supposed to do?

> The main patch 0001 was to add completion for "SELECT " 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

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On Fri, 8 Oct 2021 at 20:01, David Fetter  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 " 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
 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  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

Re: PATCH: psql tab completion for SELECT

[David Fetter]

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  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 87fb63d70f5e08f853a2b8aa94e03c0d665f5000 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Thu, 7 Oct 2021 15:59:06 -0700
Subject: [PATCH v9 1/2] Infrastructure for tab completion in the SELECT list

---
 src/bin/psql/tab-complete.c | 65 ++---
 1 file changed, 54 insertions(+), 11 deletions(-)

diff --git src/bin/psql/tab-complete.c src/bin/psql/tab-complete.c
index ecae9df8ed..5db143523b 100644
--- src/bin/psql/tab-complete.c
+++ src/bin/psql/tab-complete.c
@@ -1017,6 +1017,45 @@ static const VersionedQuery Query_for_list_of_subscriptions[] = {
 	{0, NULL}
 };
 
+static const SchemaQuery Query_for_list_of_selectable_functions[] = {
+	{
+		/* min_server_version */
+		70400,
+		/* catname */
+		"pg_catalog.pg_proc p",
+		/* Disallow functions which take or return pseudotypes which are other than all* or *record */
+		"NOT((ARRAY[p.prorettype] || coalesce(p.proallargtypes, p.proargtypes)) "
+		"&& ARRAY(SELECT oid FROM pg_catalog.pg_type WHERE typtype='p' AND typname !~ '(^all|record$)'))"
+		/* Disallow stored procedures XXX this may change later. */
+		" AND p.prokind <> 'p'"
+		/* viscondition */
+		"pg_catalog.pg_function_is_visible(p.oid)",
+		/* namespace */
+		"p.pronamespace",
+		/* result */
+		"pg_catalog.quote_ident(p.proname)||'('",
+		/* qualresult */
+		NULL
+	},
+	{0, NULL}
+};
+
+/*
+ * This addon is used to find (unqualified) column names to include
+ * alongside the function names from the query above.
+ */
+static const VersionedQuery Query_addon_for_list_of_selectable_attributes[] = {
+	{70400,
+		"UNION ALL "
+		"   SELECT DISTINCT pg_catalog.quote_ident(attname) "
+		" FROM pg_catalog.pg_attribute "
+		"WHERE attnum > 0 "
+		"  AND NOT attisdropped "
+		"  AND substring(pg_catalog.quote_ident(attname),1,%d)='%s'"
+	},
+	{0, NULL}
+};
+
 /*
  * This is a list of all "things" in Pgsql, which can show up after CREATE or
  * DROP; and there is also a query to get a list of them.
@@ -3768,7 +3807,9 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("IS");
 
 /* SELECT */
-	/* naah . . . */
+	else if (TailMatches("SELECT") || TailMatches("SELECT", "ALL|DISTINCT"))
+		COMPLETE_WITH_VERSIONED_SCHEMA_QUERY(Query_for_list_of_selectable_functions,
+			 Query_addon_for_list_of_selectable_attributes);
 
 /* SET, RESET, SHOW */
 	/* Complete with a variable name */
@@ -4432,7 +4473,8 @@ complete_from_versioned_schema_query(const char *text, int state)
  * where %d is the string length of the text and %s the text itself.
  *
  * If both simple_query and schema_query are non-NULL, then we construct
- * a schema query and append the (uninterpreted) string simple_query to it.
+ * a schema query and append the simple_query to it, replacing the %d and %s
+ * as described above.
  *
  * It is assumed that strings should be escaped to become SQL literals
  * (that is, what is in the query is actually ... '%s' ...)
@@ -4579,21 +4621,22 @@ _complete_from_query(const char *simple_query,
 			  " WHERE substring(pg_catalog.quote_ident(nspname) || '.',1,%d) ="
 			  " substring('%s',1,pg_catalog.length(pg_catalog.quote_ident(nspname))+1)) = 1",
 			  char_length, e_text);
-
-			/* If an addon query was provided, use it */
-			if (simple_query)
-appendPQExpBuffer(_buffer, "\n%s", simple_query);
 		}
 		else
 		{
 			Assert(simple_query);
-			/* simple_query is an sprintf-style format string */
-			appendPQExpBuffer(_buffer, simple_query,
-			  char_length, e_text,
-			  e_info_charp, e_info_charp,
-			  e_info_charp2, e_info_charp2);
 		}
 
+		/*
+		 * simple_query is an sprintf-style format string (or it could be NULL, but
+		 * only if this is a schema query with no addon).
+		 */
+		if (simple_query)
+			appendPQExpBuffer(_buffer, simple_query,
+	  char_length, e_text,
+	  e_info_charp, e_info_charp,
+	  e_info_charp2, e_info_charp2);
+
 		/* Limit the number of records in the result */
 		appendPQExpBuffer(_buffer, "\nLIMIT %d",
 		  completion_max_records);
-- 
2.31.1

>From 3764b9fe211db1fa322fa83103d39356928942ae Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Thu, 7 Oct 2021 18:56:52 -0700
Subject: [PATCH v9 2/2] SELECT completion after commas

---
 src/bin/psql/tab-complete.c | 64 -
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git 

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

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


01-psql-select-tab-completion-v8.patch
Description: Binary data


02-select-completion-after-commas.patch
Description: Binary data

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 17 July 2018 at 03:27, Joao De Almeida Pereira
 wrote:
> After playing alittle bit around with the patch I noticed that a comma was
> missing in line 1214
> + 1202 /* min_server_version */
> + 1203 9,
> + 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

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

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


psql-select-tab-completion-v7.patch
Description: Binary data


select-comma-completion-v1.patch
Description: Binary data

Re: PATCH: psql tab completion for SELECT

[Joao De Almeida Pereira]

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 9,
+ 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

Re: PATCH: psql tab completion for SELECT

[Heikki Linnakangas]

(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_stylepartattrs 
partcol2   partexprs  partrelid
page   parameter_mode parameter_typespartclass 
partcollation  partkeypartstrat
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.ca...@vanquo.pezone.net

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 6 April 2018 at 13:29, Peter Eisentraut
 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?

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 6 April 2018 at 13:29, Peter Eisentraut
 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.

Re: PATCH: psql tab completion for SELECT

[Peter Eisentraut]

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

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 8 March 2018 at 17:23, Edmund Horner  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

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

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.

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

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


psql-select-tab-completion-v6.patch
Description: Binary data

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

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


psql-select-tab-completion-v5.patch
Description: Binary data

Re: PATCH: psql tab completion for SELECT

[Tom Lane]

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

Re: PATCH: psql tab completion for SELECT

[Vik Fearing]

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

Re: PATCH: psql tab completion for SELECT

[Vik Fearing]

On 03/05/2018 11:33 AM, Edmund Horner wrote:
> On 5 March 2018 at 21:46, Vik Fearing  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

Re: PATCH: psql tab completion for SELECT

[David G. Johnston]

On Mon, Mar 5, 2018 at 7:41 AM, Tom Lane  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.

Re: PATCH: psql tab completion for SELECT

[Tom Lane]

Edmund Horner  writes:
> On 5 March 2018 at 08:06, Tom Lane  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, ... },
> {9, ... },
> {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

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 5 March 2018 at 21:46, Vik Fearing  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

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 5 March 2018 at 08:06, Tom Lane  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, ... },
{9, ... },
{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+uf6s...@mail.gmail.com

Re: PATCH: psql tab completion for SELECT

[Vik Fearing]

On 03/04/2018 08:06 PM, Tom Lane wrote:
> Edmund Horner  writes:
>> On 26 January 2018 at 13:44, Vik Fearing  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

Re: PATCH: psql tab completion for SELECT

[Tom Lane]

Edmund Horner  writes:
> On 26 January 2018 at 13:44, Vik Fearing  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

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 47909ed..9d0d45b 100644
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*** extern char *filename_completion_functio
*** 71,85 
--- 71,112 
  PQExpBuffer tab_completion_query_buf = NULL;
  
  /*
+  * In some situations, the query to find out what names are available to
+  * complete with must vary depending on server version.  We handle this by
+  * storing a list of queries, each tagged with the minimum server version
+  * it will work for.  Each list must be stored in descending server version
+  * order, so that the first satisfactory query is the one to use.
+  *
+  * When the query string is otherwise constant, an array of VersionedQuery
+  * suffices.  Terminate the array with 0/NULL.  (If the search reaches that
+  * entry, we give up and do no completion.)
+  */
+ typedef struct VersionedQuery
+ {
+ 	int			min_server_version;
+ 	const char *query;
+ } VersionedQuery;
+ 
+ /*
   * This struct is used to define "schema queries", which are custom-built
   * to obtain possibly-schema-qualified names of database objects.  There is
   * enough similarity in the structure that we don't want to repeat it each
   * time.  So we put the components of each query into this struct and
   * assemble them with the common boilerplate in _complete_from_query().
+  *
+  * As with VersionedQuery, we can use an array of these if the query details
+  * must vary across versions.
   */
  typedef struct SchemaQuery
  {
  	/*
+ 	 * If not zero, minimum server version this struct applies to.  If not
+ 	 * zero, there should be a following struct with a smaller minimum server
+ 	 * version; use catname == NULL in the last entry if it should do nothing.
+ 	 */
+ 	int			min_server_version;
+ 
+ 	/*
  	 * Name of catalog or catalogs to be queried, with alias, eg.
  	 * "pg_catalog.pg_class c".  Note that "pg_namespace n" will be added.
  	 */
*** static const char *completion_charp;	/* 
*** 133,138 
--- 160,166 
  static const char *const *completion_charpp;	/* to pass a list of strings */
  static const char *completion_info_charp;	/* to pass a second string */
  static const char *completion_info_charp2;	/* to pass a third string */
+ static const VersionedQuery *completion_vquery; /* to pass a VersionedQuery */
  static const SchemaQuery *completion_squery;	/* to pass a SchemaQuery */
  static bool completion_case_sensitive;	/* completion is case sensitive */
  
*** static bool completion_case_sensitive;	/
*** 140,146 
--- 168,176 
   * A few macros to ease typing. You can use these to complete the given
   * string with
   * 1) The results from a query you pass it. (Perhaps one of those below?)
+  *	  We support both simple and versioned queries.
   * 2) The results from a schema query you pass it.
+  *	  We support both simple and versioned schema queries.
   * 3) The items from a null-pointer-terminated list (with or without
   *	  case-sensitive comparison; see also COMPLETE_WITH_LISTn, below).
   * 4) A string constant.
*** do { \
*** 153,158 
--- 183,194 
  	matches = completion_matches(text, complete_from_query); \
  } while (0)
  
+ #define 

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 26 January 2018 at 13:44, Vik Fearing  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.

Re: PATCH: psql tab completion for SELECT

[Vik Fearing]

On 01/26/2018 01:28 AM, Edmund Horner wrote:
> On 19 January 2018 at 05:37, Vik Fearing  wrote:
>> On 01/18/2018 01:07 AM, Tom Lane wrote:
>>> Edmund Horner  writes:
 On 15 January 2018 at 15:45, Andres Freund  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

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 19 January 2018 at 05:37, Vik Fearing  wrote:
> On 01/18/2018 01:07 AM, Tom Lane wrote:
>> Edmund Horner  writes:
>>> On 15 January 2018 at 15:45, Andres Freund  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.

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 23 January 2018 at 21:47, Vik Fearing <vik.fear...@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.


psql-select-tab-completion-v3.patch
Description: Binary data

Re: PATCH: psql tab completion for SELECT

[Vik Fearing]

On Tue, Jan 23, 2018 at 4:17 AM, Edmund Horner  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 
> 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 
> >
> > 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

Re: PATCH: psql tab completion for SELECT

[Vik Fearing]

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 

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

Re: PATCH: psql tab completion for SELECT

[Vik Fearing]

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

Re: PATCH: psql tab completion for SELECT

[Vik Fearing]

On 01/18/2018 01:07 AM, Tom Lane wrote:
> Edmund Horner  writes:
>> On 15 January 2018 at 15:45, Andres Freund  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

Re: PATCH: psql tab completion for SELECT

[Tom Lane]

Edmund Horner  writes:
> On 15 January 2018 at 15:45, Andres Freund  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

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 15 January 2018 at 15:45, Andres Freund  wrote:
> On January 14, 2018 5:44:01 PM PST, Edmund Horner  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?

Re: PATCH: psql tab completion for SELECT

[Andres Freund]

On January 14, 2018 5:44:01 PM PST, Edmund Horner  wrote:
>On 15 January 2018 at 14:20, Andres Freund  wrote:
>> On January 14, 2018 5:12:37 PM PST, Edmund Horner 
>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.

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

On 15 January 2018 at 14:20, Andres Freund  wrote:
> On January 14, 2018 5:12:37 PM PST, Edmund Horner  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 

(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 

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

Re: PATCH: psql tab completion for SELECT

[Andres Freund]

On January 14, 2018 5:12:37 PM PST, Edmund Horner  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.

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

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


psql-tab-completion-savepoint-v1.patch
Description: Binary data

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

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


psql-select-tab-completion-v2.patch
Description: Binary data

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

[Edmund Horner]

On 11 January 2018 at 03:28, Vik Fearing  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.

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

[Vik Fearing]

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

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

[Edmund Horner]

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.