Re: New GUC to sample log queries

On 06/24/2018 08:41 PM, Vik Fearing wrote:
> On 24/06/18 13:22, Adrien Nayrat wrote:
>> Attached third version of the patch with documentation.
> 
> Hi.  I'm reviewing this.

Hi, thanks for your review.

> 
>>  		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.

I don't think there is an error. random() function return a long int between 0
and MAX_RANDOM_VALUE.

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

+1

> 
> 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**.

I do not think it could be useful to sample other case such as log_duration.

But yes, the GUC is confusing and I am not comfortable to introduce a new GUC in
my initial patch.

Maybe we should adapt current GUC with something like :

log_min_duration_statement = <time>,<sample rate>

This give :

log_min_duration_statement = 0,0.1

Equivalent to :
log_min_duration_statement = 0
log_sample_rate = 0.1

Thought?

> 
> 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.

Thanks for your corrections.

--
Adrien