Re: [COMMITTERS] 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

[Just getting back to this.]

Agreed. I have developed the attached patch which passes the strdup()
failure up from pg_fe_getauthname() and maps the failure to
PQconndefaults(), which is now documented as being memory allocation
failure.

FYI, there was odd coding in PQconndefaults() where we set local
variable 'name' to NULL, then we tested to see if it was NULL --- I
removed that test.

> idea that we're ignoring failure returns from pqGetpwuid/GetUserName?

If we want pqGetpwuid/GetUserName to be a special return value, we would
need to modify PQconndefaults()'s API, which doesn't seem worth it.

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

+ Everyone has their own god. +