TAP testing for psql's tab completion code

We've often talked about the problem that we have no regression test
coverage for psql's tab completion code.  I got interested in this
issue while messing with the filename completion logic therein [1],
so here is a draft patch that adds some testing for that code.

This is just preliminary investigation, so I've made no attempt
to test tab-complete.c comprehensively (and I'm not sure there
would be much point in covering every last code path in it anyway).
Still, it does get us from zero to 43% coverage of that file
already, and it does good things for the coverage of input.c
and prompt.c as well.

What I think is actually interesting at this stage is portability
of the tests.  There are a number of issues:

* The script requires Perl's IO::Pty module (indirectly, in that IPC::Run
requires that to make pty connections), which isn't installed everywhere.
I just made the script skip if that's not available, so that we're not
moving the goalposts for what has to be installed to run the TAP tests.
Is that the right answer?

* It seems pretty likely that this won't work on Windows, given all the
caveats in the IPC::Run documentation about nonfunctionality of the pty
support there.  Maybe we don't care, seeing that we don't really support
readline on Windows anyway.  For the moment I assumed that the skip
conditions for --without-readline and/or missing-IO::Pty would cover
this, but we might need an explicit check for Windows too.  Or maybe
somebody wants to try to make it work on Windows; but that won't be me.

* What's even more likely to be problematic is small variations in the
output behavior of different readline and libedit versions.  According
to my tests so far, though, all modern versions of them do pass these
test cases.  I noted failures on very old Apple versions of libedit:

1. macOS 10.5's version of libedit seems not to honor
rl_completion_append_character; it never emits the trailing space
we're expecting.  This seems like a plain old libedit bug, especially
since 10.4's version works as expected.

2. Both 10.4 and 10.5 emit the alternative table names in the wrong
order, suggesting that they're not internally sorting the completion
results.  If this proves to be more widespread, we could likely fix
it by adding ORDER BY to the completion queries, but I'm not sure that
it's worth doing if only these dead macOS versions have the issue.
(On the other hand, it seems like bad practice to be issuing queries
that have LIMIT without ORDER BY, so maybe we should fix them anyway.)


I'm strongly tempted to just push this and see what the buildfarm
thinks of it.  If it fails in lots of places, maybe the idea is a
dead end.  If it works, I'd look into extending the tests --- in
particular, I'd like to have some coverage for the filename
completion logic.

Thoughts?

            regards, tom lane

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

diff --git a/configure b/configure
index 9de5037..a133f66 100755
--- a/configure
+++ b/configure
@@ -706,6 +706,7 @@ with_libxml
 XML2_CONFIG
 UUID_EXTRA_OBJS
 with_uuid
+with_readline
 with_systemd
 with_selinux
 with_openssl
@@ -8000,6 +8001,7 @@ $as_echo "$as_me: WARNING: *** Readline does not work on MinGW --- disabling" >&
 fi


+
 #
 # Prefer libedit
 #
diff --git a/configure.in b/configure.in
index 9c5e5e7..9172622 100644
--- a/configure.in
+++ b/configure.in
@@ -875,6 +875,7 @@ if test "$PORTNAME" = "win32"; then
     with_readline=no
   fi
 fi
+AC_SUBST(with_readline)


 #
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 05b6638..5002c47 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -185,6 +185,7 @@ with_perl    = @with_perl@
 with_python    = @with_python@
 with_tcl    = @with_tcl@
 with_openssl    = @with_openssl@
+with_readline    = @with_readline@
 with_selinux    = @with_selinux@
 with_systemd    = @with_systemd@
 with_gssapi    = @with_gssapi@
diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b1..10b6dd3 100644
--- a/src/bin/psql/.gitignore
+++ b/src/bin/psql/.gitignore
@@ -1,5 +1,5 @@
 /psqlscanslash.c
 /sql_help.h
 /sql_help.c
-
 /psql
+/tmp_check/
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 42771bc..d08767b 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -16,6 +16,9 @@ subdir = src/bin/psql
 top_builddir = ../../..
 include $(top_builddir)/src/Makefile.global

+# make this available to TAP test scripts
+export with_readline
+
 REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref

 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
@@ -73,8 +76,15 @@ uninstall:

 clean distclean:
     rm -f psql$(X) $(OBJS) lex.backup
+    rm -rf tmp_check

 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
     rm -f sql_help.h sql_help.c psqlscanslash.c
+
+check:
+    $(prove_check)
+
+installcheck:
+    $(prove_installcheck)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
new file mode 100644
index 0000000..1c7610f
--- /dev/null
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -0,0 +1,122 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+use IPC::Run qw(pump finish timer);
+
+if ($ENV{with_readline} ne 'yes')
+{
+    plan skip_all => 'readline is not supported by this build';
+}
+
+# If we don't have IO::Pty, forget it, because IPC::Run depends on that
+# to support pty connections
+eval { require IO::Pty; };
+if ($@)
+{
+    plan skip_all => 'IO::Pty is needed to run this test';
+}
+
+# start a new server
+my $node = get_new_node('main');
+$node->init;
+$node->start;
+
+# set up a few database objects
+$node->safe_psql('postgres',
+        "CREATE TABLE tab1 (f1 int, f2 text);\n"
+      . "CREATE TABLE mytab123 (f1 int, f2 text);\n"
+      . "CREATE TABLE mytab246 (f1 int, f2 text);\n");
+
+# Developers would not appreciate this test adding a bunch of junk to
+# their ~/.psql_history, so be sure to redirect history into a temp file.
+# We might as well put it in the test log directory, so that buildfarm runs
+# capture the result for possible debugging purposes.
+my $historyfile = "${TestLib::log_path}/010_psql_history.txt";
+$ENV{PSQL_HISTORY} = $historyfile;
+
+# fire up an interactive psql session
+my $in  = '';
+my $out = '';
+
+my $timer = timer(5);
+
+my $h = $node->interactive_psql('postgres', \$in, \$out, $timer);
+
+ok($out =~ /psql/, "print startup banner");
+
+# Simple test case: type something and see if psql responds as expected
+sub check_completion
+{
+    my ($send, $pattern, $annotation) = @_;
+
+    # reset output collector
+    $out = "";
+    # restart per-command timer
+    $timer->start(5);
+    # send the data to be sent
+    $in .= $send;
+    # wait ...
+    pump $h until ($out =~ m/$pattern/ || $timer->is_expired);
+    my $okay = ($out =~ m/$pattern/ && !$timer->is_expired);
+    ok($okay, $annotation);
+    # for debugging, log actual output if it didn't match
+    note 'Actual output was "' . $out . "\"\n" if !$okay;
+}
+
+# Clear query buffer to start over
+# (won't work if we are inside a string literal!)
+sub clear_query
+{
+    check_completion("\\r\n", "postgres=# ", "\\r works");
+}
+
+# check basic command completion: SEL<tab> produces SELECT<space>
+check_completion("SEL\t", "SELECT ", "complete SEL<tab> to SELECT");
+
+clear_query();
+
+# check case variation is honored
+check_completion("sel\t", "select ", "complete sel<tab> to select");
+
+# check basic table name completion
+check_completion("* from t\t", "\\* from tab1 ", "complete t<tab> to tab1");
+
+clear_query();
+
+# check table name completion with multiple alternatives
+# note: readline might print a bell before the completion
+check_completion(
+    "select * from my\t",
+    "select \\* from my\a?tab",
+    "complete my<tab> to mytab when there are multiple choices");
+
+# some versions of readline/libedit require two tabs here, some only need one
+check_completion("\t\t", "mytab123 +mytab246",
+    "offer multiple table choices");
+
+check_completion("2\t", "246 ",
+    "finish completion of one of multiple table choices");
+
+clear_query();
+
+# check case-sensitive keyword replacement
+# XXX the output here might vary across readline versions
+check_completion(
+    "\\DRD\t",
+    "\\DRD\b\b\bdrds ",
+    "complete \\DRD<tab> to \\drds");
+
+clear_query();
+
+# send psql an explicit \q to shut it down, else pty won't close properly
+$timer->start(5);
+$in .= "\\q\n";
+finish $h or die "psql returned $?";
+$timer->reset;
+
+# done
+$node->stop;
+done_testing();
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 270bd6c..2e0cf4a 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1534,6 +1534,73 @@ sub psql

 =pod

+=item $node->interactive_psql($dbname, \$stdin, \$stdout, $timer, %params) => harness
+
+Invoke B<psql> on B<$dbname> and return an IPC::Run harness object,
+which the caller may use to send interactive input to B<psql>.
+The process's stdin is sourced from the $stdin scalar reference,
+and its stdout and stderr go to the $stdout scalar reference.
+ptys are used so that psql thinks it's being called interactively.
+
+The specified timer object is attached to the harness, as well.
+It's caller's responsibility to select the timeout length, and to
+restart the timer after each command if the timeout is per-command.
+
+psql is invoked in tuples-only unaligned mode with reading of B<.psqlrc>
+disabled.  That may be overridden by passing extra psql parameters.
+
+Dies on failure to invoke psql, or if psql fails to connect.
+Errors occurring later are the caller's problem.
+
+Be sure to "finish" the harness when done with it.
+
+The only extra parameter currently accepted is
+
+=over
+
+=item extra_params => ['--single-transaction']
+
+If given, it must be an array reference containing additional parameters to B<psql>.
+
+=back
+
+This requires IO::Pty in addition to IPC::Run.
+
+=cut
+
+sub interactive_psql
+{
+    my ($self, $dbname, $stdin, $stdout, $timer, %params) = @_;
+
+    my @psql_params = ('psql', '-XAt', '-d', $self->connstr($dbname));
+
+    push @psql_params, @{ $params{extra_params} }
+      if defined $params{extra_params};
+
+    # Ensure there is no data waiting to be sent:
+    $$stdin = "" if ref($stdin);
+    # IPC::Run would otherwise append to existing contents:
+    $$stdout = "" if ref($stdout);
+
+    my $harness = IPC::Run::start \@psql_params,
+      '<pty<', $stdin, '>pty>', $stdout, $timer;
+
+    # Pump until we see psql's help banner.  This ensures that callers
+    # won't write anything to the pty before it's ready, avoiding an
+    # implementation issue in IPC::Run.  Also, it means that psql
+    # connection failures are caught here, relieving callers of
+    # the need to handle those.  (Right now, we have no particularly
+    # good handling for errors anyway, but that might be added later.)
+    pump $harness
+      until $$stdout =~ /Type "help" for help/ || $timer->is_expired;
+
+    die "psql startup timed out" if $timer->is_expired;
+
+    return $harness;
+}
+
+=pod
+
 =item $node->poll_query_until($dbname, $query [, $expected ])

 Run B<$query> repeatedly, until it returns the B<$expected> result