Re: 001_password.pl fails with --without-readline - Mailing list pgsql-hackers

Soumya S Murali <soumyamurali.work@gmail.com> writes:
> The patches seem correct based on the testing I have done so far and
> for me I did not observe any further issues in both readline and
> non-readline configurations. Since I am one of the reviewers and not
> the only one. I consider my review as only one data point and I would
> feel more comfortable if the patches also receive input from other
> reviewers who may have more experience with this part of the code or
> different testing environments.

Well, there aren't any other reviewers listed in the CF entry, so
you need to have more faith in yourself ;-).

However, I looked at these patches and I feel like they are not doing
enough to be proof against false matches, or false non-matches, due to
variations in readline (or no-readline) behavior.

The fundamental issue in 0001 is how do we avoid matching to the echo
of the \echo or \warn command rather than the output we want to see?
Oleg's patch assumes it's okay if there's not "\echo" or "\warn" just
ahead of the banner, but I doubt that's good enough.  Insertion of a
prompt, for example, could allow a false match.  I think we can
make the test far more robust if we rely on psql's ability to do a
little computation instead of only echoing the command string verbatim.
There are various ways that could be done, but what I propose in the
attached v3 is to break the banner into two psql variables, like so:
    \set part1 something
    \set part2 else
    \echo :part1 :part2
Then, if we match for "something else", there is no possibility
whatsoever that that will match echoed commands, only the real
output of the \echo.

While debugging that I got annoyed that a match failure results
in a timeout exit with absolutely no data logged about what output
the test got.  So v3-0001 also changes timeout() --- which creates
a timeout that aborts the test --- to timer() --- which does what
the test author clearly expected, namely just stop waiting for
more input.  (There's a thread somewhere around here about making
that change more globally, but I went ahead and did it here.)

As for 0002, I agree that we can't do much except not insist that
the match cover the whole line.  However, then the question is how
much risk are we taking of a false match?  It's not too bad for the
first three tests, where we know what the query will output so we
can be sure that the pattern will (or won't) appear if the pager
is or isn't invoked.  However, testing for just "\d+\n" seems fairly
scary from that standpoint.  It happens not to match to the current
contents of information_schema.referential_constraints, but that's
a very hairy query that's subject to change.  I think we had better
take this test out of the business of relying on the contents of that
view, and instead create our own simple view that we can be sure of.

On a more nitpicky level, writing "^.*" is useless, because it'll
match anything at all, so we might as well just remove it from the
pattern.

So I arrive at the v3 patches attached.  Any thoughts?

            regards, tom lane

diff --git a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
index 5bd41a278dd..aaa4c400525 100644
--- a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
+++ b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
@@ -100,7 +100,7 @@ sub new
       "Forbidden caller of constructor: package: $package, file: $file:$line"
       unless $package->isa('PostgreSQL::Test::Cluster');

-    $psql->{timeout} = IPC::Run::timeout(
+    $psql->{timeout} = IPC::Run::timer(
         defined($timeout)
         ? $timeout
         : $PostgreSQL::Test::Utils::timeout_default);
@@ -154,12 +154,14 @@ sub wait_connect
     # errors anyway, but that might be added later.)
     #
     # See query() for details about why/how the banner is used.
-    my $banner = "background_psql: ready";
-    my $banner_match = qr/(^|\n)$banner\r?\n/;
-    $self->{stdin} .= "\\echo $banner\n\\warn $banner\n";
+    my $part1 = "background_psql:";
+    my $part2 = "ready";
+    my $banner = "$part1 $part2";
+    my $banner_match = qr/$banner\r?\n/;
+    $self->{stdin} .= "\\set part1 $part1\n\\set part2 $part2\n\\echo :part1 :part2\n\\warn :part1 :part2\n";
     $self->{run}->pump()
       until ($self->{stdout} =~ /$banner_match/
-          && $self->{stderr} =~ /$banner\r?\n/)
+          && $self->{stderr} =~ /$banner_match/)
       || $self->{timeout}->is_expired;

     note "connect output:\n",
@@ -253,7 +255,9 @@ sub query
     # Feed the query to psql's stdin, followed by \n (so psql processes the
     # line), by a ; (so that psql issues the query, if it doesn't include a ;
     # itself), and a separator echoed both with \echo and \warn, that we can
-    # wait on.
+    # wait on.  The separator is broken into two psql variables, so that we
+    # can't accidentally match to readline's echo of the commands rather than
+    # the command output.
     #
     # To avoid somehow confusing the separator from separately issued queries,
     # and to make it easier to debug, we include a per-psql query counter in
@@ -264,22 +268,16 @@ sub query
     # stderr (or vice versa), even if psql printed them in the opposite
     # order. We therefore wait on both.
     #
-    # We need to match for the newline, because we try to remove it below, and
-    # it's possible to consume just the input *without* the newline. In
-    # interactive psql we emit \r\n, so we need to allow for that. Also need
-    # to be careful that we don't e.g. match the echoed \echo command, rather
-    # than its output.
-    my $banner = "background_psql: QUERY_SEPARATOR $query_cnt:";
-    my $banner_match = qr/(^|\n)$banner\r?\n/;
-    $self->{stdin} .= "$query\n;\n\\echo $banner\n\\warn $banner\n";
-    pump_until(
-        $self->{run}, $self->{timeout},
-        \$self->{stdout}, qr/$banner_match/);
-    pump_until(
-        $self->{run}, $self->{timeout},
-        \$self->{stderr}, qr/$banner_match/);
-
-    die "psql query timed out" if $self->{timeout}->is_expired;
+    # In interactive psql we emit \r\n, so we need to allow for that.
+    my $part1 = "background_psql:";
+    my $part2 = "QUERY_SEPARATOR_$query_cnt:";
+    my $banner = "$part1 $part2";
+    my $banner_match = qr/$banner\r?\n/;
+    $self->{stdin} .= "$query\n;\n\\set part1 $part1\n\\set part2 $part2\n\\echo :part1 :part2\n\\warn :part1
:part2\n";
+    $self->{run}->pump()
+      until ($self->{stdout} =~ /$banner_match/
+          && $self->{stderr} =~ /$banner_match/)
+      || $self->{timeout}->is_expired;

     note "results query $query_cnt:\n",
       explain {
@@ -287,9 +285,12 @@ sub query
         stderr => $self->{stderr},
       } unless !$params{verbose};

+    die "psql query timed out" if $self->{timeout}->is_expired;
+
     # Remove banner from stdout and stderr, our caller doesn't care.  The
     # first newline is optional, as there would not be one if consuming an
     # empty query result.
+    $banner_match = qr/\r?\n?$banner\r?\n/;
     $output = $self->{stdout};
     $output =~ s/$banner_match//;
     $self->{stderr} =~ s/$banner_match//;
diff --git a/src/bin/psql/t/030_pager.pl b/src/bin/psql/t/030_pager.pl
index cf81fb1603c..a35f2b26293 100644
--- a/src/bin/psql/t/030_pager.pl
+++ b/src/bin/psql/t/030_pager.pl
@@ -40,6 +40,36 @@ my $node = PostgreSQL::Test::Cluster->new('main');
 $node->init;
 $node->start;
 
+# create a view we'll use below
+$node->safe_psql(
+    'postgres', 'create view public.view_030_pager as select
+1 as a,
+2 as b,
+3 as c,
+4 as d,
+5 as e,
+6 as f,
+7 as g,
+8 as h,
+9 as i,
+10 as j,
+11 as k,
+12 as l,
+13 as m,
+14 as n,
+15 as o,
+16 as p,
+17 as q,
+18 as r,
+19 as s,
+20 as t,
+21 as u,
+22 as v,
+23 as w,
+24 as x,
+25 as y,
+26 as z');
+
 # fire up an interactive psql session
 my $h = $node->interactive_psql('postgres');
 
@@ -77,25 +107,28 @@ sub do_command
 #
 # Note that interactive_psql starts psql with --no-align --tuples-only,
 # and that the output string will include psql's prompts and command echo.
+# So we have to test for patterns that can't match the command itself,
+# and we can't assume the match will extend across a whole line (there
+# might be a prompt ahead of it in the output).
 
 do_command(
     "SELECT 'test' AS t FROM generate_series(1,23);\n",
-    qr/^test\r?$/m,
+    qr/test\r?$/m,
     "execute SELECT query that needs no pagination");
 
 do_command(
     "SELECT 'test' AS t FROM generate_series(1,24);\n",
-    qr/^ *24\r?$/m,
+    qr/24\r?$/m,
     "execute SELECT query that needs pagination");
 
 do_command(
     "\\pset expanded\nSELECT generate_series(1,20) as g;\n",
-    qr/^ *39\r?$/m,
+    qr/39\r?$/m,
     "execute SELECT query that needs pagination in expanded mode");
 
 do_command(
-    "\\pset tuples_only off\n\\d+ information_schema.referential_constraints\n",
-    qr/^ *\d+\r?$/m,
+    "\\pset tuples_only off\n\\d+ public.view_030_pager\n",
+    qr/55\r?$/m,
     "execute command with footer that needs pagination");
 
 # send psql an explicit \q to shut it down, else pty won't close properly