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

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.

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

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

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.

[ 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

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

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

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

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

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

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

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

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

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

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

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

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

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

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