Message-ID: <ba65c0452e7f8218b23f958e4099f98cbabe8bf8.camel@cybertec.at>
Subject: Re: Add ldapservice connection parameter
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Andrew Jackson <andrewjackson947@gmail.com>, pgsql-hackers
	 <pgsql-hackers@postgresql.org>
Cc: Roman Khapov <rkhapov@yandex-team.ru>, niushiji@gmail.com
Date: Fri, 27 Mar 2026 23:48:00 +0100
In-Reply-To: 
 <CAKK5BkE_+rCKgZz7+VNNfH_Jm1H6=RcHxg8mmojyR8pu-LOh5w@mail.gmail.com>
References: 
	<CAKK5BkFOFGfKJNbTuYBvE0PfpHmW8iZEmdNogaCYqjAOhtNgDg@mail.gmail.com>
	 <9FF1E1E8-D9EE-43C5-ADFA-7AE94BA57159@yandex-team.ru>
	 <CAKK5BkFxWnddC2=mbHpojWnOLe=x3vLftaMUkO3ocJwqZN7Tug@mail.gmail.com>
	 <CAKK5BkE_+rCKgZz7+VNNfH_Jm1H6=RcHxg8mmojyR8pu-LOh5w@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.58.3 (3.58.3-1.fc43) 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/ba65c0452e7f8218b23f958e4099f98cbabe8bf8.camel%40cybertec.at>
Precedence: bulk

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 !=3D NULL)
+       if (strncmp(ldapservice, "ldap", 4) =3D=3D 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