Re: 001_password.pl fails with --without-readline

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