On 07/17/2018 12:04 PM, Daniel Gustafsson wrote:
On 4 Jul 2018, at 15:34, Andrew Dunstan <andrew.duns...@2ndquadrant.com> wrote:
On Wed, Jun 20, 2018 at 2:06 PM, Daniel Gustafsson <dan...@yesql.se> wrote:
On 27 Apr 2018, at 04:24, Andres Freund <and...@anarazel.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
