Re: New GUC to sample log queries

On 24/06/18 13:22, Adrien Nayrat wrote:
> Attached third version of the patch with documentation.

Hi.  I'm reviewing this.

>  		exceeded = (log_min_duration_statement == 0 ||
>  					(log_min_duration_statement > 0 &&
>  					 (secs > log_min_duration_statement / 1000 ||
> -					  secs * 1000 + msecs >= log_min_duration_statement)));
> +					  secs * 1000 + msecs >= log_min_duration_statement))) &&
> +				   log_sample_rate != 0 && (log_sample_rate == 1 ||
> +				   random() <= log_sample_rate * MAX_RANDOM_VALUE);

A few notes on this part, which is the crux of the patch.

1) Is there an off-by-one error here?  drandom() has the formula

    (double) random() / ((double) MAX_RANDOM_VALUE + 1)

but yours doesn't look like that.

2) I think the whole thing should be separated into its own expression
instead of tagging along on an already hard to read expression.

3) Is it intentional to only sample with log_min_duration_statement and
not also with log_duration?  It seems like it should affect both.  In
both cases, the name is too generic.  Something called "log_sample_rate"
should sample **everything**.

Otherwise I think it's good.  Attached is a revised patch that fixes 2)
as well as some wordsmithing throughout.  It does not deal with the
other issues I raised.

Marked "Waiting on Author".
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support