Re: auto_explain sample rate

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