Abhijit Menon-Sen wrote:
> I read through the patch, and it looks sensible.
Thanks for the thorough review!
> I would have preferred the ldap_simple_bind_s() call in the HAVE_LIBLDAP
> branch to not be inside an else {} (the if block above returns if there
> is an error anyway), but that's a minor point.
I agree, changed.
> I tested the code as follows:
>
> 1. Built the patched source --with-ldap
> 2. Set up ~/.pg_service.conf:
>
> [foo]
> ldap://localhost:3343/cn=dbserver,cn=hosts?pgconnectinfo?base?(objectclass=*)
> ldap://localhost:3443/cn=dbserver,cn=hosts?pgconnectinfo?base?(objectclass=*)
>
> 3. iptables -A INPUT -p tcp -d 127.0.0.1 --dport 3343 -j DROP
> 4. netcat -l 127.0.0.1 3343 ; netcat -l 127.0.0.1 3443
> 5. PGSERVICE=foo bin/psql
>
> psql did connect to localhost:3443 after a few seconds of trying to
> connect to :3343 and failing. (I tried without the iptables rule, so
> I know that it does try to connect to both.)
>
> This doesn't seem to handle timeouts in the sense of a server that
> doesn't respond after you connect (or perhaps the timeout was long
> enough that it outlasted my patience), but that's not the fault of
> this patch, anyway.
That's a bug.
I thought that setting LDAP_OPT_NETWORK_TIMEOUT would also
take care of an unresponsive server, but it seems that I was wrong.
The attached patch uses ldap_simple_bind / ldap_result to handle
this kind of timeout (like the original code).
> I can't say anything about the patch on Windows, but since Magnus seemed
> to think it was OK, I'm marking this ready for committer.
The Windows part should handle all kinds of timeouts the same.
Yours,
Laurenz Albe
