Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Peter Eisentraut 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
On 2020-01-14 02:38, Tom Lane wrote: I wrote: Peter Eisentraut 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
I wrote:
> Peter Eisentraut 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
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---
+# PGAC_READLINE_VARIABLES
+# ---
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
-#ifdef HAVE_READLINE_READLINE_H
-# include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
#elif defined(HAVE_READLINE_H)
-# include
+#include
#endif
],
[rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index 25cfbcb..a46ba40 100755
--- a/configure
+++ b/configure
@@ -16316,10 +16316,12 @@ else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
#include
-#ifdef HAVE_READLINE_READLINE_H
-# include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
#elif defined(HAVE_READLINE_H)
-# include
+#include
#endif
int
@@ -16345,6 +16347,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" >&5
+$as_echo_n "checking for rl_filename_quote_characters... " >&6; }
+if ${pgac_cv_var_rl_filename_quote_characters+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Peter Eisentraut 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 > 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 > 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
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 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 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 -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
I wrote:
> Peter Eisentraut 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
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
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---
+# PGAC_READLINE_VARIABLES
+# ---
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
-#ifdef HAVE_READLINE_READLINE_H
-# include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
#elif defined(HAVE_READLINE_H)
-# include
+#include
#endif
],
[rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index d2d63f2..40fe9c1 100755
--- a/configure
+++ b/configure
@@ -16316,10 +16316,12 @@ else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
#include
-#ifdef HAVE_READLINE_READLINE_H
-# include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
#elif defined(HAVE_READLINE_H)
-# include
+#include
#endif
int
@@ -16345,6 +16347,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters"
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Peter Eisentraut 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 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Alvaro Herrera writes:
> One minor thing I noticed is that if I enter
> \copy t from '/tmp/t'
> 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
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---
+# PGAC_READLINE_VARIABLES
+# ---
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
-#ifdef HAVE_READLINE_READLINE_H
-# include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
#elif defined(HAVE_READLINE_H)
-# include
+#include
#endif
],
[rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index 9de5037..994e736 100755
--- a/configure
+++ b/configure
@@ -16314,10 +16314,12 @@ else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
#include
-#ifdef HAVE_READLINE_READLINE_H
-# include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
#elif defined(HAVE_READLINE_H)
-# include
+#include
#endif
int
@@ -16343,6 +16345,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" >&5
+$as_echo_n "checking for rl_filename_quote_characters... " >&6; }
+if ${pgac_cv_var_rl_filename_quote_characters+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+
+int
+main ()
+{
+rl_filename_quote_characters = "x";
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quote_characters=yes
+else
+ pgac_cv_var_rl_filename_quote_characters=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quote_characters" >&5
+$as_echo
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
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' 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
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
diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---
+# PGAC_READLINE_VARIABLES
+# ---
# Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
[AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
-#ifdef HAVE_READLINE_READLINE_H
-# include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
#elif defined(HAVE_READLINE_H)
-# include
+#include
#endif
],
[rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
[Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+ [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
diff --git a/configure b/configure
index 9de5037..994e736 100755
--- a/configure
+++ b/configure
@@ -16314,10 +16314,12 @@ else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
#include
-#ifdef HAVE_READLINE_READLINE_H
-# include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
#elif defined(HAVE_READLINE_H)
-# include
+#include
#endif
int
@@ -16343,6 +16345,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" >&5
+$as_echo_n "checking for rl_filename_quote_characters... " >&6; }
+if ${pgac_cv_var_rl_filename_quote_characters+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include
+#elif defined(HAVE_READLINE_H)
+#include
+#endif
+
+int
+main ()
+{
+rl_filename_quote_characters = "x";
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_var_rl_filename_quote_characters=yes
+else
+ pgac_cv_var_rl_filename_quote_characters=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename_quote_characters" >&5
+$as_echo "$pgac_cv_var_rl_filename_quote_characters" >&6; }
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+
+$as_echo "#define HAVE_RL_FILENAME_QUOTE_CHARACTERS 1" >>confdefs.h
+
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quoting_function" >&5
+$as_echo_n "checking for rl_filename_quoting_function... " >&6; }
+if ${pgac_cv_var_rl_filename_quoting_function+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include
+#if defined(HAVE_READLINE_READLINE_H)
+#include
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Alvaro Herrera 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 , 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 , 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 diff --git a/config/programs.m4 b/config/programs.m4 index 90ff944..68ab823 100644 --- a/config/programs.m4 +++ b/config/programs.m4 @@ -209,17 +209,20 @@ fi -# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER -# --- +# PGAC_READLINE_VARIABLES +# --- # Readline versions < 2.1 don't have rl_completion_append_character +# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function -AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER], +AC_DEFUN([PGAC_READLINE_VARIABLES], [AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character, [AC_LINK_IFELSE([AC_LANG_PROGRAM([#include -#ifdef HAVE_READLINE_READLINE_H -# include +#if defined(HAVE_READLINE_READLINE_H) +#include +#elif defined(HAVE_EDITLINE_READLINE_H) +#include #elif defined(HAVE_READLINE_H) -# include +#include #endif ], [rl_completion_append_character = 'x';])], @@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER], if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1, [Define to 1 if you have the global variable 'rl_completion_append_character'.]) -fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER +fi +AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters, +[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include +#if defined(HAVE_READLINE_READLINE_H) +#include +#elif defined(HAVE_EDITLINE_READLINE_H) +#include +#elif defined(HAVE_READLINE_H) +#include +#endif +], +[rl_filename_quote_characters = "x";])], +[pgac_cv_var_rl_filename_quote_characters=yes], +[pgac_cv_var_rl_filename_quote_characters=no])]) +if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then +AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1, + [Define to 1 if you have the global variable 'rl_filename_quote_characters'.]) +fi +AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function, +[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include +#if defined(HAVE_READLINE_READLINE_H) +#include +#elif defined(HAVE_EDITLINE_READLINE_H) +#include +#elif defined(HAVE_READLINE_H) +#include +#endif +], +[rl_filename_quoting_function = 0;])], +[pgac_cv_var_rl_filename_quoting_function=yes], +[pgac_cv_var_rl_filename_quoting_function=no])]) +if test
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
I wrote: > Robert Haas writes: >> On Wed, Dec 11, 2019 at 10:52 AM Tom Lane 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Alvaro Herrera 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
On 2019-Dec-13, Tom Lane wrote: > I wrote: > >> Alvaro Herrera 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
I wrote: >> Alvaro Herrera 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
I wrote: > Alvaro Herrera 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Alvaro Herrera 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 , 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Robert Haas writes: > On Wed, Dec 11, 2019 at 10:52 AM Tom Lane 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
On Wed, Dec 11, 2019 at 10:52 AM Tom Lane 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Alvaro Herrera 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
On 2019-Dec-11, Robert Haas wrote: > On Sun, Nov 3, 2019 at 5:40 PM Tom Lane 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
On Sun, Nov 3, 2019 at 5:40 PM Tom Lane 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
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
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 , 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
[ 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 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 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 diff --git a/config/programs.m4 b/config/programs.m4 index 90ff944..e5bf980 100644 --- a/config/programs.m4 +++ b/config/programs.m4 @@ -209,11 +209,12 @@ fi -# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER -# --- +# PGAC_READLINE_VARIABLES +#
