Add ldapservice connection parameter

From: Andrew Jackson <andrewjackson947@gmail.com>
Sent: Monday, January 12, 2026 10:50
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Add ldapservice connection parameter

Currently there exists, only in pg_service.conf, the ability to look
up connection parameters from a centralized LDAP server. This patch
expands the usability of this by allowing it to be specified directly in
a connection string instead of only in a pg_service.conf file.

Hi, Andrew,

I have one question, if LDAP has higher priority, we should put the its code to be before "service" logic, like:

+    if (ldapservice != NULL && strncmp(ldapservice, "ldap", 4) == 0) {
+          if (!ldapServiceLookup(ldapservice, options, errorMessage))
+              return 0;  // return if LDAP processing succeeds. 

/*
* We have to special-case the environment variable PGSERVICE here, since
* this is and should be called before inserting environment defaults for
* other connection options.
*/
if (service == NULL)
service = getenv("PGSERVICE");

/* If no service name given, nothing to do */
if (service == NULL)
return 0;

This will be consistent with how PG processes the pg_service.conf file.
See https://www.postgresql.org/docs/18/libpq-ldap.html
"Processing of pg_service.conf is terminated after a successful LDAP lookup, but is continued if the LDAP server cannot be contacted."

Thanks,
Steven

Hi!

Thanks for your patch!

Adding to the one Steven wrote, I noticed one typo in the patch:

@@ -2337,7 +2337,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
       <term><literal>ldapservice</literal></term>
       <listitem>
        <para>
-        This option specifies an LDAP query that can be used to reference connection paremeters
+        This option specifies an LDAP query that can be used to reference connection parameters
         stored in an LDAP server. This functionality is described in more detail in <xref linkend="libpq-ldap"/>.
         </para>

paremeters -> parameters

--
Best regards,
Roman Khapov

Steven and Roman, Thank you for the review.

Here is an updated patch that addresses both of these issues.

Hi,

Noticed 1 variable that was unused during non-LDAP builds. Tested
locally and did not see the error/warning. Also some minor cleanup
(comments, definition placement, etc).

Thanks,
Andrew Jackson

On Mon, Jan 12, 2026 at 5:53 PM Andrew Jackson
<andrewjackson947@gmail.com> wrote:

On Sun, 2026-03-22 at 18:38 -0500, Andrew Jackson wrote:

Noticed 1 variable that was unused during non-LDAP builds. Tested
locally and did not see the error/warning. Also some minor cleanup
(comments, definition placement, etc).

I don't have an LDAP server handy, so I couldn't test the patch,
but I read through it.

I think that this is a useful addition.

+#ifdef USE_LDAP
+   if (ldapservice != NULL)
+       if (strncmp(ldapservice, "ldap", 4) == 0)
+           if (!ldapServiceLookup(ldapservice, options, errorMessage))
+               return 0;
+#endif

I think that the test if the string starts with "ldap" is midguided.
It was probably copied from the other call site of ldapServiceLookup(),
but there it is necessary, because an entry in the connection service
file could start with something else.

A value for the parameter "ldapservice" that doesn't start with "ldap"
should cause an error.

ldapServiceLookup() checks if the string starts with "ldap://" and
throws an error if it doesn't. So I'd say that you should simply
remove the test if the string starts with "ldap".

I also think that it is wrong to return 0 (success) if
ldapServiceLookup() fails. Why should it be OK to specify a bad
LDAP URL?

I don't understand why that code is in parseServiceInfo().
After all, it has nothing to do with a connection service file.
Calling it from conninfo_add_defaults() would make more sense to me.
For the same reason, I am not entirely happy with the name
"ldapservice", but I can't think of anything better.

The way your code is now, "ldapservice" sets default values that
get overridden by explicitly named parameters. I think the
documentation should mention that.

Yours,
Laurenz Albe

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

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 to understand.

That were all my random thoughts about the patch. Thanks!

Best regards, Andrey Borodin.

Andrey,

Thank you for the review.

I like "ldapservice" approach more. I don't like breaking changes, even for very small number of users.

After thinking about it more it would be pretty easy to make this non
breaking for users. The attached patch
`pgservice-0002-Allow-LDAP-lookup-from-pgservice-connection-paramete.patch`
implements this. Basically if the ldap connection parameter happens to
already exist in pg_service, no LDAP lookup is performed, the values
are instead taken from the pg_service.conf file. I'm fine with either
implementation direction that is the most simple though.

Also attaching the patch
`ldapserviceurl-0005-Add-ldapservice-connection-parameter.patch` which
addresses some issues you found with the previous patch.
- renames parameter from ldapservice to ldapserviceurl
- Adds a comment above the ldapserviceurl call explaining the reason
for ignoring any error info besides zero/non-zero
- Changes dispsize to 64. In reality it maybe should be higher but I
was not comfortable changing it to something like 1024 because it was
an order of magnitude larger than anything else. Just set it to the
current max of 64.
- Also moved the parameter definition and added comments about how it
is defined even on non LDAP builds. This coincides with similar
comments for SSL/GSS specific parameters, though there are no such
comments left for oauth.

Let me know if there is anything else you see missing.

Thanks again,
Andrew Jackson

The naming of your patches is confusing and diverges from the accepted
style: 0001, 0002 etc. are not supposed to be the patch versions, but
the sequence in a patch set. The version number of the patches should
be before that, like v5-0001-First-patch-version-5.patch

This may confuse the commitfest bot that automatically tests the
patches, and indeed it is failing right now.

On Sun, 2026-03-29 at 08:55 -0500, Andrew Jackson wrote:

- 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

In my opinion, "service" pertains to the connection service file,
as described in libpq-pgservice.html. Your proposed patch defines an
alternative to the service file that also specifies default parameters.

- A failed parseServiceInfo() now returns false in conninfo_add_defaults()

Good, but you should append something to the "errorMessage", like
conninfo_add_defaults() does elsewhere.

- Made documentation a bit more explicit.

Good. There is at least one thing still lacking, in my opinion.
The chapter on "LDAP Lookup of Connection Parameters" (32.18 in v18)
still talks exclusively about the connection service file. E.g.,

LDAP connection parameter lookup uses the connection service file
pg_service.conf (see Section 32.17).

You could append "... or the XY connection parameter". Other parts
of that page will need to get modified too.

Also I added support for specifying PGLDAPPSERVICE in the form of an
env var as well. Also added additional tests.

Good about the environment variable - I didn't think of that.

About the tests: there is one test "connection fails with ldapserviceurl
specified in pg_service.conf file", but it doesn't look like it is doing
what it says. Perhaps I am missing something.

I think there should be more tests for failure. Some of my complaints
with the patch were about handling failure cases, and I think you would
have noticed them if you had had regression 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.

I get your point about future extensions of the functionality, but having
"LDAP" in the name is a useful clue. I see no problem with a
"httpserviceurl" next to a "ldapserviceurl".

The name "ldapserviceurl" still contains the "service".
What about a simple "ldapurl"? Or, to hint at the purpose, something like
"default_value_ldap_url"? Well, perhaps we cannot find anything better
than "ldapserviceurl". I won't fight about it.

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 am not fond of the idea. I think it is confusing to repurpose an existing
parameter like that. This feeling of confusion is confirmed by what you
wrote in your most recent mail on the thread:

After thinking about it more it would be pretty easy to make this non
breaking for users. The attached patch
`pgservice-0002-Allow-LDAP-lookup-from-pgservice-connection-paramete.patch`
implements this. Basically if the ldap connection parameter happens to
already exist in pg_service, no LDAP lookup is performed, the values
are instead taken from the pg_service.conf file. I'm fine with either
implementation direction that is the most simple though.

I read through that paragraph, but failed to understand it.
Let's keep things simple.

Yours,
Laurenz Albe

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't look 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

On Wed, 2026-04-01 at 18:07 -0500, Andrew Jackson wrote:

Attached is an updated patch.

Thank you, it applies and passes the regression tests.

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.

Oh, I didn't look at that. If ldapServiceLookup() already appends a
meaningful error message, there is no need to add another one.

The documentation is better now.

--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
#ifdef USE_LDAP
-               if (strncmp(line, "ldap", 4) == 0)
+               /*
+                * Is this a potential ldapurl or a ldapserviceurl parameter?
+                */
+               if (strncmp(line, "ldap:", 5) == 0)
{
int         rc = ldapServiceLookup(line, options, errorMessage);

That change does not really belong to the patch, but is alright by me.

I'll mark the patch as "ready for committer".

Yours,
Laurenz Albe