Re: Fallout from PQhost() semantics changes

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> On Fri, Aug 3, 2018 at 2:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I think we should do about it is change those internal calls to
>> fetch connhost[].host directly instead of going through PQhost(), as
>> in the attached libpq-internal-PQhost-usage-1.patch.  This will restore
>> the semantics to what they were pre-v10, including erroring out when
>> hostaddr is supplied without host.

> The Attached patch is good and I also verified that it is not missed anymore
> places that needs only a host.

Thanks for reviewing!

>> In fact, I think there's probably a good case for doing something
>> comparable to libpq-internal-PQhost-usage-1.patch all the way back.
>> In exactly what scenario is it sane to be comparing "/tmp" or
>> "localhost" to a server's SSL certificate?

> Yes, I agree that this problem present from a long, but may be till now
> everyone using along with host only?

Yeah, after thinking some more I'm not excited about changing this
pre-v10.  There haven't been any field complaints, and there's also
a technical problem: the older versions didn't insert the default host
value (DEFAULT_PGSOCKET_DIR/DefaultHost) into the data structure but
just substituted it on-the-fly.  So these call sites in fe-auth.c etc
would have to do that too, which seems messy and error-prone.

You could argue perhaps that we shouldn't be making that substitution
at all for fe-auth.c's purposes, but I think it's all right when both
host and hostaddr have been omitted.  The default value then accurately
describes where we're connecting, and it's the same as the behavior
you'd get if you'd written out "host=/tmp" or "host=localhost" explicitly.
So I don't want to touch that behavior --- and indeed, I imagine that the
reason these call sites are using PQhost() is exactly that we wanted that
substitution to happen for these purposes.

Now that the default does get injected into the data structure, but only
when hostaddr isn't supplied, looking directly at the .host field does
exactly what we need in these places.  But we'd need a bit of logic to get
comparable behavior pre-v10, and I don't think it's worth messing with.

			regards, tom lane