Re: pg_upgrade segfaults when given an invalid PGSERVICE value

On Mon, Mar 25, 2013 at 10:59:21AM -0400, Steve Singer wrote:
> On 13-03-20 05:54 PM, Tom Lane wrote:
> >Steve Singer <ssinger@ca.afilias.info> writes:
> 
> >
> >>  From a end-user expectations point of view I am okay with somehow
> >>marking the structure returned by PQconndefaults in a way that the
> >>connect calls will later fail.
> >
> >Unless the program changes the value of PGSERVICE, surely all subsequent
> >connection attempts will fail for the same reason, regardless of what
> >PQconndefaults returns?
> >
> >            regards, tom lane
> >
> >
> 
> So your proposing we do something like the attached patch?  Where we
> change conninfo_add_defaults to ignore an invalid PGSERVICE if being
> called by PQconndefaults() but keep the existing behaviour in other
> contexts where it is actually being used to establish a connection?
> 
> In this case even if someone takes the result of PQconndefaults and
> uses that to build connection options for a new connection it should
> fail when it does the pgservice lookup when establishing the
> connection. That sounds reasonable to me.

I was just about to look at this --- thanks for doing the legwork.  Your
fix is for conninfo_add_defaults() to conditionally fail, and that is a
logical approach.

One big problem I see is that effectively we have made
conninfo_add_defaults() to conditionally fail based on a missing service
(ignore_missing_service), and I think that reinforced my feeling that
having PQconndefaults() not fail on a missing service is just an awkward
approach to the problem.

We are taking this approach because PQconndefaults() doesn't have an API
to return the error cause, while other API calls do.  Returning true so
we can later report the right error from a later API call just feels
wrong.

However, I am also unclear about what alternative to suggest, except to
sprinkle a "possible service problem" message to all the call failure.

If we do want to the route of the patch, we should document this in the
docs and C code so it is clear why we went in this direction.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +