Improve guards against false regex matches in BackgroundPsql.pm.

BackgroundPsql needs to wait for all the output from an interactive
psql command to come back.  To make sure that's happened, it issues
the command, then issues \echo and \warn psql commands that echo
a "banner" string (which we assume won't appear in the command's
output), then waits for the banner strings to appear.  The hazard
in this approach is that the banner will also appear in the echoed
psql commands themselves, so we need to distinguish those echoes from
the desired output.  Commit 8b886a4e3 tried to do that by positing
that the desired output would be directly preceded and followed by
newlines, but it turns out that that assumption is timing-sensitive.
In particular, it tends to fail in builds made --without-readline,
wherein the command echoes will be made by the pty driver and may
be interspersed with prompts issued by psql proper.

It does seem safe to assume that the banner output we want will be
followed by a newline, since that should be the last output before
things quiesce.  Therefore, we can improve matters by putting quotes
around the banner strings in the \echo and \warn psql commands, so
that their echoes cannot include banner directly followed by newline,
and then checking for just banner-and-newline in the match pattern.

While at it, spruce up the pump() call in sub query() to look like
the neater version in wait_connect(), and don't die on timeout
until after printing whatever we got.

Reported-by: Oleg Tselebrovskiy <o.tselebrovskiy@postgrespro.ru>
Diagnosed-by: Oleg Tselebrovskiy <o.tselebrovskiy@postgrespro.ru>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Soumya S Murali <soumyamurali.work@gmail.com>
Discussion: https://postgr.es/m/db6fdb35a8665ad3c18be01181d44b31@postgrespro.ru
Backpatch-through: 14

diff --git a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
index 4e0b73571dc5b46760f9498814223c0f4823c231..e0a6775156405b7bca2a84b0eed13f4b14930269 100644 (file)
--- a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
+++ b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
@@ -152,11 +152,11 @@ sub wait_connect
        #
        # 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 $banner_match = qr/$banner\r?\n/;
+       $self->{stdin} .= "\\echo '$banner'\n\\warn '$banner'\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",
@@ -256,22 +256,17 @@ 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.
+       # In interactive psql we emit \r\n, so we need to allow for that.
+       # Also, include quotes around the banner string in the \echo and \warn
+       # commands, not because the string needs quoting but so that $banner_match
+       # can't match readline's echoing of these commands.
        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;
+       my $banner_match = qr/$banner\r?\n/;
+       $self->{stdin} .= "$query\n;\n\\echo '$banner'\n\\warn '$banner'\n";
+       $self->{run}->pump()
+         until ($self->{stdout} =~ /$banner_match/
+                 && $self->{stderr} =~ /$banner_match/)
+         || $self->{timeout}->is_expired;
 
        note "results query $query_cnt:\n",
          explain {
@@ -279,9 +274,12 @@ sub query
                stderr => $self->{stderr},
          };
 
-       # 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.
+       die "psql query timed out" if $self->{timeout}->is_expired;
+
+       # Remove banner from stdout and stderr, our caller doesn't want it.
+       # Also remove the query output's trailing newline, if present (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//;