The patch seems useful, small and nice.
> On 29 Mar 2026, at 18:55, Andrew Jackson <andrewjackson947@gmail.com> wrote:
>
> Included with this email are 2 patches.
> `0004-Add-ldapservice-connection-parameter.patch` contains all of the
> changes you recommended:
> - Removed the redundant/unneeded check for the string ldap
> - The function has now been moved from parseServiceInfo() to
> conninfo_add_defaults().
> - I will say though the reason I included this in parseServiceInfo()
> was I consider this new LDAP functionality and the existing
> pg_service.conf() functionality all different ways of accessing a
> postgres "service". I thought it made sense to handle all service
> parsing in a single function. Happy to keep it moved it if I was
> incorrect about this
> - A failed parseServiceInfo() now returns false in conninfo_add_defaults()
> - Made documentation a bit more explicit.
>
> Also I added support for specifying PGLDAPPSERVICE in the form of an
> env var as well. Also added additional tests.
I like "ldapservice" approach more. I don't like breaking changes, even for very small number of users.
Consider names like ldapurl (used by HBA), ldapserviceurl or ldapuri.
Also dispsize == 20 may be small for URL. (in PQconninfoOptions[])
ldapServiceLookup() returns many values:
* Returns
* 0 if the lookup was successful,
* 1 if the connection to the LDAP server could be established but
* the search was unsuccessful,
* 2 if a connection could not be established, and
* 3 if a fatal error occurred.
are you sure in
In parseServiceFile() we use return code differently. Probably, you know what you are doing, but a comment would help
tounderstand.
That were all my random thoughts about the patch. Thanks!
Best regards, Andrey Borodin.
