On 16/02/2016 22:51, Alvaro Herrera wrote:
> Julien Rouhaud wrote:
>
> Hijacking this macro is just too obscure:
>
>> #define auto_explain_enabled() \
>> (auto_explain_log_min_duration >= 0 && \
>> - (nesting_level == 0 || auto_explain_log_nested_statements))
>> + (nesting_level == 0 || auto_explain_log_nested_statements) && \
>> + current_query_sampled)
>
> because it then becomes hard to figure out that assigning to _sampled is
> what makes the enabled() check pass or not depending on sampling:
>
>> @@ -191,6 +211,14 @@ _PG_fini(void)
>> static void
>> explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
>> {
>> + /*
>> + * For ratio sampling, randomly choose top-level statement. Either
>> + * all nested statements will be explained or none will.
>> + */
>> + if (auto_explain_log_min_duration >= 0 && nesting_level == 0)
>> + current_query_sampled = (random() < auto_explain_sample_ratio *
>> + MAX_RANDOM_VALUE);
>> +
>> if (auto_explain_enabled())
>> {
>
> I think it's better to keep the "enabled" macro unmodified, and just add
> another conditional to the "if" test there.
>
Thanks for looking at this!
Agreed, it's too obscure. Attached v4 fixes as you said.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
