Attached is an updated patch.
> I think there should be more tests for failure.
This is fixed. I have added a new suite of tests that cover various
(but not exhaustive) failure scenarios within ldapServiceLookup. Prior
to this patch there were no tests for failure within ldapServiceLookup
so I think it's an improvement.
I have also added 5 additional tests to validate corner cases around
the new ldapserviceurl functionality.
> Good, but you should append something to the "errorMessage", like
conninfo_add_defaults() does elsewhere.
Added an error append in this patch. It has the impact of printing 2
lines of errors on failure though: one from the newly appended message
and one from ldapServiceLookup. Not sure if there are other examples
of this behavior in libpq. Will look into this more tommorow.
> About the tests: there is one test "connection fails with ldapserviceurl specified in pg_service.conf file", but it
doesn'tlook like it is doing what it says.
You are correct, this test was incorrect. All of the tests should now
have expected_stdout/stderr conditions so they should be a lot more
precise now.
> The version number of the patches should be before that, like v5-0001-First-patch-version-5.patch
Understood. All future patches will be attached in this manner.
> I think it is confusing to repurpose an existing parameter like that.
Understood. Will stick to the idea of using a new connection parameter.
Thanks,
Andrew Jackson
