Re: Add ldapservice connection parameter - Mailing list pgsql-hackers

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