Re: pgsql: libpq: change PQconndefaults() to ignore invalid service files

On Tue, Dec 3, 2013 at 11:42:08AM -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > On Tue, Dec 3, 2013 at 11:30:15AM -0500, Tom Lane wrote:
> >> Why does this patch remove the errorMessage argument from
> >> pg_fe_getauthname? I gauge the amount of thought that went into
> >> that choice by the fact that the comment wasn't updated.
>
> > Oh, the argument was not used, so I remove it. C comment updated.
> > Thanks.
>
> My point was that I didn't think you'd thought about error handling.
>
> In particular, it appears to me that if the strdup in pg_fe_getauthname
> fails, we'll just let that pass without comment, which is inconsistent
> with all the other out-of-memory cases in conninfo_add_defaults.
> (I wonder whether any code downstream of this supposes that we always
> have a value for the "user" option. It's a pretty safe bet that the
> case is hardly ever tested.)
>
> More generally, why is it that we'd want to eliminate any prospect
> of reporting other errors in pg_fe_getauthname? Is it such a great
> idea that we're ignoring failure returns from pqGetpwuid/GetUserName?

Great question. I was not happy we were ignoring service file errors in
PQconndefaults() either, though that is an external function so changing
it is more of an issue.

The issue I had was that PQconndefaults() was calling a function that
called pg_fe_getauthname(), and since the argument was useless anyway, I
just remove it.

I am happy to go the other way and try to propogate things up. One
problem is that this is going to be another case that PQconndefaults()
has to be documented as ignoring.

Should I work on a patch?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +