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