Re: Allow auto_explain to log to NOTICE

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>
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 16:04:44
Message-ID: D8A969F2-4905-44A9-9F68-83CB57A8B962@yesql.se
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

> 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! 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},

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.

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

cheers ./daniel

Attachment Content-Type Size
auto_explain_tests.diff application/octet-stream 7.4 KB
unknown_filename text/plain 5 bytes

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2018-07-17 17:11:12 Re: Allow auto_explain to log to NOTICE
Previous Message Alvaro Herrera 2018-07-17 14:46:08 Re: Another fun fact about temp tables and wraparound