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.
