Обсуждение: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

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

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
localfilesystem, 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
removinga 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
believedto 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
betweenthem.
 

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

diff --git a/src/bin/psql/stringutils.c b/src/bin/psql/stringutils.c
index 8c8b4c2..23a6f9f 100644
--- a/src/bin/psql/stringutils.c
+++ b/src/bin/psql/stringutils.c
@@ -282,6 +282,7 @@ strip_quotes(char *source, char quote, char escape, int encoding)
  * entails_quote -    any of these present?  need outer quotes
  * quote -            doubled within string, affixed to both ends
  * escape -            doubled within string
+ * force_quote -    if true, quote the output even if it doesn't "need" it
  * encoding -        the active character-set encoding
  *
  * Do not use this as a substitute for PQescapeStringConn().  Use it for
@@ -289,12 +290,13 @@ strip_quotes(char *source, char quote, char escape, int encoding)
  */
 char *
 quote_if_needed(const char *source, const char *entails_quote,
-                char quote, char escape, int encoding)
+                char quote, char escape, bool force_quote,
+                int encoding)
 {
     const char *src;
     char       *ret;
     char       *dst;
-    bool        need_quotes = false;
+    bool        need_quotes = force_quote;

     Assert(source != NULL);
     Assert(quote != '\0');
diff --git a/src/bin/psql/stringutils.h b/src/bin/psql/stringutils.h
index 16a7af0..fa00141 100644
--- a/src/bin/psql/stringutils.h
+++ b/src/bin/psql/stringutils.h
@@ -22,6 +22,7 @@ extern char *strtokx(const char *s,
 extern void strip_quotes(char *source, char quote, char escape, int encoding);

 extern char *quote_if_needed(const char *source, const char *entails_quote,
-                             char quote, char escape, int encoding);
+                             char quote, char escape, bool force_quote,
+                             int encoding);

 #endif                            /* STRINGUTILS_H */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 2b1e3cd..3cc1ae4 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4345,6 +4345,12 @@ complete_from_variables(const char *text, const char *prefix, const char *suffix
  * This function wraps rl_filename_completion_function() to strip quotes from
  * the input before searching for matches and to quote any matches for which
  * the consuming command will require it.
+ *
+ * Caller must set completion_charp to a zero- or one-character string
+ * containing the escape character.  This is necessary since \copy has no
+ * escape character, but every other backslash command recognizes "\" as an
+ * escape character.  Since we have only two callers, don't bother providing
+ * a macro to simplify this.
  */
 static char *
 complete_from_files(const char *text, int state)
@@ -4370,14 +4376,13 @@ complete_from_files(const char *text, int state)
     if (unquoted_match)
     {
         /*
-         * Caller sets completion_charp to a zero- or one-character string
-         * containing the escape character.  This is necessary since \copy has
-         * no escape character, but every other backslash command recognizes
-         * "\" as an escape character.  Since we have only two callers, don't
-         * bother providing a macro to simplify this.
+         * Pass the escape char if any.  Also, if original input started with
+         * "'", force the output to have that too.
          */
         ret = quote_if_needed(unquoted_match, " \t\r\n\"`",
-                              '\'', *completion_charp, pset.encoding);
+                              '\'', *completion_charp,
+                              (*text == '\''),
+                              pset.encoding);
         if (ret)
             free(unquoted_match);
         else

>> 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
turninga valid (partial) invocation into an invalid one. 

Best,
Steve.