From: | Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> |
---|---|
To: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
Cc: | Tom Dunstan <tom(at)tomd(dot)cc>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, jdealmeidapereira(at)pivotal(dot)io |
Subject: | Re: Allow auto_explain to log to NOTICE |
Date: | 2018-07-17 17:11:12 |
Message-ID: | a1d0d665-a584-407a-3bae-4cdd90006382@2ndQuadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 07/17/2018 12:04 PM, Daniel Gustafsson wrote:
>> On 4 Jul 2018, at 15:34, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>>
>> On Wed, Jun 20, 2018 at 2:06 PM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>>> On 27 Apr 2018, at 04:24, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>>>
>>>> On 2018-04-27 11:52:18 +0930, Tom Dunstan wrote:
>>>>>> I'd argue this should contain the non-error cases. It's just as
>>>>>> reasonable to want to add this as a debug level or such.
>>>>> So all of warning, info, debug and debug1-5?
>>>> Yea. Likely nobody will ever use debug5, but I don't see a point making
>>>> that judgement call here.
>>> Did you have a chance to hack up a new version of the patch with Andres’ review
>>> comments? I’m marking this patch as Waiting on Author for now based on the
>>> feedback so far in this thread.
>>>
>> Here is an updated version of the patch. Setting this to "needs review”.
Thanks for the review
> Thanks! Looking at the code, and documentation this seems a worthwhile
> addition. Manual testing proves that it works as expected, and the patch
> follows the expected style. A few small comments:
>
> Since DEBUG is not a defined loglevel, it seems superfluous to include it here.
> It’s also omitted from the documentation so it should probably be omitted from
> here.
>
> + {"debug", DEBUG2, true},
I treated this like we do for client_min_messages and log_min_messages -
the alias is there but I don;t think we document it either.
I don't mind removing it, was just trying to be consistent. It seems odd
that we would accept it in one place but not another.
>
> The <varname> tag should be closed with a matching </varname>.
>
> + <primary><varname>auto_explain.log_level</> configuration parameter</primary>
>
> With those fixed it’s ready for committer.
Thanks, will fix.
>> I haven't added tests for auto_explain - I think that would be useful
>> but it's a separate project.
> Agreeing that this would be beneficial, the attached patch (to apply on top of
> the patch in question) takes a stab at adding tests for this new functionality.
>
> In order to test plan output we need to support COSTS in the explain output, so
> a new GUC auto_explain.log_costs is added. We also need to not print the
> duration, so as a hack this patch omits the duration if auto_explain.log_timing
> is set to off and auto_explain.log_analyze is set off. This is a hack and not
> a nice overloading, but it seems overkill to add a separate GUC just to turn
> off the duration, any better ideas on how support omitting the duration?
>
Great, I'll check it out.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2018-07-17 17:15:08 | Re: PG 10: could not generate random cancel key |
Previous Message | Daniel Gustafsson | 2018-07-17 16:04:44 | Re: Allow auto_explain to log to NOTICE |