st 19. 6. 2019 v 10:49 odesílatel Adrien Nayrat <adrien.nay...@anayrat.info> napsal:
> On 6/18/19 8:29 PM, Pavel Stehule wrote: > > > > > > út 18. 6. 2019 v 14:03 odesílatel Adrien Nayrat < > adrien.nay...@anayrat.info > > <mailto: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. > > 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 > >
