út 18. 6. 2019 v 14:03 odesílatel Adrien Nayrat <adrien.nay...@anayrat.info>
napsal:
> Hi,
>
> I tried the patch, here my comment:
>
> > gettext_noop("Zero effective disables sampling. "
> > "-1 use sampling every time (without limit)."),
>
> I do not agree with the zero case. In fact, sampling is disabled as soon as
> setting is less than log_min_duration_statements. Furthermore, I think we
> should
> provide a more straightforward description for users.
>
You have true, but I have not a idea,how to describe it in one line. In
this case the zero is corner case, and sampling is disabled without
dependency on log_min_duration_statement.
I think so this design has only few useful values and ranges
a) higher than log_min_duration_statement .. sampling is active with limit
b) 0 .. for this case - other way how to effective disable sampling - no
dependency on other
c) -1 or negative value - sampling is allowed every time.
Sure, there is range (0..log_min_duration_statement), but for this range
this value has not sense. I think so this case cannot be mentioned in short
description. But it should be mentioned in documentation.
> I changed few comments and documentation:
>
> * As we added much more logic in this function with statement and
> transaction
> sampling. And now with statement_sample_rate, it is not easy to understand
> the
> logic on first look. I reword comment in check_log_duration, I hope it is
> more
> straightforward.
>
> * I am not sure if "every_time" is a good naming for the variable. In
> fact, if
> duration exceeds limit we disable sampling. Maybe sampling_disabled is
> more clear?
>
For me important is following line
(exceeded && (in_sample || every_time))
I think so "every_time" or "always" or "every" is in this context more
illustrative than "sampling_disabled". But my opinion is not strong in this
case, and I have not a problem accept common opinion.
>
> * I propose to add some words in log_min_duration_statement and
> log_statement_sample_rate documentation.
>
> * Rephrased log_statement_sample_limit documentation, I hope it help
> understanding.
>
> Patch attached.
>
> Regards,
>
> --
> Adrien
>
