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
signature.asc
Description: OpenPGP digital signature
