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

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