On Fri, Mar 11, 2016 at 3:03 PM, Magnus Hagander <mag...@hagander.net> wrote:
> > > On Fri, Mar 11, 2016 at 2:51 PM, Julien Rouhaud <julien.rouh...@dalibo.com > > wrote: > >> On 11/03/2016 11:45, Magnus Hagander wrote: >> > >> > Coming back to the previous discussions about random() - AFAICT this >> > patch will introduce the random() call always (in >> explain_ExecutorStart): >> > >> > +if (auto_explain_log_min_duration >= 0 && nesting_level == 0) >> > +current_query_sampled = (random() < auto_explain_sample_ratio * >> > +MAX_RANDOM_VALUE); >> > >> > >> > Not sure what the overhead is, but wouldn't it be better to include a >> > check for current_query_sampled>0 in the if part of that statement? >> > Regardless of performance, that feels cleaner to me. Or am I missing >> > something? >> >> You mean check for auto_explain_sample_ratio > 0 ? >> > > I did, but I think what I should have meant is auto_explain_sample_ratio < > 1. > > > >> There would be a corner case if a query is sampled >> (current_query_sampled is true) and then auto_explain_sample_ratio is >> set to 0, all subsequent queries in this backend would be processed. >> > > There would have to be an else block as well of course, that set it back. > > > >> We could add a specific test for this case to spare a random() call, but >> I fear it'd be overkill. Maybe it's better to document that the good way >> to deactivate auto_explain is to set auto_explain.log_min_duration to -1. >> > > I guess in the end it probably is - a general random() call is pretty > cheap compared to all the things we're doing. I think my mind was stuck in > crypto-random which can be more expensive :) > > Applied with a minor word-fix in the documentation and removal of some unrelated whitespace changes. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
