pgsql: Add basic TAP tests for psql's tab-completion logic.

Re: Tom Lane 2020-01-02 <E1in6ft-0004zR-6l@gemulon.postgresql.org>

Add basic TAP tests for psql's tab-completion logic.

The \DRD test fails on Debian/unstable:

# 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");

00:58:02 rm -rf '/<<PKGBUILDDIR>>/build/src/bin/psql'/tmp_check
00:58:02 /bin/mkdir -p '/<<PKGBUILDDIR>>/build/src/bin/psql'/tmp_check
00:58:02 cd /<<PKGBUILDDIR>>/build/../src/bin/psql && TESTDIR='/<<PKGBUILDDIR>>/build/src/bin/psql' PATH="/<<PKGBUILDDIR>>/build/tmp_install/usr/lib/postgresql/13/bin:$PATH" LD_LIBRARY_PATH="/<<PKGBUILDDIR>>/build/tmp_install/usr/lib/x86_64-linux-gnu" PGPORT='65432' PG_REGRESS='/<<PKGBUILDDIR>>/build/src/bin/psql/../../../src/test/regress/pg_regress' REGRESS_SHLIB='/<<PKGBUILDDIR>>/build/src/test/regress/regress.so' /usr/bin/prove -I /<<PKGBUILDDIR>>/build/../src/test/perl/ -I /<<PKGBUILDDIR>>/build/../src/bin/psql t/*.pl
00:58:09
00:58:09 # Failed test 'complete \DRD<tab> to \drds'
00:58:09 # at t/010_tab_completion.pl line 64.
00:58:09 # Looks like you failed 1 test of 12.
00:58:09 t/010_tab_completion.pl ..
00:58:09 Dubious, test returned 1 (wstat 256, 0x100)
00:58:09 Failed 1/12 subtests
00:58:09
00:58:09 Test Summary Report
00:58:09 -------------------
00:58:09 t/010_tab_completion.pl (Wstat: 256 Tests: 12 Failed: 1)
00:58:09 Failed test: 11
00:58:09 Non-zero exit status: 1
00:58:09 Files=1, Tests=12, 7 wallclock secs ( 0.01 usr 0.01 sys + 0.77 cusr 0.23 csys = 1.02 CPU)
00:58:09 Result: FAIL
00:58:09 make[2]: *** [Makefile:87: check] Error 1

https://pgdgbuild.dus.dg-i.net/job/postgresql-13-binaries/architecture=amd64,distribution=sid/444/console

Shouldn't this print some "expected foo, got bar" diagnostics instead
of just dying?

Christoph

On Thu, Jan 02, 2020 at 08:02:29PM +0000, Tom Lane wrote:

Add basic TAP tests for psql's tab-completion logic.

Up to now, psql's tab-complete.c has had exactly no regression test
coverage. This patch is an experimental attempt to add some.

This needs Perl's IO::Pty module, which isn't installed everywhere,
so the test script just skips all tests if that's not present.
There may be other portability gotchas too, so I await buildfarm
results with interest.

Reading through the commit logs, I am not a fan of this part:
+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';
+}

This has the disadvantage to have people never actually notice if the
tests are running or not because this does not generate a dependency
error. Skipping things if libreadline is not around is perfectly fine
IMO, but I think that we should harden things for IO::Pty by removing
this skipping part, and by adding a test in configure.in's
AX_PROG_PERL_MODULES. That would be also more consistent with the
approach we take with other tests.
--
Michael

Christoph Berg <myon@debian.org> writes:

Re: Tom Lane 2020-01-02 <E1in6ft-0004zR-6l@gemulon.postgresql.org>

Add basic TAP tests for psql's tab-completion logic.

The \DRD test fails on Debian/unstable:

Indeed. It appears that recent libedit breaks tab-completion for
words involving a backslash, which is the fault of this upstream
commit:

http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/filecomplete.c.diff?r1=1.52&amp;r2=1.53

Basically what that's doing is applying de-backslashing to EVERY
word that completion is attempted on, whether it might be a filename
or not. So what psql_complete sees in this test case is just "DRD"
which of course it does not recognize as a possible psql backslash
command.

I found out while investigating this that the libedit version shipping
with buster (3.1-20181209) is differently broken for the same case:
instead of inapproriate forced de-escaping of the input of the
application-specific completion function, it applies inapproriate
forced escaping to the output of said function, so that when we see
"\DRD" and return "\drds", what comes out to the user is "\\drds".
libedit apparently needs a regression test suite even worse than we do.

I was kind of despairing of fixing this last night, but in the light
of morning it occurs to me that there's a possible workaround for the
de-escape bug: we could make psql_completion ignore the passed "text"
string and look at the original input buffer, as
get_previous_words() is already doing. I don't see any way to
dodge buster's bug, though.

regards, tom lane

Christoph Berg <myon@debian.org> writes:

Shouldn't this print some "expected foo, got bar" diagnostics instead
of just dying?

BTW, as far as that goes, we do: see for instance the tail end of

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=desmoxytes&amp;dt=2020-01-02%2020%3A04%3A03

ok 8 - offer multiple table choices
ok 9 - finish completion of one of multiple table choices
ok 10 - \r works
not ok 11 - complete \DRD<tab> to \drds

# Failed test 'complete \DRD<tab> to \drds'
# at t/010_tab_completion.pl line 64.
# Actual output was "\DRD"
ok 12 - \r works

Not sure why you are not seeing the "Actual output" bit in your log.
I used a "note" command to print it, maybe that's not best practice?

Also, while I'm asking for Perl advice: I can see in my editor that
there's a control-G bell character in that string, but this is far
from obvious on the web page. I'd kind of like to get the report
to escapify control characters so that what comes out is more like

# Actual output was "\DRD^G"
or
# Actual output was "\\DRD\007"

or some such. Anybody know an easy way to do that in Perl?

regards, tom lane

Michael Paquier <michael@paquier.xyz> writes:

This has the disadvantage to have people never actually notice if the
tests are running or not because this does not generate a dependency
error. Skipping things if libreadline is not around is perfectly fine
IMO, but I think that we should harden things for IO::Pty by removing
this skipping part, and by adding a test in configure.in's
AX_PROG_PERL_MODULES. That would be also more consistent with the
approach we take with other tests.

I do not think that requiring IO::Pty is practical. It's not going
to be present in Windows installations, for starters, because it's
nonfunctional there. I've also found that it fails to compile on
some of my older buildfarm dinosaurs.

In the case of IPC::Run, having a hard dependency is sensible because
the TAP tests pretty much can't do anything at all without it.
However, we don't need IO::Pty except for testing a few psql behaviors,
so it's fine with me if some buildfarm members don't run those tests.

There's precedent, too: see for instance
src/test/recovery/t/017_shm.pl
which is where I stole this coding technique from.

regards, tom lane

Re: Tom Lane 2020-01-03 <13708.1578059577@sss.pgh.pa.us>

I found out while investigating this that the libedit version shipping
with buster (3.1-20181209) is differently broken for the same case:

(Fwiw this wasn't spotted before because we have this LD_PRELOAD hack
that replaces libedit with readline at psql runtime. I guess that
means that the hack is pretty stable... Still, looking forward to the
day that OpenSSL is finally relicensing so we can properly link to
readline.)

Re: Tom Lane 2020-01-03 <14261.1578060227@sss.pgh.pa.us>

Shouldn't this print some "expected foo, got bar" diagnostics instead
of just dying?

BTW, as far as that goes, we do: see for instance the tail end of

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=desmoxytes&amp;dt=2020-01-02%2020%3A04%3A03

ok 8 - offer multiple table choices
ok 9 - finish completion of one of multiple table choices
ok 10 - \r works
not ok 11 - complete \DRD<tab> to \drds

# Failed test 'complete \DRD<tab> to \drds'
# at t/010_tab_completion.pl line 64.
# Actual output was "\DRD"
ok 12 - \r works

Not sure why you are not seeing the "Actual output" bit in your log.
I used a "note" command to print it, maybe that's not best practice?

I think best practice is to use something like

like($out, qr/$pattern/, $annotation)

instead of plain "ok()" which doesn't know about the actual values
compared. The "&& !$timer->is_expired" condition can be dropped from
the test because all we care about is if the output matches.

I never really grasped in which contexts TAP is supposed to print the
full test output ("ok 10 -..."). Apparently the way the testsuite is
invoked at package build time only prints the terse failure summary in
which "note"s aren't included. Is there a switch to configure that?

Also, while I'm asking for Perl advice: I can see in my editor that
there's a control-G bell character in that string, but this is far
from obvious on the web page. I'd kind of like to get the report
to escapify control characters so that what comes out is more like

# Actual output was "\DRD^G"
or
# Actual output was "\\DRD\007"

or some such. Anybody know an easy way to do that in Perl?

I don't know for note(), but maybe like() would do that automatically.

Christoph

Tom Lane <tgl@sss.pgh.pa.us> writes:

Also, while I'm asking for Perl advice: I can see in my editor that
there's a control-G bell character in that string, but this is far
from obvious on the web page. I'd kind of like to get the report
to escapify control characters so that what comes out is more like

# Actual output was "\DRD^G"
or
# Actual output was "\\DRD\007"

or some such. Anybody know an easy way to do that in Perl?

I was going to suggest using Test::More's like() function to do the
regex check, but sadly that only escapes things that would break the TAP
stream syntax, not non-printables in general. The next obvious thing is
Data::Dumper with the 'Useqq' option enabled, which makes it use
double-quoted-string escapes (e.g. "\a" for ^G).

The attaced patch does that, and also bumps $Test::Builder::Level so the
diagnostic references the calling line, and uses diag() instad of
note(), so it shows even in non-verbose mode.

- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen

Christoph Berg <myon@debian.org> writes:

Re: Tom Lane 2020-01-03 <13708.1578059577@sss.pgh.pa.us>

I found out while investigating this that the libedit version shipping
with buster (3.1-20181209) is differently broken for the same case:

(Fwiw this wasn't spotted before because we have this LD_PRELOAD hack
that replaces libedit with readline at psql runtime.

You do? I went looking in the Debian package source repo just the
other day for some evidence that that was true, and couldn't find
any, so I concluded that it was only an urban legend. Where is that
done exactly?

Perhaps more importantly, *why* is it done? It seems to me that it
takes a pretty fevered imagination to suppose that using libreadline
that way meets the terms of its license but just building against
the library normally would not. Certainly when I worked for Red Hat,
their lawyers did not think there was any problem with building
Postgres using both openssl and readline.

The reason I'm concerned about this is that there's a patch on the
table [1]/messages/by-id/16059-8836946734c02b84@postgresql.org that will probably not behave nicely at all if it's
compiled against libedit headers and then executed with libreadline,
because it will draw the wrong conclusions about whether the
filename quoting hooks are available. So that hack is going to
fail on you soon, especially after I add regression testing around
the filename completion stuff ;-)

I used a "note" command to print it, maybe that's not best practice?

I think best practice is to use something like
like($out, qr/$pattern/, $annotation)

I'll check into that, thanks!

regards, tom lane

[1]: /messages/by-id/16059-8836946734c02b84@postgresql.org

Re: Tom Lane 2020-01-03 <26339.1578072930@sss.pgh.pa.us>

Christoph Berg <myon@debian.org> writes:

Re: Tom Lane 2020-01-03 <13708.1578059577@sss.pgh.pa.us>

I found out while investigating this that the libedit version shipping
with buster (3.1-20181209) is differently broken for the same case:

(Fwiw this wasn't spotted before because we have this LD_PRELOAD hack
that replaces libedit with readline at psql runtime.

You do? I went looking in the Debian package source repo just the
other day for some evidence that that was true, and couldn't find
any, so I concluded that it was only an urban legend. Where is that
done exactly?

/usr/share/postgresql-common/pg_wrapper

https://salsa.debian.org/postgresql/postgresql-common/blob/master/pg_wrapper#L129-157

Perhaps more importantly, *why* is it done? It seems to me that it
takes a pretty fevered imagination to suppose that using libreadline

Tom, claiming that things are "fevered" just because you didn't like
them is not appropriate. It's not fun working with PostgreSQL when the
tone is like that.

that way meets the terms of its license but just building against
the library normally would not. Certainly when I worked for Red Hat,
their lawyers did not think there was any problem with building
Postgres using both openssl and readline.

I'm not starting that debate here, but Debian thinks otherwise:

https://lwn.net/Articles/428111/

The reason I'm concerned about this is that there's a patch on the
table [1] that will probably not behave nicely at all if it's
compiled against libedit headers and then executed with libreadline,
because it will draw the wrong conclusions about whether the
filename quoting hooks are available. So that hack is going to
fail on you soon, especially after I add regression testing around
the filename completion stuff ;-)

Well, so far, it worked well. (The biggest problem used to be that
libedit didn't have the history append function so it wasn't used even
with readline, but that got implemented ~2 years ago.)

Christoph

ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

Anybody know an easy way to do that in Perl?

I was going to suggest using Test::More's like() function to do the
regex check, but sadly that only escapes things that would break the TAP
stream syntax, not non-printables in general. The next obvious thing is
Data::Dumper with the 'Useqq' option enabled, which makes it use
double-quoted-string escapes (e.g. "\a" for ^G).

The attaced patch does that, and also bumps $Test::Builder::Level so the
diagnostic references the calling line, and uses diag() instad of
note(), so it shows even in non-verbose mode.

LGTM, pushed (along with a fix to deal with what hopefully is the
only remaining obstacle for Andres' critters).

regards, tom lane

On Fri, Jan 3, 2020 at 12:48 PM Christoph Berg <myon@debian.org> wrote:

Perhaps more importantly, *why* is it done? It seems to me that it
takes a pretty fevered imagination to suppose that using libreadline

Tom, claiming that things are "fevered" just because you didn't like
them is not appropriate. It's not fun working with PostgreSQL when the
tone is like that.

+1.

that way meets the terms of its license but just building against
the library normally would not. Certainly when I worked for Red Hat,
their lawyers did not think there was any problem with building
Postgres using both openssl and readline.

I'm not starting that debate here, but Debian thinks otherwise:

https://lwn.net/Articles/428111/

I take no position on whether Debian is correct in its assessment of
such things, but I reiterate my previous opposition to breaking it
just because we don't agree with it, or because Tom specifically
doesn't. It's too mainstream a platform to arbitrarily break. And it
will probably just have the effect of increasing the number of patches
they're carrying against our sources, which will not make things
better for anybody.

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

Christoph Berg <myon@debian.org> writes:

Re: Tom Lane 2020-01-03 <26339.1578072930@sss.pgh.pa.us>

You do? I went looking in the Debian package source repo just the
other day for some evidence that that was true, and couldn't find
any, so I concluded that it was only an urban legend. Where is that
done exactly?

/usr/share/postgresql-common/pg_wrapper
https://salsa.debian.org/postgresql/postgresql-common/blob/master/pg_wrapper#L129-157

Oh, so not in the Postgres package per se.

What that means is that our regression tests will pass (as it's
just a regular libedit install while they're running) but then filename
completion will not work well for actual users. And there's not really
anything I can do about that from this end.

(On the other hand, filename completion is already kind of buggy,
which is why that patch exists in the first place. So maybe it
won't get any worse. Hard to say.)

regards, tom lane

On Fri, Jan 3, 2020 at 9:59 AM Robert Haas <robertmhaas@gmail.com> wrote:

I take no position on whether Debian is correct in its assessment of
such things, but I reiterate my previous opposition to breaking it
just because we don't agree with it, or because Tom specifically
doesn't. It's too mainstream a platform to arbitrarily break. And it
will probably just have the effect of increasing the number of patches
they're carrying against our sources, which will not make things
better for anybody.

Even with commit 56a3921a, "make check-world" is broken on my Ubuntu
18.04 workstation. This is now adversely impacting my work, so I hope
it can be resolved soon.

Not sure if the specifics matter, but FWIW "make check-world" ended
with the following failure just now:

make[2]: Entering directory '/code/postgresql/patch/build/src/bin/psql'
rm -rf '/code/postgresql/patch/build/src/bin/psql'/tmp_check
/bin/mkdir -p '/code/postgresql/patch/build/src/bin/psql'/tmp_check
cd /code/postgresql/patch/build/../source/src/bin/psql &&
TESTDIR='/code/postgresql/patch/build/src/bin/psql'
PATH="/code/postgresql/patch/build/tmp_install/code/postgresql/patch/install/bin:$PATH"
LD_LIBRARY_PATH="/code/postgresql/patch/build/tmp_install/code/postgresql/patch/install/lib"
PGPORT='65432'
PG_REGRESS='/code/postgresql/patch/build/src/bin/psql/../../../src/test/regress/pg_regress'
REGRESS_SHLIB='/code/postgresql/patch/build/src/test/regress/regress.so'
/usr/bin/prove -I
/code/postgresql/patch/build/../source/src/test/perl/ -I
/code/postgresql/patch/build/../source/src/bin/psql t/*.pl
t/010_tab_completion.pl .. 8/?
# Failed test 'offer multiple table choices'
# at t/010_tab_completion.pl line 105.
# Actual output was "\r\n\e[01;35mmytab\e[0m\e[K123\e[0m\e[K
\e[01;35mmytab\e[0m\e[K246\e[0m\e[K \r\npostgres=# select * from
mytab\r\n\e[01;35mmytab\e[0m\e[K123\e[0m\e[K
\e[01;35mmytab\e[0m\e[K246\e[0m\e[K \r\npostgres=# select * from
mytab"
#
# Looks like you failed 1 test of 12.
t/010_tab_completion.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/12 subtests

Test Summary Report
-------------------
t/010_tab_completion.pl (Wstat: 256 Tests: 12 Failed: 1)
Failed test: 8
Non-zero exit status: 1
Files=1, Tests=12, 7 wallclock secs ( 0.02 usr 0.00 sys + 0.80 cusr
0.09 csys = 0.91 CPU)
Result: FAIL
Makefile:87: recipe for target 'check' failed
make[2]: *** [check] Error 1
make[2]: Leaving directory '/code/postgresql/patch/build/src/bin/psql'
Makefile:41: recipe for target 'check-psql-recurse' failed
make[1]: *** [check-psql-recurse] Error 2
make[1]: Leaving directory '/code/postgresql/patch/build/src/bin'
GNUmakefile:70: recipe for target 'check-world-src/bin-recurse' failed
make: *** [check-world-src/bin-recurse] Error 2

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

Not sure if the specifics matter, but FWIW "make check-world" ended
with the following failure just now:

# Failed test 'offer multiple table choices'
# at t/010_tab_completion.pl line 105.
# Actual output was "\r\n\e[01;35mmytab\e[0m\e[K123\e[0m\e[K
\e[01;35mmytab\e[0m\e[K246\e[0m\e[K \r\npostgres=# select * from
mytab\r\n\e[01;35mmytab\e[0m\e[K123\e[0m\e[K
\e[01;35mmytab\e[0m\e[K246\e[0m\e[K \r\npostgres=# select * from
mytab"

Huh. What readline or libedit version are you using, on what
platform? I'm curious also what is your prevailing setting
of TERM? (I've been wondering if the test doesn't need to
force that to something standard. The buildfarm hasn't shown
any signs of needing that, but manual invocations might be
a different story.)

regards, tom lane

On Fri, Jan 3, 2020 at 6:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Huh. What readline or libedit version are you using, on what
platform?

Ubuntu 18.04. I used ldd to verify that psql links to the system
libreadline, which is libreadline7:amd64 -- that's what Debian
packages as "7.0-3".

I'm curious also what is your prevailing setting
of TERM?

I use zsh, with a fairly customized setup. $TERM is "xterm-256color"
in the affected shell. (I have a feeling that this has something to do
with my amazing technicolor terminal.)

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

I'm curious also what is your prevailing setting
of TERM?

I use zsh, with a fairly customized setup. $TERM is "xterm-256color"
in the affected shell. (I have a feeling that this has something to do
with my amazing technicolor terminal.)

Hmm. If you set it to plain "xterm", does the test pass?

regards, tom lane

On Fri, Jan 3, 2020 at 6:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hmm. If you set it to plain "xterm", does the test pass?

No. Also tried setting PG_COLOR="off" and CLICOLOR=0 -- that also
didn't help. (This was based on possibly-relevant vars that "env"
showed were set).

--
Peter Geoghegan

On Fri, Jan 3, 2020 at 7:06 PM Peter Geoghegan <pg@bowt.ie> wrote:

No. Also tried setting PG_COLOR="off" and CLICOLOR=0 -- that also
didn't help. (This was based on possibly-relevant vars that "env"
showed were set).

Removing the single check_completion() test from 010_tab_completion.pl
that actually fails on my system ("offer multiple table choices")
fixes the problem for me -- everything else passes.

I suppose that this means that the problem is in "offer multiple table
choices" specifically.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

On Fri, Jan 3, 2020 at 7:06 PM Peter Geoghegan <pg@bowt.ie> wrote:

No. Also tried setting PG_COLOR="off" and CLICOLOR=0 -- that also
didn't help. (This was based on possibly-relevant vars that "env"
showed were set).

Yeah, that's not terribly surprising, because if I'm reading those
escape sequences correctly they're not about color. They seem to be
just cursor movement and line clearing, according to [1]https://www.xfree86.org/current/ctlseqs.html.

What I'm mystified by is why your copy of libreadline is choosing to
do that, rather than just space over to where the word should be printed
which is what every other copy seems to be doing. I have a fresh new
Debian installation at hand, with

$ dpkg -l | grep readline
ii libreadline-dev:amd64 7.0-5 amd64 GNU readline and history libraries, development files
ii libreadline5:amd64 5.2+dfsg-3+b13 amd64 GNU readline and history libraries, run-time libraries
ii libreadline7:amd64 7.0-5 amd64 GNU readline and history libraries, run-time libraries
ii readline-common 7.0-5 all GNU readline and history libraries, common files

and I'm not seeing the failure on it, either with TERM=xterm
or with TERM=xterm-256color. So what's the missing ingredient?

Removing the single check_completion() test from 010_tab_completion.pl
that actually fails on my system ("offer multiple table choices")
fixes the problem for me -- everything else passes.
I suppose that this means that the problem is in "offer multiple table
choices" specifically.

I'd hate to conclude that we can't test any completion behavior that
involves offering a list.

If we can't coerce libreadline into being less avant-garde in its
screen management, I suppose we could write a regex to recognize
xterm escape sequences and ignore those. But I'd be happier about
this if I could reproduce the behavior. I don't like the feeling
that there's something going on here that I don't understand.

BTW, it seems somewhat likely that this is less about libreadline
than about its dependency libtinfo. On my machine that's from

ii libtinfo6:amd64 6.1+20181013-2+deb10u2 amd64 shared low-level terminfo library for terminal handling

what about yours?

regards, tom lane

[1]: https://www.xfree86.org/current/ctlseqs.html