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.
