Thread

Add ldapservice connection parameter

Andrew Jackson <andrewjackson947@gmail.com> — 2026-01-12T02:50:39Z

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.

Re: Add ldapservice connection parameter

Steven Niu <niushiji@gmail.com> — 2026-01-12T06:08:17Z

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

Re: Add ldapservice connection parameter

Roman Khapov <rkhapov@yandex-team.ru> — 2026-01-12T10:05:53Z

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

Re: Add ldapservice connection parameter

Andrew Jackson <andrewjackson947@gmail.com> — 2026-01-12T23:53:14Z

Steven and Roman, Thank you for the review.

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

On Mon, Jan 12, 2026 at 4:06 AM Roman Khapov <rkhapov@yandex-team.ru> wrote:
>
> 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

Re: Add ldapservice connection parameter

Andrew Jackson <andrewjackson947@gmail.com> — 2026-03-22T23:38:41Z

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:
>
> Steven and Roman, Thank you for the review.
>
> Here is an updated patch that addresses both of these issues.
>
> On Mon, Jan 12, 2026 at 4:06 AM Roman Khapov <rkhapov@yandex-team.ru> wrote:
> >
> > 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

Re: Add ldapservice connection parameter

Laurenz Albe <laurenz.albe@cybertec.at> — 2026-03-27T22:48:00Z

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

Re: Add ldapservice connection parameter

Andrew Jackson <andrewjackson947@gmail.com> — 2026-03-29T13:55:54Z

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

Re: Add ldapservice connection parameter

Andrey Borodin <x4mmm@yandex-team.ru> — 2026-03-30T11:52:20Z

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.

Re: Add ldapservice connection parameter

Andrew Jackson <andrewjackson947@gmail.com> — 2026-03-30T17:32:20Z

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

Re: Add ldapservice connection parameter

Laurenz Albe <laurenz.albe@cybertec.at> — 2026-03-31T09:08:10Z

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

Re: Add ldapservice connection parameter

Andrew Jackson <andrewjackson947@gmail.com> — 2026-04-01T23:07:47Z

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

Re: Add ldapservice connection parameter

Laurenz Albe <laurenz.albe@cybertec.at> — 2026-04-02T14:55:02Z

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

Re: Add ldapservice connection parameter

Andrew Jackson <andrewjackson947@gmail.com> — 2026-04-07T16:20:20Z

Hi,

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

Updated patch to only append an error message when rc from
ldapServiceLookup is 2. This is the only non zero return of
ldapServiceLookup that does not append an error message because it is
meant to signal that the lookup was not successful but we can continue
parsing the service file. In this case I think it makes sense to raise
an error and append an error message. Also increased the specificity
of the test assertions.

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

I added this because now that there is a connection parameter that
begins with ldap it could cause a confusing error message if the user
tries to use ldapserviceurl in a service file and are then given an
error message that roughly says 'invalid LDAP URL ".*" scheme must be
ldap://'.

Thanks,
Andrew Jackson

Re: Add ldapservice connection parameter

Zsolt Parragi <zsolt.parragi@percona.com> — 2026-04-08T07:59:33Z

Hello

+ /*
+ * ldapServiceLookup has 4 potential return values. We only care here
+ * if it succeeded, if it failed we dont care why, return failure.
+ */
+ if ((rc = ldapServiceLookup(ldapserviceurl, options, errorMessage)) != 0){
+
+ /*
+ * ldapServiceLookup == 2 is the only return code for libpq_append_error
+ * that does not append error because when used in pg_service.conf it is
+ * allowed to fallback to additional URLs without failing.
+ */
+ if (rc == 2)
+ libpq_append_error(errorMessage,
+            "connection could not be established to ldapserviceurl: \"%s\"",
+    ldapserviceurl);
+
+ return false;

This comment seems to be confusing to me, at first I thought that it
is the opposite of what the code below does, and then I realized that
no, it's just difficult to understand.

Maybe something like:

  /*
   * ldapServiceLookup() return code 2 means the LDAP server could
   * not be contacted. Unlike other non-zero returns, it does not
   * append an error message, because in pg_service.conf parsing
   * the caller silently falls back to the next URL. Here there is
   * no fallback, so we must provide an error message ourselves.
   */

+        This option specifies an LDAP query that can be used to
reference connection parameters
+        stored on an LDAP server. Any connection parameter that is
looked up in this way is
+        overridden by explicitly named connection parameters or
environment variables. This

Is the environment variable part true? ldapServiceLookup is now at
line 6765, environment variables are handled later at 6794 in
conninfo_add_defaults, so it is later, but it also has a NULL check in
it. If a value is already set in ldapServiceLookup, the environment
variable loop later won't override it.

Re: Add ldapservice connection parameter

Andrew Jackson <andrewjackson947@gmail.com> — 2026-04-08T16:31:54Z

Thank you for catching that. Attached patch adds your comment verbatim
and fixes the docs that previously contained incorrect info about the
envvar override.

I also added 2 new tests: one that validates that LDAP lookups
override envvars and another that tests the stderr when parsing an
LDAP entry with an unterminated quote in the option value.
Additionally I made some minor changes in other comments and in tests.

Thanks,
Andrew Jackson

On Wed, Apr 8, 2026 at 2:59 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> Hello
>
> + /*
> + * ldapServiceLookup has 4 potential return values. We only care here
> + * if it succeeded, if it failed we dont care why, return failure.
> + */
> + if ((rc = ldapServiceLookup(ldapserviceurl, options, errorMessage)) != 0){
> +
> + /*
> + * ldapServiceLookup == 2 is the only return code for libpq_append_error
> + * that does not append error because when used in pg_service.conf it is
> + * allowed to fallback to additional URLs without failing.
> + */
> + if (rc == 2)
> + libpq_append_error(errorMessage,
> +            "connection could not be established to ldapserviceurl: \"%s\"",
> +    ldapserviceurl);
> +
> + return false;
>
> This comment seems to be confusing to me, at first I thought that it
> is the opposite of what the code below does, and then I realized that
> no, it's just difficult to understand.
>
> Maybe something like:
>
>   /*
>    * ldapServiceLookup() return code 2 means the LDAP server could
>    * not be contacted. Unlike other non-zero returns, it does not
>    * append an error message, because in pg_service.conf parsing
>    * the caller silently falls back to the next URL. Here there is
>    * no fallback, so we must provide an error message ourselves.
>    */
>
> +        This option specifies an LDAP query that can be used to
> reference connection parameters
> +        stored on an LDAP server. Any connection parameter that is
> looked up in this way is
> +        overridden by explicitly named connection parameters or
> environment variables. This
>
> Is the environment variable part true? ldapServiceLookup is now at
> line 6765, environment variables are handled later at 6794 in
> conninfo_add_defaults, so it is later, but it also has a NULL check in
> it. If a value is already set in ldapServiceLookup, the environment
> variable loop later won't override it.