Re: PATCH: psql tab completion for SELECT
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
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
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
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
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
On 17 July 2018 at 00:00, Heikki Linnakangas wrote: > Playing around with this a little bit, I'm not very satisfied with the > completions. Sometimes this completes too much, and sometimes too little. > All of this has been mentioned in this and the other thread [1] already, > this just my opinion on all this. Hi Heikki, thanks for getting this thread active again. I do actually have another patch, which I didn't post yet because everyone was busy with v11, and I wanted to wait for more feedback about inclusions/exclusions. Holding it back now seems like a mistake (especially since the last patch had a bug) so here it is. There's also a patch, to be applied on top, that adds completion after commas. > Too much: > > postgres=# \d lp >Table "public.lp" >Column | Type | Collation | Nullable | Default > --+-+---+--+- > id | integer | | | > partkey | text| | | > partcol1 | text| | | > partcol2 | text| | | > Partition key: LIST (partkey) > Number of partitions: 1000 (Use \d+ to list them.) > > postgres=# select pa[TAB] > pad_attribute parameter_default parameter_stylepartattrs partcol2 > partexprs partrelid > page parameter_mode parameter_typespartclass > partcollation partkeypartstrat > pageno parameter_name parse_ident( partcol1 partdefid > partnatts passwd I agree that there is too much. I don't know what the best way to reduce it is, short of specifically excluding certain things. In theory, I think the query could be sensitive to how much text is entered, for example, let pa[TAB] not show partrelid and other columns from pg_partition, but part[TAB] show them; the general rule could be "only show attributes for system tables if 3 or more letters entered". But I'm wary of overcomplicating a patch that is (IMHO) a useful improvement even if simple. > Too little: > > postgres=# select partkey, p[TAB] > [no completions] > > > Then there's the multi-column case, which seems weird (to a user - I know > the implementation and understand why): > > postgres=# select partkey, partc[TAB] > [no completions] Yep, there was no completion after commas in the previous patches. > And I'd love this case, where go back to edit the SELECT list, after already > typing the FROM part, to be smarter: > > postgres=# select p[TAB] FROM lp; > Display all 370 possibilities? (y or n) I don't know enough about readline to estimate how easy this would be. I think all our current completions use only the text up to the cursor. > There's something weird going on with system columns, from a user's point of > view: > > SELECT oi[TAB] > oid oidvectortypes( > > postgres=# select xm[TAB] > xmin xmlcomment( xml_is_well_formed_content( > xmlvalidate( > xmlagg( xml_is_well_formed( > xml_is_well_formed_document( > > So oid and xmin are completed. But "xmax" and "ctid" are not. I think this > is because in fact none of the system columns are completed, but there > happen to be some tables with regular columns named "oid" and "xmin". So it > makes sense once you know that, but it's pretty confusing to a user. > Tab-completion is a great way for a user to explore what's available, so > it's weird that some system columns are effectively completed while others > are not. You are correct, system columns are excluded, but of course there are always the same column names in other tables. Do you think we should just include all columns? > I'd like to not include set-returning functions from the list. Although you > can do "SELECT generate_series(1,10)", I'd like to discourage people from > doing that, since using set-returning functions in the target list is a > PostgreSQL extension to the SQL standard, and IMHO the "SELECT * FROM > generate_series(1,10)" syntax is easier to understand and works more sanely > with joins etc. Conversely, it would be really nice to include set-returning > function in the completions after FROM. I'm happy to exclude SRFs after SELECT if others agree. Including them after FROM seems worthwhile, but best left for a different patch? > I understand that there isn't much you can do about some of those things, > short of adding a ton of new context information about previous queries and > heuristics. I think the completion needs to be smarter to be acceptable. I > don't know what exactly to do, but perhaps someone else has ideas. > > I'm also worried about performance, especially of the query to get all the > column names. It's pretty much guaranteed to do perform a > SeqScan+Sort+Unique on pg_attribute. pg_attribute can be very large. In my > little test database, with the above 1000-partition table, hitting tab after > "SELECT " takes about 1 second, until you get the "Display all 1000 >
Re: PATCH: psql tab completion for SELECT
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
(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
On 6 April 2018 at 13:29, Peter Eisentrautwrote: > 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
On 6 April 2018 at 13:29, Peter Eisentrautwrote: > 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
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
On 8 March 2018 at 17:23, Edmund Hornerwrote: > 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
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
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
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
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
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
On 03/05/2018 11:33 AM, Edmund Horner wrote: > On 5 March 2018 at 21:46, Vik Fearingwrote: >> 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
On Mon, Mar 5, 2018 at 7:41 AM, Tom Lanewrote: > 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
Edmund Hornerwrites: > 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
On 5 March 2018 at 21:46, Vik Fearingwrote: > 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
On 5 March 2018 at 08:06, Tom Lanewrote: > 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
On 03/04/2018 08:06 PM, Tom Lane wrote: > Edmund Hornerwrites: >> 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
Edmund Hornerwrites: > 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
On 26 January 2018 at 13:44, Vik Fearingwrote: > 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
On 01/26/2018 01:28 AM, Edmund Horner wrote: > On 19 January 2018 at 05:37, Vik Fearingwrote: >> 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
On 19 January 2018 at 05:37, Vik Fearingwrote: > 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
On 23 January 2018 at 21:47, Vik Fearingwrote: > 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
On Tue, Jan 23, 2018 at 4:17 AM, Edmund Hornerwrote: > 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
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
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
On 01/18/2018 01:07 AM, Tom Lane wrote: > Edmund Hornerwrites: >> 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
Edmund Hornerwrites: > 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
On 15 January 2018 at 15:45, Andres Freundwrote: > 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
On January 14, 2018 5:44:01 PM PST, Edmund Hornerwrote: >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
On 15 January 2018 at 14:20, Andres Freundwrote: > 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
On January 14, 2018 5:12:37 PM PST, Edmund Hornerwrote: >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
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
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