Re: Support tab completion for upper character inputs in psql

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>
Cc: Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Japin Li <japinli(at)hotmail(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>, "smithpb2250(at)gmail(dot)com" <smithpb2250(at)gmail(dot)com>, "david(dot)zhang(at)highgo(dot)ca" <david(dot)zhang(at)highgo(dot)ca>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Support tab completion for upper character inputs in psql
Date: 2022-01-27 20:23:42
Message-ID: 2645164.1643315022@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> I spent some time contemplating my navel about the concerns I raised
> upthread about double-quoted identifiers. I concluded that the reason
> things don't work well in that area is that we're trying to get all the
> work done by applying quote_ident() on the backend side and then
> ignoring quoting considerations in tab-complete itself. That sort of
> works, but not terribly well. The currently proposed patch is sticking
> a toe into the water of dealing with quoting/downcasing in tab-complete,
> but we need to go a lot further. I propose that we ought to drop the
> use of quote_ident() in the tab completion queries altogether, instead
> having the backend return names as-is, and doing all the dequoting and
> requoting work in tab-complete.

Here's a fleshed-out patch series for this idea.

0001 below is a more evolved version of my previous WIP patch.
The main thing that's changed is that I found that it no longer
works to handle extra keywords (such as adding COLUMN to the list
of attribute names after ALTER TABLE tab RENAME) via tab-complete's
traditional method of sticking on "UNION SELECT 'foo'". That's
because such a result row looks like something that needs to be
double-quoted, which of course makes the completion incorrect.
So I've created a side-channel whereby _complete_from_query() can
return some verbatim keywords alongside the actual query results.
(I think this is a good thing anyway, because the UNION method is
incredibly wasteful of server cycles. Yeah, I know that these
queries only need to run at human speed, but if your server is
heavily loaded you might still not appreciate the extra cycles.)

Having done that, I solved the format_type problem by just dropping
the use of format_type altogether, and returning only the normal
pg_type.typname entries. The only cases where format_type did
anything useful for us are the small number of built-in types where
it substitutes a SQL-mandated name, and we can treat those like
keywords (as indeed they are). The list of such types changes
seldom enough that I don't think it's a huge maintenance burden to
have one more place that knows about them.

BTW, I was amused to notice that many of the format_type special cases
don't actually work as completions, and never have. For example,
if we return both "timestamp with time zone" and "timestamp without
time zone", readline can complete as far as "timestamp with", but
further completion fails because "timestamp" is now seen as a previous
word that's not part of what's to be completed. So I've dropped those
cases from the keyword list. Maybe somebody will get interested in
figuring a way to make that work, but IMO the cost/benefit ratio for
such effort would be pretty bad.

Incidentally, I found that some of the completion queries were
intentionally ignoring the given prefix text, with stuff like

/* the silly-looking length condition is just to eat up the current word */
" WHERE ... (%d = pg_catalog.length('%s'))"

I'm not sure why we ever thought that was a good idea, but it
definitely doesn't work anymore, since I removed the filtering
that _complete_from_query() used to do on the query results.
It's now incumbent on the queries to only return valid matches,
so I replaced all instances of this pattern with the regular
substring() checks, or even added a substring() check in a
couple of queries where there was nothing at all.

0001 takes care of quoting and case-folding issues for the actual
subject name of a completion operation, but there's more to do.
A lot of queries have to reference a previously-entered name
(for example, ALTER TABLE tab1 DROP COLUMN <TAB> has to find the
column names of table tab1), and we had variously shoddy code
for dealing with those names. Only a few queries even attempted
to handle schema-qualified names, and none at all of them would
downcase unquoted names. So 0002 tries to fix that up, using
the same code to parse/downcase/de-quote the name as we would
use if it were the subject text.

It didn't take long to find that the existing methods for this
were incredibly tedious, requiring near-duplicate queries
depending on whether the previous name was schema-qualified or
not. So I've extended the SchemaQuery mechanism to support
adding qualifications based on an additional name, and now
we use that wherever we need a possibly-schema-qualified
previous name.

A couple of the existing queries of this sort used WHERE oid
IN (sub-SELECT), which I didn't see a great way to jam into
the SchemaQuery mechanism. What I've done here is to convert
those semijoins into plain joins, which might yield multiple
instances of wanted names, and then stick DISTINCT onto the
queries. It's not very pretty, but it works fine.

In 0001 and 0002, I left the core of _complete_from_query()
un-reindented, in hopes of making the actual code changes
more readily reviewable. 0003 is just an application of pgindent
to fix that up and make the finished code legible again.

Finally, 0004 adds some test cases. I'm not too confident about
how portable these will be, but I don't think they are making any
assumptions the existing tests didn't make already. They do pass
for me on Linux (readline 7.0) and macOS (Apple's libedit).

This is sufficiently invasive to tab-complete.c that I'd like to
get it pushed fairly soon, before that code changes under me.
Thoughts?

regards, tom lane

Attachment Content-Type Size
v14-0001-rethink-tab-completion-quoting.patch text/x-diff 120.4 KB
v14-0002-fix-up-completion-info.patch text/x-diff 51.4 KB
v14-0003-re-pgindent.patch text/x-diff 9.0 KB
v14-0004-add-test-cases.patch text/x-diff 3.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nathan Bossart 2022-01-27 20:27:02 Re: Two noncritical bugs of pg_waldump
Previous Message Peter Geoghegan 2022-01-27 20:20:18 Why is INSERT-driven autovacuuming based on pg_class.reltuples?