Re: Log LDAP "diagnostic messages"?

From: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
To: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Christoph Berg <myon(at)debian(dot)org>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Log LDAP "diagnostic messages"?
Date: 2017-09-24 11:00:02
Message-ID: CAEepm=03vDdAVraazxHc52CM0V1BJCimYX9Y747KqSqpvGdkiQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> In the 0001 patch, I would move the ldap_unbind() calls after the
> ereport(LOG) calls. We do all the other resource cleanup (pfree() etc.)
> after the ereport() calls, so it would be weird to do this one
> differently. Also, in the second patch you move one of the
> ldap_unbind() calls down anyway.

Fair point. In that case there are a few others we should consider
moving down too for consistency, like in the attached.

> In the 0002 patch, I think this is a bit repetitive and could be
> refactored even more. The end result could look like
>
> ereport(LOG,
> (errmsg("blah"),
> errdetail_for_ldap(ldap)));
> ldap_unbind(ldap);

Thanks, that is much tidier. Done that way in the attached.

Here also is a small addition to your TAP test which exercises the
non-NULL code path because slapd rejects TLS by default with a
diagnostic message. I'm not sure if this is worth adding, since it
doesn't actually verify that the code path is reached (though you can
see that it is from the logs).

--
Thomas Munro
http://www.enterprisedb.com

Attachment Content-Type Size
0001-Improve-LDAP-cleanup-code-in-error-paths.patch application/octet-stream 4.4 KB
0002-Log-diagnostic-messages-if-errors-occur-during-LDAP-.patch application/octet-stream 4.1 KB
0003-Add-a-regression-test-to-log-an-LDAP-diagnostic-mess.patch application/octet-stream 1.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2017-09-24 12:04:46 Re: ICU locales and text/char(n) SortSupport on Windows
Previous Message Alvaro Hernandez 2017-09-24 10:36:56 Re: Built-in plugin for logical decoding output