Re: auto_explain sample rate

On Fri, Mar 11, 2016 at 3:03 PM, Magnus Hagander <magnus(at)hagander(dot)net>
wrote:

>
>
> On Fri, Mar 11, 2016 at 2:51 PM, Julien Rouhaud <julien(dot)rouhaud(at)dalibo(dot)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/