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.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to