Обсуждение: 001_password.pl fails with --without-readline

Greetings!

If you configure PostgreSQL with --without-readline, test 
authentication/t/001_password.pl fails due to timeout

This can be reproduced on any branch that contains commit 8b886a4 (so, 
on PostgreSQL 13-19)

I've looked through debug info using IPCRUNDEBUG = "data" or "details"

Here's some output:

This is with readline:
     [18:31:45.030](0.027s) ok 22 - scram_iterations in server side ROLE
     IPC::Run 0001 [#15(253566)]: ** pumping
     IPC::Run 0001 [#15(253566)]: write( 10, '\echo background_psql: 
ready
     IPC::Run 0001 [#15(253566)]: \warn background_psql: ready
     IPC::Run 0001 [#15(253566)]: ' ) = 58
     IPC::Run 0001 [#15(253566)]: ** pumping
     IPC::Run 0001 [#15(253566)]: read( 10 ) = 60 chars '\echo 
background_psql: ready
     IPC::Run 0001 [#15(253566)]: \warn background_psql: ready
     IPC::Run 0001 [#15(253566)]: '
     IPC::Run 0001 [#15(253566)]: ** pumping
     IPC::Run 0001 [#15(253566)]: read( 10 ) = 41 chars 'psql (19devel)
     IPC::Run 0001 [#15(253566)]: Type "help" for help.
     IPC::Run 0001 [#15(253566)]:
     IPC::Run 0001 [#15(253566)]: '
     IPC::Run 0001 [#15(253566)]: ** pumping
     IPC::Run 0001 [#15(253566)]: read( 10 ) = 11 chars 'postgres=# '
     IPC::Run 0001 [#15(253566)]: ** pumping
     IPC::Run 0001 [#15(253566)]: read( 10 ) = 65 chars '\echo 
background_psql: ready
     IPC::Run 0001 [#15(253566)]: background_psql: ready
     IPC::Run 0001 [#15(253566)]: postgres=# '
     IPC::Run 0001 [#15(253566)]: ** pumping
     IPC::Run 0001 [#15(253566)]: read( 10 ) = 41 chars '\warn 
background_psql: ready
     IPC::Run 0001 [#15(253566)]: postgres=# '
     IPC::Run 0001 [#15(253566)]: read( 12 ) = 23 chars 'background_psql: 
ready
     IPC::Run 0001 [#15(253566)]: '
     [18:31:45.042](0.013s) # connect output:

And this is without readline:
     [18:34:32.050](0.029s) ok 22 - scram_iterations in server side ROLE
     IPC::Run 0001 [#15(263802)]: ** pumping
     IPC::Run 0001 [#15(263802)]: write( 10, '\echo background_psql: 
ready
     IPC::Run 0001 [#15(263802)]: \warn background_psql: ready
     IPC::Run 0001 [#15(263802)]: ' ) = 58
     IPC::Run 0001 [#15(263802)]: ** pumping
     IPC::Run 0001 [#15(263802)]: read( 10 ) = 60 chars '\echo 
background_psql: ready
     IPC::Run 0001 [#15(263802)]: \warn background_psql: ready
     IPC::Run 0001 [#15(263802)]: '
     IPC::Run 0001 [#15(263802)]: ** pumping
     IPC::Run 0001 [#15(263802)]: read( 10 ) = 98 chars 'psql (19devel)
     IPC::Run 0001 [#15(263802)]: Type "help" for help.
     IPC::Run 0001 [#15(263802)]:
     IPC::Run 0001 [#15(263802)]: postgres=# background_psql: ready
     IPC::Run 0001 [#15(263802)]: postgres=# postgres=# '
     IPC::Run 0001 [#15(263802)]: ** pumping
     IPC::Run 0001 [#15(263802)]: read( 12 ) = 23 chars 'background_psql: 
ready
     IPC::Run 0001 [#15(263802)]: '
     IPC::Run 0001 [#15(263802)]: ** pumping

As you can see, those outputs are different

1) In readline output commands \echo and \warn are re-read (that's why 
banner_match in BackgroundPsql.pm has checks for \n or start of line), 
but in no-readline output both \echo and \warn are read only once, when 
they are first send to psql

2) In readline ouput stdout is re-read multiple times, and banner 
(background_psql: ready) is placed neatly on it's own line, while in 
no-readline output banner in stdout is mixed with "postgres=#" prompts, 
and they are read as one batch

Those differences are probably an effect of inner workings of readline, 
turning input echoing off.

Also, without readline, stdout during query exectution can look like 
this
     # \\echo background_psql: QUERY_SEPARATOR 1:
     # \\warn background_psql: QUERY_SEPARATOR 1:
     # SET
     # postgres=# postgres=# background_psql: QUERY_SEPARATOR 1:
     # postgres=# '

So, there are some ways to handle this situation

1) Skip this test if Postgres is configured without readline support. I 
don't think this is a good way, since this test doesn't really test 
anything that needs readline to work

2) Modify variable banner_match depending on having readline or not. 
Something like the attached

I'm interested to hear your thoughts on this

Oleg Tselebrovskiy, PostgresPro

Hi,

I reapplied the patch and tested the patch with (enable-tap-tests +
readline and enable-tap-tests + without-readline). I think the issue
was caused by differences in how psql emits prompts and echoed output
without readline which could lead to banner interleaving, timeouts or
premature TAP termination. In my patch I included the fixes that make
banner detection in BackgroundPsql.pm to be tolerant while keeping
strict line-based cleanup to preserve correct TAP behavior. The fix is
behavior-based and does not rely on configuration variables or test
skipping.
Kindly review my patch attached herewith and please let me know the feedback.

Regards
Soumya

Thanks for your patch!

I like your behaviour-based approach more, but I have some questions
about the patch itself

1) There is a comment in query() function:
# 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.

So, originally, checking that the banner is on it's own line is needed
to distiguish \echo $banner and \warn $banner from their output.

It seems that your patch does not distingush them all the time

(taken from query() function with your patch applied)
    my $banner_detect_stdout = qr/\Q$banner\E/;
    my $banner_detect_stderr = qr/(^|\n)\Q$banner\E\r?\n/;

Why do you check stderr for newline but not the stdout?

2) Why did you add /Q/E around the $banner variable? It doesn't
contain any regex metacharacters (in query() function there is
$query_cnt inside of $banner, but it should be substituted by Perl).
Maybe it is really necessary and I just don't get it

-----

While testing and checking, I've made a new patch. It's very
simple and it just checks that there is banner in a string, but
the string doesn't start with \echo or \warn

Maybe it needs some additional comments, but lets decide on which
approach to use

-----

There is also a problem of test src/bin/psql/t/030_pager.pl - it
just doesn't work with --without-readline, all regexes seem to be right
and debug output shows correct data. None of our patches
currently solve it, I'll look into it

-----

Regards,
Oleg

The easiest way to fix 030_pager.pl is to just replace ' ' with '*'
in regex. With readline, everything that we look for is placed on
its own line so we don't break anything, but --without-readline
produces the following output (with some hand-written debug info):

    IPC::Run 0000 [#2(438962)]: ** pumping
    IPC::Run 0000 [#2(438962)]: read( 11 ) = 4 chars '39
    IPC::Run 0000 [#2(438962)]: '

    stream:postgres=# \pset expanded
    SELECT generate_series(1,20) as g;
    Expanded display is on.
    postgres=# 39

So the output that is turned on with IPCRUNDEBUG=data shows us "we got
only 4 chars, '39\n\0'!", but in reality we have more stuff in stream,
so pump_until() function doesn't match with passed regex

Patch is attached

Regards,
Oleg

Hi all,

Thanks for the detailed review and for sharing the updated patches.

> 1) There is a comment in query() function:
> # 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.
>
> So, originally, checking that the banner is on it's own line is needed
> to distiguish \echo $banner and \warn $banner from their output.
>
> It seems that your patch does not distingush them all the time
>
> (taken from query() function with your patch applied)
>         my $banner_detect_stdout = qr/\Q$banner\E/;
>         my $banner_detect_stderr = qr/(^|\n)\Q$banner\E\r?\n/;
>
> Why do you check stderr for newline but not the stdout?
>

I agree that the original intent of requiring the banner to be on its
own line was to distinguish the output of \echo / \warn from the
echoed commands themselves. In without-readline builds, stdout
buffering can merge prompts and echoed output into a single chunk,
which makes line-anchored matching unreliable there. Stderr, on the
other hand, continues to emit the banner cleanly on its own line, so
keeping it line-based remains reliable. That was the motivation for
relaxing stdout detection while preserving stricter handling on
stderr.

> 2) Why did you add /Q/E around the $banner variable? It doesn't
> contain any regex metacharacters (in query() function there is
> $query_cnt inside of $banner, but it should be substituted by Perl).
> Maybe it is really necessary and I just don't get it
>

This was mainly a defensive choice to ensure the banner is always
treated as a literal string and to avoid any accidental regex
interpretation if the banner format changes in the future. If you feel
this is unnecessary here, I am fine with simplifying it.

>
> While testing and checking, I've made a new patch. It's very
> simple and it just checks that there is banner in a string, but
> the string doesn't start with \echo or \warn
>
> Maybe it needs some additional comments, but lets decide on which
> approach to use
>

I reviewed the patch and agree that it preserves the original intent
more directly and provides a clean way to avoid false matches. I
tested the patch on a clean master branch with and without readline
and from my testing it behaves correctly in both configurations
without introducing TAP synchronization issues or regressions.

> The easiest way to fix 030_pager.pl is to just replace ' ' with '*'
> in regex. With readline, everything that we look for is placed on
> its own line so we don't break anything, but --without-readline
> produces the following output (with some hand-written debug info):
>
>         IPC::Run 0000 [#2(438962)]: ** pumping
>         IPC::Run 0000 [#2(438962)]: read( 11 ) = 4 chars '39
>         IPC::Run 0000 [#2(438962)]: '
>
>         stream:postgres=# \pset expanded
>         SELECT generate_series(1,20) as g;
>         Expanded display is on.
>         postgres=# 39
>
> So the output that is turned on with IPCRUNDEBUG=data shows us "we got
> only 4 chars, '39\n\0'!", but in reality we have more stuff in stream,
> so pump_until() function doesn't match with passed regex
>

Regarding 030_pager.pl, thank you for flagging this. I understand this
is a separate issue and I agree it should be handled independently. I
have tested the patch independently by running psql TAP tests in a
--without-readline build with IPCRUNDEBUG=data enabled via make psql
check. While the debug output is largely suppressed when tests pass,
the behavior described by the patch is
consistent with IPC::Run’s incremental read and the test passes
reliably only after relaxing the regex to tolerate fragmented
output. I think this confirms the correctness of the fix. Also tested
with readline enabled, all psql tests continue to pass cleanly and I
did not observe any regressions.
Overall based on the testing, both patches behave correctly in
readline and non-readline builds and address the respective issues
reliably.
Thank you for working on these and please let me know if you would
like me to test any additional scenarios or branches.


Regards,
Soumya

Hi,

On Fri, Jan 16, 2026 at 12:53 PM Oleg Tselebrovskiy
<o.tselebrovskiy@postgrespro.ru> wrote:
>
> Thanks for your answer!
>
> Personally, I can’t think of any more test scenarios that could be
> tested here (maybe some versions of readline library on some exotic
> OS could break, but that seems a little far fetched), so, if you see
> no further problems with the patches, you could change
> the commitfest entry status to "Ready for Committer", so we could get
> another opinion on patches and, if possible, get them committed.
>

Thank you for the clarification.
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.
I am happy to add my test results and observations to the CommitFest
entry to assist further review. Once others have had a chance to look
and review, it would likely be more appropriate to move the entry
forward.


Regards,
Soumya

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

I wrote:
> 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.

On second thought, that's far more complicated than necessary.
It should be sufficient to insert quote marks in the echo commands.

            regards, tom lane

diff --git a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
index 5bd41a278dd..eb2a252a7d7 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);
@@ -155,11 +155,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",
@@ -264,22 +264,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 {
@@ -287,9 +282,12 @@ sub query
         stderr => $self->{stderr},
       } unless !$params{verbose};

-    # 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//;
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

Hi all,

Thank you for the detailed review.

> 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?

I understand the concerns raised and agree that relying purely on
relaxed regex matching creates room for false matches if prompts or
other output get interleaved. Also agree that leveraging psql
semantics to make such matches impossible is a much stronger approach.
The change from timeout() to timer() seems to give better visibility
into partial output on failure and will make testing far easier to
diagnose.
For 030_pager.pl, I agree that relaxing the match is necessary for
no-readline builds, but for that we should avoid depending on fragile
catalog contents making the test self-contained with a controlled
query or view which sounds like the right direction to reduce
long-term risk.

> I wrote:
> > 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.
>
> On second thought, that's far more complicated than necessary.
> It should be sufficient to insert quote marks in the echo commands.

I tried applying both the v3 and v4 patches on a clean branch tree
reset to the current origin/master, but neither of them applied
cleanly due to context mismatches in BackgroundPsql.pm and
030_pager.pl. I think the patches need rebasing on the current master
before they can be tested further. I will be happy to re-test once
rebased patches are available.


Regards,
Soumya

Hi all,


On Mon, Jan 19, 2026 at 2:15 PM Oleg Tselebrovskiy
<o.tselebrovskiy@postgrespro.ru> wrote:
>
> > I tried applying both the v3 and v4 patches on a clean branch tree
> > reset to the current origin/master, but neither of them applied
> > cleanly due to context mismatches in BackgroundPsql.pm and
> > 030_pager.pl. I think the patches need rebasing on the current master
> > before they can be tested further. I will be happy to re-test once
> > rebased patches are available.
>
> Just applied both v3 and v4 onto 34740b9 with no conflicts. I don’t
> see any commits that touch 030_pager.pl and BackgroundPsql.pm
> last few days, so maybe you still have your code changes from
> v1 or v2, or something from debugging v3 or v4? I see that you
> specified clean code tree, but for me both v3 and v4 applied with
> no trouble
>

Thank you for the guidance.
I fetched all refs and tags locally and reset my tree to a clean
origin/master, but I am unable to locate commit 34740b9 in my
repository, so I assume it may be a local or abbreviated hash. On a
fresh branch, both v3 and v4 patches failed to apply cleanly which
suggests a base mismatch rather than an issue with the patches
themselves.
Could you please share the full commit hash or branch that these
patches are based on? I will be happy to apply them again and re-run
the full test coverage once it is available.

Regards,
Soumya

=?UTF-8?B?T2xlZyBUc2VsZWJyb3Zza2l5?= <o.tselebrovskiy@postgrespro.ru> writes:
>> I fetched all refs and tags locally and reset my tree to a clean
>> origin/master, but I am unable to locate commit 34740b9 in my
>> repository

> Maybe your origin is out-of-date or is just different?
> What does ‘​git remote get-url origin’ say in your local repository?

34740b9 is there for me, but it's less than a day old:

commit 34740b90bc123d645a3a71231b765b778bdcf049
Author: Richard Guo <rguo@postgresql.org>
Date:   Mon Jan 19 11:13:23 2026 +0900

    Fix unsafe pushdown of quals referencing grouping Vars

However, none of these patches are touching any recently-modified
code AFAIK, so they should apply cleanly even to a slightly out
of date tree.  I suspect Soumya's failure-to-apply problem has
a different cause.  The patches I posted were just "git diff"
output, without a commit message, so I believe you can't use
"git am" to apply them.  Hoary old "patch -p1" ought to work
though.

            regards, tom lane

=?UTF-8?B?T2xlZyBUc2VsZWJyb3Zza2l5?= <o.tselebrovskiy@postgrespro.ru> writes:
>> 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.)

> I've found your thread about this - [1], and I agree, using
> timer() is better here, we get the stdout and stderr of a timed-out
> query

Thanks for digging that up.  After re-reading that thread I'm feeling
nervous about changing timeout() to timer() in something we need to
back-patch, so I'll leave that change out of the committed patch.
We ought to raise the priority of making that happen, though.

> Also, thanks for making both "pump until" blocks identical, it seemed
> a little strange to have them be different.

Yeah, I couldn't see a reason for that either.

            regards, tom lane

Hi all,

Thank you for confirming the full commit hash.

On Mon, Jan 19, 2026 at 3:16 PM Oleg Tselebrovskiy
<o.tselebrovskiy@postgrespro.ru> wrote:
>
> > Could you please share the full commit hash or branch that these
> > patches are based on? I will be happy to apply them again and re-run
> > the full test coverage once it is available.
>
> The full commit hash is 34740b90bc123d645a3a71231b765b778bdcf049,
> it’s the latest commit in repository on github, master branch, see [1]
>
> > I fetched all refs and tags locally and reset my tree to a clean
> > origin/master, but I am unable to locate commit 34740b9 in my
> > repository
>
> Maybe your origin is out-of-date or is just different?
> What does ‘git remote get-url origin’ say in your local repository?

My local workflow uses a fork repository for convenience, but the
origin is the official PostgreSQL repository. I reset my tree to
34740b90bc123d645a3a71231b765b778bdcf049 commit as per suggested and
have fetched all refs. But still I am unable to apply both v3 and v4
patches cleanly on that commit. Every time it fails with context
mismatches in BackgroundPsql.pm (around line 100), even on a clean
working tree.

error: patch failed: src/test/perl/PostgreSQL/Test/BackgroundPsql.pm:100
error: src/test/perl/PostgreSQL/Test/BackgroundPsql.pm: patch does not apply
error: patch failed: src/bin/psql/t/030_pager.pl:40
error: src/bin/psql/t/030_pager.pl: patch does not apply

Anyway I am trying it and will let you know the status once I complete
applying and testing the patches.


Regards,
Soumya

Hi all,

Thank you for the guidance and clarifications.

> =?UTF-8?B?T2xlZyBUc2VsZWJyb3Zza2l5?= <o.tselebrovskiy@postgrespro.ru> writes:
> >> I fetched all refs and tags locally and reset my tree to a clean
> >> origin/master, but I am unable to locate commit 34740b9 in my
> >> repository
>
> > Maybe your origin is out-of-date or is just different?
> > What does ‘git remote get-url origin’ say in your local repository?
>
> 34740b9 is there for me, but it's less than a day old:
>
> commit 34740b90bc123d645a3a71231b765b778bdcf049
> Author: Richard Guo <rguo@postgresql.org>
> Date:   Mon Jan 19 11:13:23 2026 +0900
>
>     Fix unsafe pushdown of quals referencing grouping Vars
>
> However, none of these patches are touching any recently-modified
> code AFAIK, so they should apply cleanly even to a slightly out
> of date tree.  I suspect Soumya's failure-to-apply problem has
> a different cause.  The patches I posted were just "git diff"
> output, without a commit message, so I believe you can't use
> "git am" to apply them.  Hoary old "patch -p1" ought to work
> though.

I missed the fact that the patches were raw git diff outputs rather
than meant for git am. I will retry applying them on a clean upstream
master tree and re-run the relevant test suites.

> =?UTF-8?B?T2xlZyBUc2VsZWJyb3Zza2l5?= <o.tselebrovskiy@postgrespro.ru> writes:
> >> 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.)
>
> > I've found your thread about this - [1], and I agree, using
> > timer() is better here, we get the stdout and stderr of a timed-out
> > query
>
> Thanks for digging that up.  After re-reading that thread I'm feeling
> nervous about changing timeout() to timer() in something we need to
> back-patch, so I'll leave that change out of the committed patch.
> We ought to raise the priority of making that happen, though.
>
> > Also, thanks for making both "pump until" blocks identical, it seemed
> > a little strange to have them be different.
>
> Yeah, I couldn't see a reason for that either.

I too agree that changing the behavior in a back-patch could be risky
and it makes sense to leave that part out for now while still
addressing the core test robustness issues. I will follow up once I
have re-applied the patches and completed testing.

Regards,
Soumya

Soumya S Murali <soumyamurali.work@gmail.com> writes:
> I too agree that changing the behavior in a back-patch could be risky
> and it makes sense to leave that part out for now while still
> addressing the core test robustness issues. I will follow up once I
> have re-applied the patches and completed testing.

Since we're reaching the end of the January commitfest,
I went ahead and pushed these patches.  There's still time to
revise them if your testing uncovers anything undesirable.

            regards, tom lane

Hi all,


On Sat, Jan 31, 2026 at 1:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Soumya S Murali <soumyamurali.work@gmail.com> writes:
> > I too agree that changing the behavior in a back-patch could be risky
> > and it makes sense to leave that part out for now while still
> > addressing the core test robustness issues. I will follow up once I
> > have re-applied the patches and completed testing.
>
> Since we're reaching the end of the January commitfest,
> I went ahead and pushed these patches.  There's still time to
> revise them if your testing uncovers anything undesirable.


Thank you for the update and for pushing the patches.
I will continue testing on the committed code in both readline and
non-readline configurations and will report back if I notice any
unexpected behavior or regressions.

Regards,
Soumya

Hi all,

I verified the committed fixes on upstream master by running the tests
with and without readline support and confirmed that the tests passed
successfully in both configurations, validating robustness
improvements to BackgroundPsql and pager tests.

Regards,
Soumya