Re: auto_explain sample rate

From: Julien Rouhaud <julien(dot)rouhaud(at)dalibo(dot)com>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: auto_explain sample rate
Date: 2016-02-17 00:17:35
Message-ID: 56C3BC1F.1040607@dalibo.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
auto_explain_sample_rate-v4.patch text/x-patch 3.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jim Nasby 2016-02-17 00:38:20 Re: proposal: function parse_ident
Previous Message Bruce Momjian 2016-02-16 23:50:41 Re: PostgreSQL Audit Extension