On 18.02.2025 00:55, Nathan Bossart wrote:
On Tue, Feb 18, 2025 at 08:14:58AM +1100, Peter Smith wrote:
SUGGESTION
"-1 disables logging plans. 0 means log all plans."
+1
DefineCustomIntVariable("auto_explain.log_parameter_max_length",
"Sets the maximum length of query parameters to log.",
- "Zero logs no query parameters, -1 logs them in full.",
+ "0 logs no query parameters, -1 logs them in full.",
SUGGESTION #1 (main description part)
In fact that whole description seems incompatible with the
documentation. IMO it should be "Sets the maximum length of query
parameter VALUES to log.".
I'm not sure omitting "values" changes the meaning in any discernible way,
but I'm not opposed to adding it.
SUGGESTION #2 (special value part)
"-1 disables logging parameter values."
OR
"-1 disables logging parameter values. 0 means log parameter values in full"
TBH, I am unsure if this one should even mention the value 0 because
there are already examples of "max" GUCs similar to this that say
nothing about 0 since the meaning should be clear.
e.g. log_parameter_max_length doesn't mention 0.
e.g. log_parameter_max_length_on_error doesn't mention 0.
I think you've got the meanings swapped here. It should be:
-1 means log parameter values in full. 0 disables logging parameter
values.
But I agree that we can omit the description for 0 here.
Thank you for reviewing! I agree with all of them. I updated patch v9
with these changes.
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
From 44f60949a4636eb649d29d752a55a8410213d12e Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov <ilya.evdoki...@tantorlabs.com>
Date: Tue, 18 Feb 2025 01:02:15 +0300
Subject: [PATCH v9] Standardize parameter descriptions in auto_explain
Align the descriptions of log_min_duration and log_parameter_max_length
in auto_explain with the formatting conventions used for other GUC parameters,
as established in commit 977d865.
Remove the description of the zero value for log_parameter_max_length,
as there are numerous examples of similar "max" GUC parameters that
do not explicitly mention zero, as its meaning is considered clear.
---
contrib/auto_explain/auto_explain.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index f1ad876e82..1c6f840e30 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -93,7 +93,7 @@ _PG_init(void)
/* Define custom GUC variables. */
DefineCustomIntVariable("auto_explain.log_min_duration",
"Sets the minimum execution time above which plans will be logged.",
- "Zero prints all plans. -1 turns this feature off.",
+ "-1 disables logging plans. 0 means log all plans.",
&auto_explain_log_min_duration,
-1,
-1, INT_MAX,
@@ -105,7 +105,7 @@ _PG_init(void)
DefineCustomIntVariable("auto_explain.log_parameter_max_length",
"Sets the maximum length of query parameters to log.",
- "Zero logs no query parameters, -1 logs them in full.",
+ "-1 means log parameter values in full.",
&auto_explain_log_parameter_max_length,
-1,
-1, INT_MAX,
--
2.34.1
