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

Soumya S Murali <soumyamurali(dot)work(at)gmail(dot)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