Re: Add ldapservice connection parameter
Andrew Jackson <andrewjackson947@gmail.com>
From: Andrew Jackson <andrewjackson947@gmail.com>
To: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
Roman Khapov <rkhapov@yandex-team.ru>, niushiji@gmail.com
Date: 2026-03-29T13:55:54Z
Lists: pgsql-hackers
Attachments
- 0004-Add-ldapservice-connection-parameter.patch (application/x-patch) patch 0004
- 0001-Allow-LDAP-lookup-from-pgservice-connection-paramete.patch (text/x-patch) patch 0001
Laurenz, Thank you for the review. 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. > For the same reason, I am not entirely happy with the name "ldapservice", but I can't think of anything better. I have a few suggestions here. Maybe `remoteserviceuri` would be a better name. This tells the reader that it's a uri and not the name of a service. LDAP is not mentioned because maybe in the future this functionality could be expanded to read connection parameters from an HTTP server, postgres side channel, etc. Alternatively we could bypass the naming altogether and add this functionality into the existing pgservice parameter. I have attached a second patch `0001-Allow-LDAP-lookup-from-pgservice-connection-paramete.patch` that has an implementation of this. The idea being that if you pass a string that starts with ldap:// it will look up the service at the given LDAP uri instead of looking for the corresponding service in the pg_service.conf file. This would be a breaking change for anyone who populated their service names as LDAP uris, I would imagine this would be a very small number of users, if any. It also ends up being a smaller patch overall. > I don't have an LDAP server handy, so I couldn't test the patch On the off chance that it helps, we now have unit tests over the LDAP service functionality. It should automatically start a slapd server, etc. If you add a false assertion in the perl code it should fail the test and also dump a bunch of files into the src/test/ldap directory. You can copy paste the slapd invocation that is left there. I did have to fight with App Armor a bit to get it to be okay with a slapd process reading files in unexpected locations. Thanks again for the review, Andrew Jackson