Fabien COELHO <coelho@cri.ensmp.fr> writes:
> The mention of possible reverse dns queries has been removed... but I do
> not think there was any before?
Yeah, that's why I took it out. If there ever were any reverse lookups in
libpq, we got rid of them AFAICS, so this statement in the docs seemed just
unnecessarily confusing to me.
> I read the rational of the host/hostaddr artificial mapping. I cannot say
> I'm thrilled with the result: I do not really see a setting where avoiding
> a DNS query is required but which still needs a hostname for auth... If
> you have GSSAPI or SSPI then you have an underlying network, in which a
> dns query should be fine.
The point is that you might wish to avoid a DNS query for speed reasons,
not because it's "required" to do so. Or, perhaps, you did the DNS
query already using some async DNS library, and you want to pass the
result on to PQconnectStart so it will not block.
> STM that we should have had only "host" for specifying the target (name,
> ip, path), and if really necessary an ugly "authname" directive to provide
> names on the side. I know, too late.
> I think that in the string format host=foo:5433,bla:5432 could be
> accepted as it is in the URL format.
I'm uninterested in these proposals, and they certainly are not something
to include in this particular patch even if I were.
Also, it's too late to redefine the meaning of colon in a host string,
since as you mentioned that could already be an IPv6 address.
>>> I'd consider wrapping some of the logic. I'd check the port first, then
>>> move the host resolution stuff into a function.
>> Don't really see the value of either ...
> What I see is that the host logic is rather lengthy and specific (it
> depends on the connection type -- name, ip, socket including a so nice
> #ifdef), distinct from the port stuff which is dealt with quite quickcly.
> By separating the port & host and putting the lengthy stuff in a function
> the scope of the for loop on potential connections would be easier to read
> and understand, and would not require to see CHT_ cases to understand what
> is being done for managing "host" variants, which is a mere detail.
I still don't see the value of it. This code was inlined into the calling
function before, and it still is with this patch --- just a different
calling function. Maybe there's an argument for a wholesale refactoring
of PQconnectPoll into smaller pieces, but that's not a task for this patch
either. (I'm not really convinced it'd be an improvement anyway, at least
not enough of one to justify creating so much back-patching pain.)
regards, tom lane
