Re: libpq should append auth failures, not overwrite

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
>> As I explained in my comments, the reason I did not do these things
>> is that I didn't want to change the output for cases in which just a
>> single host name is given. I still don't.

> Ok, I get your argument when there is just one target server (cluster),
> which is probably at least 99.9% of the use case in practice.
> However, ISTM multiple ip & multiple hostname look pretty close.
> I guess that the number of people that use these features is small, but
> for these when things go wrong better messages are useful... so I would
> see no harm to trigger the server introductory line when there are
> multiples servers, whatever the reason why there are multiples servers.

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, and I think that would inevitably give rise to
more complaints than kudos. So I'm still of the opinion that we're better
off with the second patch's behavior as it stands.

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. In
particular, the messiness around suppressing extraneous complaints from
lo_close is still there, if anything worse than before (it's hard to
get rid of because lo_close will reset the buffer). I'd rather just
leave fe-lobj.c alone, possibly adding a header comment explaining why
it's different. Which is basically because none of those functions
expect to be appending multiple errors; they're all gonna quit after
the first one.

> I agree with your suggestion of adding a function trying to preserve
> messages on OOM errors if possible. Maybe "appendPQExpBufferOOM"?

Yeah, done.

> 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.

> In passing, some OOM message are rather terse: "out of memory\n", other
> are more precise "out of memory allocating GSSAPI buffer (%d)\n".

I got rid of the latter; I think they're unnecessary translation burdens,
and it'd be hard to fit them into a one-size-fits-all OOM reporting
subroutine, and it's pretty unclear what's the point of special-casing
them anyway.

> 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.

regards, tom lane