Re: idea: log_statement_sample_rate - bottom limit for sampling - Mailing list pgsql-hackers

From Pavel Stehule
Subject Re: idea: log_statement_sample_rate - bottom limit for sampling
Date
Msg-id CAFj8pRCLff4Ykpbk9DcLvP3pk-wcPn3bewmmuO+V4a4C3R_gCQ@mail.gmail.com
Whole thread Raw
In response to Re: idea: log_statement_sample_rate - bottom limit for sampling  (Adrien Nayrat <adrien.nayrat@anayrat.info>)
Responses Re: idea: log_statement_sample_rate - bottom limit for sampling
List pgsql-hackers


út 18. 6. 2019 v 14:03 odesílatel Adrien Nayrat <adrien.nayrat@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

pgsql-hackers by date:

Previous
From: Oleksii Kliukin
Date:
Subject: Re: pgsql: Avoid spurious deadlocks when upgrading a tuple lock
Next
From: Andres Freund
Date:
Subject: Re: Valgrind failures in Apply Launcher's bgworker_quickdie() exit