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 |
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 |