Re: idea: log_statement_sample_rate - bottom limit for sampling

st 19. 6. 2019 v 10:49 odesílatel Adrien Nayrat <adrien(dot)nayrat(at)anayrat(dot)info>
napsal:

> On 6/18/19 8:29 PM, Pavel Stehule wrote:
> >
> >
> > út 18. 6. 2019 v 14:03 odesílatel Adrien Nayrat <
> adrien(dot)nayrat(at)anayrat(dot)info
> > <mailto:adrien(dot)nayrat(at)anayrat(dot)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.
>
> Yes, it took me a while to understand :) I am ok to keep simple in GUC
> description and give more information in documentation.
>

Maybe some like. "The zero block sampling. Negative value forces sampling
without limit"

> >
> >
> > 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.
>
> Oh, yes, that's correct. I do not have a strong opinion too. Maybe someone
> else
> will have better idea.
>

the naming in this case is not hard issue, and comitter can decide.

Regards

Pavel

>
> --
> Adrien
>
>