public inbox for [email protected]  

help / color / mirror / Atom feed

From: Laurenz Albe <[email protected]>
To: Andrew Jackson <[email protected]>
To: pgsql-hackers <[email protected]>
Cc: Roman Khapov <[email protected]>
Cc: [email protected]
Subject: Re: Add ldapservice connection parameter
Date: Fri, 27 Mar 2026 23:48:00 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAKK5BkE_+rCKgZz7+VNNfH_Jm1H6=RcHxg8mmojyR8pu-LOh5w@mail.gmail.com>
References: <CAKK5BkFOFGfKJNbTuYBvE0PfpHmW8iZEmdNogaCYqjAOhtNgDg@mail.gmail.com>
	<[email protected]>
	<CAKK5BkFxWnddC2=mbHpojWnOLe=x3vLftaMUkO3ocJwqZN7Tug@mail.gmail.com>
	<CAKK5BkE_+rCKgZz7+VNNfH_Jm1H6=RcHxg8mmojyR8pu-LOh5w@mail.gmail.com>

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

view thread (10+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add ldapservice connection parameter
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox