Horton

Thread

Commits

  1. Improve psql's tab completion for filenames.

  2. Modernize our readline API a tad.

  3. Add tab completion for EXPLAIN (SETTINGS) in psql

  1. BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    The Post Office <noreply@postgresql.org> — 2019-10-15T13:11:29Z

    The following bug has been logged on the website:

Bug reference:      16059
Logged by:          Steven Winfield
Email address:      steven.winfield@cantabcapital.com
PostgreSQL version: 11.5
Operating system:   Linux
Description:        

As per the documentation[1], the COPY command requires the output filename
to be single-quoted.

However, when using psql, a partial COPY command such as this...
COPY pg_catalog.pg_class TO '/usr
...will, on hitting TAB, be converted to this...
COPY pg_catalog.pg_class TO /usr/
...requiring the user to move the cursor back to re-insert the single quote
before finishing the command and executing.

The issue seems to be somewhere around here[2], where complete_from_files[3]
is used to suggest replacements - that function strips quotes from the
existing (partial) filename but doesn't put them back unless quote_if_needed
is true (which I guess it isn't, unless there is a space in the filename for
example).

Note that using the \copy command instead works fine, as filenames do not
need to be quoted in that case.

[1] https://www.postgresql.org/docs/11/sql-copy.html
[2]
https://github.com/postgres/postgres/blob/4b011cad272e997935eb8d80ab741a40b395fdf5/src/bin/psql/tab-complete.c#L2234
[3]
https://github.com/postgres/postgres/blob/4b011cad272e997935eb8d80ab741a40b395fdf5/src/bin/psql/tab-complete.c#L4350

  2. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Francisco Olarte <folarte@peoplecall.com> — 2019-10-15T13:38:15Z

    Steven:

On Tue, Oct 15, 2019 at 3:12 PM PG Bug reporting form
<noreply@postgresql.org> wrote:
> As per the documentation[1], the COPY command requires the output filename
> to be single-quoted.
> However, when using psql, a partial COPY command such as this...
> COPY pg_catalog.pg_class TO '/usr
> ...will, on hitting TAB, be converted to this...
> COPY pg_catalog.pg_class TO /usr/
> ...requiring the user to move the cursor back to re-insert the single quote
> before finishing the command and executing.

> The issue seems to be somewhere around here[2], where complete_from_files[3]
> is used to suggest replacements - that function strips quotes from the
> existing (partial) filename but doesn't put them back unless quote_if_needed
> is true (which I guess it isn't, unless there is a space in the filename for
> example).

Not saying it's not a bug, but bear in mind psql CAN NOT correctly
complete filenames for SERVER SIDE copy. You may be running in the
same machine, but even with this and using unix domain sockets it's
difficult to know what is at the other end of the socket ( not sure if
you can always know it even if you are root, and you can have things
like psql connecting through unix domain socket to pgbouncer which
forwards to I-do-not-know-where (.com) .

> Note that using the \copy command instead works fine, as filenames do not
> need to be quoted in that case.

They are different beasts, in \copy you are not completing an sql
command to send to the server, you are completing a command to psql (
which it implemts using an sql command plus some magic ).

Francisco Olarte.

  3. RE: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Steven Winfield <steven.winfield@cantabcapital.com> — 2019-10-15T13:52:52Z

    > Not saying it's not a bug, but bear in mind psql CAN NOT correctly
> complete filenames for SERVER SIDE copy. You may be running in the same
> machine, but even with this and using unix domain sockets it's difficult
> to know what is at the other end of the socket ( not sure if you can
> always know it even if you are root, and you can have things like psql
> connecting through unix domain socket to pgbouncer which forwards to I-do-
> not-know-where (.com) .

That's very true, but at some point the decision was made to tab-complete COPY commands using information from the local filesystem, since that might be useful.
I doubt there was ever an intention to take an otherwise-well-formed (partial) COPY command and make it invalid by removing a single quote in the middle of it!

> They are different beasts, in \copy you are not completing an sql command
> to send to the server, you are completing a command to psql ( which it
> implemts using an sql command plus some magic ).

Yep, I'm aware of that - I'm just pointing out the difference in syntax between the two commands, which I had always believed to be near-drop-in replacements for each other syntax-wise. 
It's also relevant because the same tab-completion code is used for both \copy and COPY and currently can't distinguish between them.

Perhaps complete_from_files() needs an extra argument to specify the quoting behaviour.

Steven

  4. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-02T21:14:20Z

    PG Bug reporting form <noreply@postgresql.org> writes:
> As per the documentation[1], the COPY command requires the output filename
> to be single-quoted.

> However, when using psql, a partial COPY command such as this...
> COPY pg_catalog.pg_class TO '/usr
> ...will, on hitting TAB, be converted to this...
> COPY pg_catalog.pg_class TO /usr/
> ...requiring the user to move the cursor back to re-insert the single quote
> before finishing the command and executing.

> The issue seems to be somewhere around here[2], where complete_from_files[3]
> is used to suggest replacements - that function strips quotes from the
> existing (partial) filename but doesn't put them back unless quote_if_needed
> is true (which I guess it isn't, unless there is a space in the filename for
> example).

> Note that using the \copy command instead works fine, as filenames do not
> need to be quoted in that case.

Yeah, it seems like a bad idea to override the user's choice to write
a quote, even if one is not formally necessary.  I propose the attached.

			regards, tom lane

  5. RE: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Steven Winfield <steven.winfield@cantabcapital.com> — 2019-11-02T22:00:30Z

    >> As per the documentation[1], the COPY command requires the output filename
>> to be single-quoted.
...
>> Note that using the \copy command instead works fine, as filenames do not
>> need to be quoted in that case.

>Yeah, it seems like a bad idea to override the user's choice to write
>a quote, even if one is not formally necessary.  I propose the attached.

Thanks for taking a look at this. It will save me (and I hope many others) from much frustration!

But, to be clear, for the COPY command the single quotes *are* formally necessary, so at the moment tab-completion is turning a valid (partial) invocation into an invalid one.

Best,
Steve.

  6. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-03T22:40:00Z

    [ redirecting to -hackers ]

I wrote:
> Yeah, it seems like a bad idea to override the user's choice to write
> a quote, even if one is not formally necessary.  I propose the attached.

After further experimentation, I concluded that that patch is a bad idea;
it breaks a lot of cases that used to work before.  It turns out that
Readline has a bunch of behaviors for filename completion that occur
outside of the rl_filename_completion_function function proper, and they
all assume that what's passed back from that function is plain unquoted
filename(s).  Notably, completion of a path that includes directory names
just doesn't work well at all anymore with that patch ... nor did it
work well before, if the path contained characters that we thought we
should quote.

The right way to do things, seemingly, is to let
rl_filename_completion_function be invoked without any interference,
and instead put our SQL-aware quoting/dequoting logic into the hooks
Readline provides for that purpose, rl_filename_quoting_function and
rl_filename_dequoting_function.  (It appears that somebody tried to do
that before, way back around the turn of the century, but gave up on it.
Too bad, because it's the right thing.)

Of course, this only works if we have those hooks :-(.  So far as
I can tell, all libreadline variants that might still exist in the wild
do have them; but libedit doesn't, at least not in the version Apple
is shipping.  Hence, what the attached patch does is to make configure
probe for the existence of the hook variables; if they're not there,
fall back to what I proposed previously.  The behavior on libedit is
a bit less nice than one could wish, but it's better than what we have
now.

I've tested this on the oldest and newest readline versions I have at
hand (4.2a and 8.0), as well as the oldest and newest versions of
Apple's libedit derivative; but I haven't tried it on whatever the
BSDen are shipping as libedit.

There's enough moving parts here that this probably needs to go through
a full review cycle, so I'll add it to the next commitfest.  Some notes
for anybody wanting to review:

* The patch now always quotes completed filenames, so quote_if_needed()
is misnamed and overcomplicated for this use-case.  I left the extra
generality in place for possible future use.  On the other hand, this
is the *only* use-case, so you could also argue that we should just
simplify the function's API.  I have no strong opinion about that.

* In addition to the directly-related-to-filenames changes, it turns out
to be necessary to set rl_completer_quote_characters to include at least
single quotes, else Readline doesn't understand that a quoted filename
is quoted.  The patch sets it to include double quotes as well.  This
is probably the aspect of the patch that most needs testing.  The general
effect of this change is that Readline now understands that quoted
strings are single entities, plus it will try to complete the contents
of a string if you ask it.  The side-effects I've noticed seem to be
all beneficial -- for example, if you do

	select * from "foo<TAB>

it now correctly completes table names starting with "foo", which it
did not before.  But there might be some bad effects as well.  Also,
although libedit has this variable, setting it doesn't have that effect
there; I've not really found that the variable does anything at all there.

* The behavior of quote_file_name is directly modeled on what Readline's
default implementation rl_quote_filename does, except that it uses
SQL-aware quoting rules.  The business of passing back the final quote
mark separately is their idea.

* An example of the kind of case that's interesting is that if you type

\lo_import /usr/i<TAB>

then what you get on readline (with this patch) is

\lo_import '/usr/include/

while libedit produces

\lo_import '/usr/include'     (with a space after the trailing quote)

That is, readline knows that the completion-so-far is a directory and
assumes that's not all you want, whereas libedit doesn't know that.
So you typically now have to back up two characters, type slash, and
resume completing.  That's kind of a pain, but I'm not sure we can
make it better very easily.  Anyway, libedit did something close to
that before, too.

* There are also some interesting cases around special characters in
the filename.  It seems to work well for embedded spaces, not so well
for embedded single quotes, though that may well vary across readline
versions.  Again, there seems to be a limited amount we can do about
that, given how much of the relevant logic is buried where we can't
modify it.  And I'm not sure how much I care about that case, anyway.

			regards, tom lane

  7. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-11T01:55:52Z

    All in all, after testing this for a bit, I think this patch is a clear
improvement over the statu quo.  Thanks for working on this.

I suggest to indicate in complete_from_files where to find the hook
functions it refers to (say "see quote_file_name, below", or something.)

I tested this on libreadline 7.x (where #define
HAVE_RL_FILENAME_COMPLETION_FUNCTION 1).  I noticed that if I enter a
filename that doesn't exist and then <tab>, it adds a closing quote.
Bash manages to do nothing somehow, which is the desired behavior IMO.

(I tried to make sense of these hooks, but couldn't readily and I don't
have the readline documentation installed, so I have no opinion on
whether this problem is fixable.  Maybe the trick is to let
quote_if_needed know that this is a completion for a filename, and have
it test for file existence?)

Also, some commands such as \cd want a directory rather than just any
file.  Not sure rl_filename_completion_function has a way to pass this
down.  (This point is a bit outside this patch's charter perhaps, but
may as well think about it since we're here ...)

I don't quite understand why a readline library that doesn't have
rl_filename_completion_function is known to have a
filename_completion_function, ie. this bit 

#ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
#define filename_completion_function rl_filename_completion_function
#else
/* decl missing in some header files, but function exists anyway */
extern char *filename_completion_function();
#endif

What's going on here?  How does this ever work?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Robert Haas <robertmhaas@gmail.com> — 2019-12-11T14:17:35Z

    On Sun, Nov 3, 2019 at 5:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Of course, this only works if we have those hooks :-(.  So far as
> I can tell, all libreadline variants that might still exist in the wild
> do have them; but libedit doesn't, at least not in the version Apple
> is shipping.  Hence, what the attached patch does is to make configure
> probe for the existence of the hook variables; if they're not there,
> fall back to what I proposed previously.

Are people still compiling against libedit and then redirecting to
libreadline at runtime? I seem to recall some discussion about this
being a thing, many years ago. If it were being done it would be
advantageous to have the checks be runtime rather than compile-time,
although I guess that would probably be tough to make work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-11T15:06:37Z

    On 2019-Dec-11, Robert Haas wrote:

> On Sun, Nov 3, 2019 at 5:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Of course, this only works if we have those hooks :-(.  So far as
> > I can tell, all libreadline variants that might still exist in the wild
> > do have them; but libedit doesn't, at least not in the version Apple
> > is shipping.  Hence, what the attached patch does is to make configure
> > probe for the existence of the hook variables; if they're not there,
> > fall back to what I proposed previously.
> 
> Are people still compiling against libedit and then redirecting to
> libreadline at runtime? I seem to recall some discussion about this
> being a thing, many years ago.

Yeah, Debian did that out of licensing concerns.  It seems they still do
that, based on
https://packages.debian.org/bullseye/postgresql-client-12

> If it were being done it would be
> advantageous to have the checks be runtime rather than compile-time,
> although I guess that would probably be tough to make work.

Yeah.  On the other hand, I suppose Debian uses the BSD version of the
libraries, not the Apple version, so I think it should be fine?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  10. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-11T15:18:35Z

    On 2019-Dec-11, Alvaro Herrera wrote:

> On 2019-Dec-11, Robert Haas wrote:

> > If it were being done it would be
> > advantageous to have the checks be runtime rather than compile-time,
> > although I guess that would probably be tough to make work.
> 
> Yeah.  On the other hand, I suppose Debian uses the BSD version of the
> libraries, not the Apple version, so I think it should be fine?

... actually, grepping libedit's source at
http://deb.debian.org/debian/pool/main/libe/libedit/libedit_3.1-20191025.orig.tar.gz
there's no occurrence of rl_filename_quoting_function.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-11T15:52:30Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Dec-11, Robert Haas wrote:
>> Are people still compiling against libedit and then redirecting to
>> libreadline at runtime? I seem to recall some discussion about this
>> being a thing, many years ago.

> Yeah, Debian did that out of licensing concerns.  It seems they still do
> that, based on
> https://packages.debian.org/bullseye/postgresql-client-12

I think it's Debian's problem, not ours, if that doesn't work.  It is
not unreasonable for a package to probe existence of a library function
at configure time.  It's up to them to make sure that the headers match
the actual library.

			regards, tom lane

  12. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Robert Haas <robertmhaas@gmail.com> — 2019-12-11T15:57:30Z

    On Wed, Dec 11, 2019 at 10:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think it's Debian's problem, not ours, if that doesn't work.  It is
> not unreasonable for a package to probe existence of a library function
> at configure time.  It's up to them to make sure that the headers match
> the actual library.

That seems like an unhelpful attitude. Debian is a mainstream
platform, and no doubt feels that they have important reasons for what
they are doing.

That's not to say that I'm against the patch, but I don't believe it's
right to treat the concerns of mainstream Linux distributions in
anything less than a serious manner.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  13. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-11T16:34:51Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Dec 11, 2019 at 10:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think it's Debian's problem, not ours, if that doesn't work.  It is
>> not unreasonable for a package to probe existence of a library function
>> at configure time.  It's up to them to make sure that the headers match
>> the actual library.

> That seems like an unhelpful attitude. Debian is a mainstream
> platform, and no doubt feels that they have important reasons for what
> they are doing.

Nonetheless, if they're doing that, it's *their* bug not ours when
the run-time library fails to match what was supplied to compile
against.  I think it would fall to them to patch either libedit
or readline to make those two agree.  This is not different in any
way from the expectation that a platform supply a libc whose ABI
is stable.

In any case, this discussion is a bit hypothetical isn't it?
If I understand correctly, your concern is that the proposed
patch might fail to take advantage of functionality that actually
might be present at runtime.  So what?  It's no worse than before.
More, it's likely that there are other similar losses of functionality
already in our code and/or other peoples'.  Debian bought into that
tradeoff, not us.

			regards, tom lane

  14. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-11T16:43:23Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I tested this on libreadline 7.x (where #define
> HAVE_RL_FILENAME_COMPLETION_FUNCTION 1).  I noticed that if I enter a
> filename that doesn't exist and then <tab>, it adds a closing quote.
> Bash manages to do nothing somehow, which is the desired behavior IMO.

Hmm.  I'll take a look, but I'm not terribly hopeful.  I have looked
briefly at what Bash does for filename completion, and as I recall
it was massive, spaghetti-ish, and way too much in bed with various
implementation details of libreadline --- they don't pretend to work
with libedit.  I'm not prepared to go there.  It's reasonable for Bash
to expend huge effort on filename completion, because that's such a core
use-case for them, but I don't think it deserves as much work in psql.

> I don't quite understand why a readline library that doesn't have
> rl_filename_completion_function is known to have a
> filename_completion_function, ie. this bit 

> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
> #define filename_completion_function rl_filename_completion_function
> #else
> /* decl missing in some header files, but function exists anyway */
> extern char *filename_completion_function();
> #endif

I think the point is that before rl_filename_completion_function the
function existed but was just called filename_completion_function.
It's possible that that's obsolete --- I've not really checked.

			regards, tom lane

  15. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-12T05:49:50Z

    I wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> I don't quite understand why a readline library that doesn't have
>> rl_filename_completion_function is known to have a
>> filename_completion_function, ie. this bit 

>> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
>> #define filename_completion_function rl_filename_completion_function
>> #else
>> /* decl missing in some header files, but function exists anyway */
>> extern char *filename_completion_function();
>> #endif

> I think the point is that before rl_filename_completion_function the
> function existed but was just called filename_completion_function.
> It's possible that that's obsolete --- I've not really checked.

I had a look through the buildfarm results, and it seems that the only
(non-Windows) animals that don't HAVE_RL_FILENAME_COMPLETION_FUNCTION
are prairiedog and locust.  prairiedog is using the libedit that
Apple supplied in its stone-age version of macOS, and I imagine the
same can be said of locust, though that's one macOS release newer.

prairiedog's version does define filename_completion_function:

$ grep completion_func /usr/include/readline/readline.h 
extern CPPFunction      *rl_attempted_completion_function;
char            *filename_completion_function(const char *, int);
char            *username_completion_function(const char *, int);

so the assumption embodied in our code is both correct and necessary
so far as the current universe of buildfarm critters is concerned.

Having said that, prairiedog's version of libedit is buggy as hell;
it generates bogus warnings at every psql exit, for instance.

$ psql postgres
psql (13devel)
Type "help" for help.

postgres=# \q
could not save history to file "/Users/tgl/.psql_history": operating system error 0
$ 

It wouldn't be an unreasonable idea to desupport this version,
if it allowed additional simplifications in psql beside this
particular #ifdef mess.  I'm not sure whether any of the
contortions in e.g. saveHistory could go away if we did so.

			regards, tom lane

  16. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-13T14:42:25Z

    I wrote:
>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>> I don't quite understand why a readline library that doesn't have
>>> rl_filename_completion_function is known to have a
>>> filename_completion_function, ie. this bit 

>>> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
>>> #define filename_completion_function rl_filename_completion_function
>>> #else
>>> /* decl missing in some header files, but function exists anyway */
>>> extern char *filename_completion_function();
>>> #endif

>> I think the point is that before rl_filename_completion_function the
>> function existed but was just called filename_completion_function.
>> It's possible that that's obsolete --- I've not really checked.

Looking closer at this, the "extern" could be got rid of, I think.
prairiedog's readline header does have that extern, so it's hard to
believe anybody is still using libedit versions that don't declare it.

A possible further change is to switch the code over to calling
"rl_filename_completion_function", and then invert the sense of
this logic, like

/*
 * Ancient versions of libedit provide filename_completion_function()
 * instead of rl_filename_completion_function().
 */
#ifndef HAVE_RL_FILENAME_COMPLETION_FUNCTION
#define rl_filename_completion_function filename_completion_function
#endif

This would make it easier to compare our code to the readline
documentation, so maybe it's helpful ... or maybe it's just
churn.  Thoughts?

			regards, tom lane

  17. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-13T15:38:05Z

    On 2019-Dec-13, Tom Lane wrote:

> I wrote:
> >> Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> >>> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
> >>> #define filename_completion_function rl_filename_completion_function
> >>> #else
> >>> /* decl missing in some header files, but function exists anyway */
> >>> extern char *filename_completion_function();
> >>> #endif

> Looking closer at this, the "extern" could be got rid of, I think.
> prairiedog's readline header does have that extern, so it's hard to
> believe anybody is still using libedit versions that don't declare it.

Agreed.

> A possible further change is to switch the code over to calling
> "rl_filename_completion_function", and then invert the sense of
> this logic, like
> 
> /*
>  * Ancient versions of libedit provide filename_completion_function()
>  * instead of rl_filename_completion_function().
>  */
> #ifndef HAVE_RL_FILENAME_COMPLETION_FUNCTION
> #define rl_filename_completion_function filename_completion_function
> #endif
> 
> This would make it easier to compare our code to the readline
> documentation, so maybe it's helpful ... or maybe it's just
> churn.  Thoughts?

+1, I think that's clearer.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  18. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-13T16:18:09Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Dec-13, Tom Lane wrote:
>> A possible further change is to switch the code over to calling
>> "rl_filename_completion_function", and then invert the sense of
>> this logic, like ...

> +1, I think that's clearer.

OK, I went ahead and pushed that change, since it seems separate
and uncontroversial.  I'll send along a new patch for the main
change in a little bit.

			regards, tom lane

  19. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-13T18:55:07Z

    I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Dec 11, 2019 at 10:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think it's Debian's problem, not ours, if that doesn't work.  It is
>>> not unreasonable for a package to probe existence of a library function
>>> at configure time.  It's up to them to make sure that the headers match
>>> the actual library.

>> That seems like an unhelpful attitude. Debian is a mainstream
>> platform, and no doubt feels that they have important reasons for what
>> they are doing.

Actually, this argument is based on a false premise anyhow.  I took
a look into Debian's source package, and AFAICS they are not doing
anything as weird as a run-time substitution.  They are just filling
the build environment with libedit-dev not libreadline-dev.  So that
is certainly a supported configuration from our side, and if there
is any header-to-library discrepancy then it's just a simple bug
in the libedit package.

(Maybe at one time they were doing something weird; I didn't look
back further than the current sources for PG 12.)

			regards, tom lane

  20. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-13T19:16:41Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I suggest to indicate in complete_from_files where to find the hook
> functions it refers to (say "see quote_file_name, below", or something.)

Done.

> I tested this on libreadline 7.x (where #define
> HAVE_RL_FILENAME_COMPLETION_FUNCTION 1).  I noticed that if I enter a
> filename that doesn't exist and then <tab>, it adds a closing quote.
> Bash manages to do nothing somehow, which is the desired behavior IMO.

Fixed --- on looking closer, I'd drawn the wrong conclusions from
looking at readline's default implementation of the quoting function
(which seems to be a tad broken, at least in the version I looked at).
It turns out that there are some special cases we need to handle if
we want it to behave nicely.

> Also, some commands such as \cd want a directory rather than just any
> file.  Not sure rl_filename_completion_function has a way to pass this
> down.  (This point is a bit outside this patch's charter perhaps, but
> may as well think about it since we're here ...)

I ended up adding an S_ISDIR stat check in the completion function,
because the desired behavior of terminating a directory name with '/'
(and no quote) doesn't seem to be possible to get otherwise.  So it would
be possible to do something different for \cd, but I am not clear that
there's any real advantage.  You can't really guess if the user wants the
currently completable directory or a subdirectory, so it wouldn't do to
emit a closing quote.

I've now spent some effort on hacking the libedit code path (i.e. the
one where we don't have the hooks) as well as the libreadline path.
This version of the patch seems to behave well on all the following:
* readline 6.0 (RHEL 6)
* readline 8.0 (Fedora 30)
* libedit 3.1 (Debian stretch)
* whatever libedit Apple is shipping in current macOS

I also tried it on ancient libedits from prairiedog and some other
old macOS releases.  There are cosmetic issues there (e.g. prairiedog
wants to double the slash after a directory name) but I doubt we care
enough to fix them.  It does compile and more-or-less work.

I noticed along the way that configure's probe for
rl_completion_append_character fails if we're using <editline/readline.h>,
because that configure macro was never taught to honor
HAVE_EDITLINE_READLINE_H.  This might account for weird behavior on
libedit builds, perhaps.  Arguably that could be a back-patchable bug fix,
but I'm disinclined to do so because it might break peoples' muscle memory
about whether a space needs to be typed after a completion; not a great
idea in a minor release.

			regards, tom lane

  21. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-27T19:27:17Z

    I wrote:
> [ psql-filename-completion-fixes-2.patch ]

The cfbot noted this was broken by the removal of pg_config.h.win32,
so here's a new version rebased over that.  No changes other than
adjusting the MSVC autoconf-substitute code.

			regards, tom lane

  22. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-27T20:03:33Z

    On 2019-Dec-27, Tom Lane wrote:

> I wrote:
> > [ psql-filename-completion-fixes-2.patch ]
> 
> The cfbot noted this was broken by the removal of pg_config.h.win32,
> so here's a new version rebased over that.  No changes other than
> adjusting the MSVC autoconf-substitute code.

Works well for me.

One minor thing I noticed is that if I enter
\copy t from '/tmp/t'<tab>
and I have files /tmp/t and /tmp/tst then it removes the ending quote.
I can live with that, since the tab there is pointless anyway.
(The unpatched version removes *both* quotes.  The quotes there are not
mandatory, but the new version works better when there are files with
whitespace in the name.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  23. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-27T22:36:56Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> One minor thing I noticed is that if I enter
> \copy t from '/tmp/t'<tab>
> and I have files /tmp/t and /tmp/tst then it removes the ending quote.

Yeah, that bothered me too.  [ pokes at it for a bit... ]  The
quote_file_name function isn't passed enough info to handle this
in a clean way, but we can make it work if we don't mind introducing
some more coupling with psql_completion, ie having the latter remember
whether the raw input word ended with a quote.  As per v4.

> (The unpatched version removes *both* quotes.  The quotes there are not
> mandatory, but the new version works better when there are files with
> whitespace in the name.)

Or when you're completing in a COPY rather than \copy --- then, removing
the quotes is broken whether there's whitespace or not.

			regards, tom lane

  24. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-01-03T07:54:35Z

    On 2019-11-03 23:40, Tom Lane wrote:
> * The patch now always quotes completed filenames, so quote_if_needed()
> is misnamed and overcomplicated for this use-case.  I left the extra
> generality in place for possible future use.  On the other hand, this
> is the*only*  use-case, so you could also argue that we should just
> simplify the function's API.  I have no strong opinion about that.

I haven't found an explanation in this thread why it does always quote 
now.  That seems a bit unusual.  Is there a reason for this?  Can we do 
without it?

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

  25. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-03T13:37:37Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-11-03 23:40, Tom Lane wrote:
>> * The patch now always quotes completed filenames, so quote_if_needed()
>> is misnamed and overcomplicated for this use-case.  I left the extra
>> generality in place for possible future use.  On the other hand, this
>> is the*only*  use-case, so you could also argue that we should just
>> simplify the function's API.  I have no strong opinion about that.

> I haven't found an explanation in this thread why it does always quote 
> now.  That seems a bit unusual.  Is there a reason for this?  Can we do 
> without it?

The core problem we're trying to solve is stated in the thread title:
if you do

prompt# copy mytab from 'myfil<TAB>

then (assuming some completion is available) the current code actually
*removes* the quote, which is completely wrong.  Even if the user
didn't type a leading quote, it's better to add one, because COPY
won't work otherwise.

It'd be possible, perhaps, to distinguish between this case and the
cases in backslash commands, which are okay with omitted quotes
(for some filenames).  I'm not sure that that would be an improvement
though.

			regards, tom lane

  26. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-06T06:06:28Z

    I wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> I haven't found an explanation in this thread why it does always quote 
>> now.  That seems a bit unusual.  Is there a reason for this?  Can we do 
>> without it?

> The core problem we're trying to solve is stated in the thread title:
> ...
> It'd be possible, perhaps, to distinguish between this case and the
> cases in backslash commands, which are okay with omitted quotes
> (for some filenames).  I'm not sure that that would be an improvement
> though.

I realized that there *is* a good reason to worry about this.  Fairly
recent versions of libedit will try to backslash-escape the output
of psql_completion(), as I described over at [1].  This is absolutely
disastrous if we've emitted a quoted filename: we end up going from,
say,
	\lo_import myf<TAB>
to
	\lo_import \'myfile\'
which of course doesn't work at all (psql thinks \' is in itself a
backslash command).

There isn't much we can do about this in contexts where we must
quote the filename: not doing so produces an equally broken command.
However, this problem does suggest that we shouldn't force quoting
if we don't have to, as that just breaks cases we needn't break.

Hence, the attached revision only forces quoting in a SQL COPY
command, or if the user already typed a quote.

I also added some regression tests (whee!).  They pass for me with a
couple different readline and libedit versions, but I have only minimal
hopes for them passing everywhere without further hacking ... the
results of the other thread suggest that pain is to be expected here.

			regards, tom lane

[1] https://www.postgresql.org/message-id/13708.1578059577%40sss.pgh.pa.us

  27. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-01-13T16:34:37Z

    On 2020-01-06 07:06, Tom Lane wrote:
> Hence, the attached revision only forces quoting in a SQL COPY
> command, or if the user already typed a quote.

Yes, that seems better.  Users tend to not like if tab completion messes 
with what they have already typed unless strictly necessary.

The file name completion portion of this patch seems to work quite well now.

I have found a weird behavior with identifier quoting, which is not the 
subject of this patch, but it appears to be affected by it.

The good thing is that the new code will behave sensibly with

select * from "pg_cl<TAB>

which the old code didn't offer anything on.

The problem is that if you have

create table "test""1" (a int);

then the new code responds to

select * from "te<TAB>

by making that

select * from "te"

whereas the old code curiously handled that perfectly.

Neither the old nor the new code will produce anything from

select * from te<TAB>

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

  28. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-13T20:36:34Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> The file name completion portion of this patch seems to work quite well now.

Thanks for testing!

> I have found a weird behavior with identifier quoting, which is not the 
> subject of this patch, but it appears to be affected by it.

> The good thing is that the new code will behave sensibly with
> select * from "pg_cl<TAB>
> which the old code didn't offer anything on.

Check.

> The problem is that if you have
> create table "test""1" (a int);
> then the new code responds to
> select * from "te<TAB>
> by making that
> select * from "te"
> whereas the old code curiously handled that perfectly.

Right.  The underlying cause of both these changes seems to be that
what readline passes to psql_completion is just the contents of the
string, now that we've told it that double-quote is a string quoting
character.  So that works fine with 'pg_cl' which didn't need quoting
anyway.  In your second example, because we generate possible matches
that are already quoted-if-necessary, we fail to find anything that
bare 'te' is a prefix of, where before we were considering '"te' and
it worked.

I'll think about how to improve this.  Given that we're dequoting
filenames explicitly anyway, maybe we don't need to tell readline that
double-quote is a quoting character.  Another idea is that maybe
we ought to refactor things so that identifier dequoting and requoting
is handled explicitly, similarly to the way filenames work now.
That would make the patch a lot bigger though.

			regards, tom lane

  29. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-14T01:38:17Z

    I wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> I have found a weird behavior with identifier quoting, which is not the 
>> subject of this patch, but it appears to be affected by it.

> I'll think about how to improve this.  Given that we're dequoting
> filenames explicitly anyway, maybe we don't need to tell readline that
> double-quote is a quoting character.  Another idea is that maybe
> we ought to refactor things so that identifier dequoting and requoting
> is handled explicitly, similarly to the way filenames work now.
> That would make the patch a lot bigger though.

On reflection, it seems like the best bet for the moment is to
remove double-quote from the rl_completer_quote_characters string,
which should restore all behavior around double-quoted strings to
what it was before.  (We have to keep single-quote in that string,
though, or quoted file names fail entirely.)

The only impact this has on filename completion is that we're no
longer bright enough to convert a double-quoted filename to
single-quoted for you, which would be a nice-to-have but it's
hardly core functionality.

At some point it'd be good to undo that and upgrade quoted-identifier
processing, but I don't really want to include such changes in this
patch.  I'm about burned out on tab completion for right now.

			regards, tom lane

  30. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-01-23T13:46:08Z

    On 2020-01-14 02:38, Tom Lane wrote:
> I wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>> I have found a weird behavior with identifier quoting, which is not the
>>> subject of this patch, but it appears to be affected by it.
> 
>> I'll think about how to improve this.  Given that we're dequoting
>> filenames explicitly anyway, maybe we don't need to tell readline that
>> double-quote is a quoting character.  Another idea is that maybe
>> we ought to refactor things so that identifier dequoting and requoting
>> is handled explicitly, similarly to the way filenames work now.
>> That would make the patch a lot bigger though.
> 
> On reflection, it seems like the best bet for the moment is to
> remove double-quote from the rl_completer_quote_characters string,
> which should restore all behavior around double-quoted strings to
> what it was before.  (We have to keep single-quote in that string,
> though, or quoted file names fail entirely.)

This patch (version 6) looks good to me.

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

  31. Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-23T16:09:36Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-01-14 02:38, Tom Lane wrote:
>> On reflection, it seems like the best bet for the moment is to
>> remove double-quote from the rl_completer_quote_characters string,
>> which should restore all behavior around double-quoted strings to
>> what it was before.  (We have to keep single-quote in that string,
>> though, or quoted file names fail entirely.)

> This patch (version 6) looks good to me.

Thanks for reviewing!  Pushed, now we'll see what the buildfarm thinks...

			regards, tom lane