Re: libpq should append auth failures, not overwrite

Hello Tom,

> The thing is that there are a *lot* of systems nowadays on which localhost
> maps to both ipv4 and ipv6, so that "host=localhost" would be enough to
> trigger the new behavior,

I think that people would survive having the ip spelled out on localhost
errors when there are several ips associated to the name.

You could also create an exception for "localhost" if you see it as a
large problem, but if someone redefined localhost somehow (I did that once
by inadvertence), it would be nice to help and be explicit.

> and I think that would inevitably give rise to more complaints than
> kudos.

Hmmm. I'm not sure about what the complaint could be in case of multiple
ips. "Hey, I have a 'host' with multiple ips, and on errors you tell me
which ip generated an issue, and I do not want to know, really".

> So I'm still of the opinion that we're better off with the
> second patch's behavior as it stands.

This was a mere suggestion.

As a user, and as a support person for others on occasions, I like precise
error messages which convey all relevant information. In this instance a
key information is hidden, hence the proposal.

> Responding to your other points from upthread:
>
>> There are still 86 instances of "printfPQExpBuffer", which seems like a
>> lot. They are mostly OOM messages, but not only. This make checking the
>> patch quite cumbersome.
>> I'd rather there would be only one rule, and clear reset with a comment
>> when needded (eg "fe-lobj.c").
>
> Well, I did this for discussion's sake, but I don't really find the
> resulting changes in fe-lobj.c to be any sort of improvement. [...]

The improvement I see is that if there would not be single remaining
printf, so it is easy to check that no case were forgotten in libpq, and
for future changes that everywhere in libpq there is only one rule which
is "append errors to expbuf", not "it depends on the file or function you
are in, please look at it in detail".

>> It is unclear to me why the "printf" variant is used in
>> "PQencryptPasswordConn" and "connectOptions2", it looks that you
>> skipped these functions.
>
> I did because it seemed like unnecessary extra diff content, since
> we're expecting the error buffer to be initially empty anyway (for
> connectOptions2) or we'd just need an extra reset (for
> PQencryptPasswordConn). In the attached I went ahead and made those
> changes, but again I'm unsure that it's really a net improvement.

Same as above: easier to check, one simple rule for all libpq errors.

>> I do not like the resulting output
>> server:
>> problem found
>> more details
>> I'd rather have
>> server: problem found
>> more details
>
> Done in the attached, although I'm concerned about the resulting effects
> for error message line length --- in my tests, lines that used to reliably
> fit in 80 columns don't anymore. Again, I'm not really convinced this is
> an improvement. It would absolutely *not* be an improvement if we tried
> to also shoehorn the server's IP address in there; that'd make the lines
> way too long, with way too much stuff before the important part.

I understand your concern about line length.

A possible compromise would be to newline *and* indent before "problem
found". To avoid messy code, there could be an internal function to manage
that, eg.

appendErrorContext(...), for "server:"
appendError(exbuf, fmt, ...) , for errors, which would indent the
initial line by two spaces.

server1:
problem found
details
server2:
problem found
details

This would also give room for the ip on a 80-column server line.

The alternative is that the committer does as they want, which is also
fine with me:-)

--
Fabien.