From: | Adrien Nayrat <adrien(dot)nayrat(at)anayrat(dot)info> |
---|---|
To: | Vik Fearing <vik(dot)fearing(at)2ndquadrant(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: New GUC to sample log queries |
Date: | 2018-06-27 21:13:34 |
Message-ID: | 6f886aa0-cd54-e333-d084-269360b53687@anayrat.info |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | David Steele | 2018-06-27 21:29:43 | Re: Monitoring time of fsyncing WALs |
Previous Message | Alvaro Herrera | 2018-06-27 21:01:13 | Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables) |