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