> 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 worthwhileaddition. Manual testing proves that it works as expected, and the patchfollows 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 fromhere.

> 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 ofthe 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, soa new GUC auto_explain.log_costs is added. We also need to not print theduration, so as a hack this patch omits the duration if auto_explain.log_timingis set to off and auto_explain.log_analyze is set off. This is a hack and nota nice overloading, but it seems overkill to add a separate GUC just to turnoff the duration, any better ideas on how support omitting the duration?