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.
