Ăș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 >