Thread: pgsql: libpq: change PQconndefaults() to ignore invalid service files
libpq: change PQconndefaults() to ignore invalid service files Previously missing or invalid service files returned NULL. Also fix pg_upgrade to report "out of memory" for a null return from PQconndefaults(). Patch by Steve Singer, rewritten by me Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/9e0a97f1c8316e36fa4a8626e0a60792b0fb0c2e Modified Files -------------- contrib/pg_upgrade/server.c | 3 +++ doc/src/sgml/libpq.sgml | 3 ++- src/interfaces/libpq/fe-auth.c | 2 +- src/interfaces/libpq/fe-auth.h | 2 +- src/interfaces/libpq/fe-connect.c | 27 ++++++++++++++++----------- 5 files changed, 23 insertions(+), 14 deletions(-)
Bruce Momjian <bruce@momjian.us> writes:
> libpq: change PQconndefaults() to ignore invalid service files
> Previously missing or invalid service files returned NULL. Also fix
> pg_upgrade to report "out of memory" for a null return from
> PQconndefaults().
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.
regards, tom lane
On Tue, Dec 3, 2013 at 11:30:15AM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > libpq: change PQconndefaults() to ignore invalid service files > > Previously missing or invalid service files returned NULL. Also fix > > pg_upgrade to report "out of memory" for a null return from > > PQconndefaults(). > > 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. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.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?
regards, tom lane
On Tue, Dec 3, 2013 at 11:42:08AM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +