The patch seems useful, small and nice. > On 29 Mar 2026, at 18:55, Andrew Jackson <[email protected]> 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 to understand. That were all my random thoughts about the patch. Thanks! Best regards, Andrey Borodin.
