Re: Allow auto_explain to log to NOTICE
От | Andrew Dunstan |
---|---|
Тема | Re: Allow auto_explain to log to NOTICE |
Дата | |
Msg-id | a1d0d665-a584-407a-3bae-4cdd90006382@2ndQuadrant.com обсуждение исходный текст |
Ответ на | Re: Allow auto_explain to log to NOTICE (Daniel Gustafsson <daniel@yesql.se>) |
Ответы |
Re: Allow auto_explain to log to NOTICE
(Daniel Gustafsson <daniel@yesql.se>)
|
Список | pgsql-hackers |
On 07/17/2018 12:04 PM, Daniel Gustafsson wrote: >> On 4 Jul 2018, at 15:34, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: >> >> On Wed, Jun 20, 2018 at 2:06 PM, Daniel Gustafsson <daniel@yesql.se> wrote: >>>> On 27 Apr 2018, at 04:24, Andres Freund <andres@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
В списке pgsql-hackers по дате отправления: