Re: describe special values in GUC descriptions more consistently

For many GUCs, special values like -1, "", etc. have some sort of special
meaning, such as disabling the feature. While the documentation seems to
be reasonably good about listing special values, the GUC descriptions are
less consistent. Many GUC descriptions don't state the special values at
all, and the ones that do are inconsistent in phrasing and placement (e.g.,
a couple list them in the short description, but most list them in the long
description). I figured I'd try to add some consistency here.

In the attached patch, I've attempted to apply the following proposed rules
to the descriptions of GUCs that accept special values:

* The special values should be listed at the end of the long description.
* Descriptions should use numerals (0) instead of words (zero).
* The special value notes should be consistently worded and as direct as
possible (e.g., "0 disables the timeout.", "An empty string means to use
the system setting.").

There are a few exceptions, such as max_pred_locks_per_relation and
search_path, where the special values are too complicated to include. And
there are cases like unix_socket_directories, listen_addresses,
application_name, and cluster_name, where the meaning of "" is arguably too
obvious to bother including. There's a good chance that I've missed a
couple, too.

Thoughts?

--
nathan

On Sat, Feb 8, 2025 at 9:27 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> For many GUCs, special values like -1, "", etc. have some sort of special
> meaning, such as disabling the feature. While the documentation seems to
> be reasonably good about listing special values, the GUC descriptions are
> less consistent. Many GUC descriptions don't state the special values at
> all, and the ones that do are inconsistent in phrasing and placement (e.g.,
> a couple list them in the short description, but most list them in the long
> description). I figured I'd try to add some consistency here.
>
> In the attached patch, I've attempted to apply the following proposed rules
> to the descriptions of GUCs that accept special values:
>
> * The special values should be listed at the end of the long description.
> * Descriptions should use numerals (0) instead of words (zero).
> * The special value notes should be consistently worded and as direct as
> possible (e.g., "0 disables the timeout.", "An empty string means to use
> the system setting.").
>
> There are a few exceptions, such as max_pred_locks_per_relation and
> search_path, where the special values are too complicated to include. And
> there are cases like unix_socket_directories, listen_addresses,
> application_name, and cluster_name, where the meaning of "" is arguably too
> obvious to bother including. There's a good chance that I've missed a
> couple, too.
>
> Thoughts?
>

+1 for improving consistency.

======

Some patch observations and nits.

1. IMO all places wording as "XXX means to YYY" should be just "XXX
means YYY" (e.g. remove the "to")

e.g. "-1 means to wait forever." => "-1 means wait forever."
e.g. ""-1 means to log values in full." => "-1 means log values in full."

~~~

2. GUC names in messages should always be double quoted.

~~~

3. Wording for the automatic selections.

Some of the special values get calculated and assigned *automatically*
on your behalf. The patch currently seems to be using "means to use"
for these:

I felt all these should be written as:
"XXX means to use YYY" => "XXX means YYY is used."

e.g. "0 means to use a suitable default value." => "0 means a suitable
default value is used."
e.g. "0 means to use a fraction of \"shared_buffers\"." => "0 means a
fraction of \"shared_buffers\" is used".
e.g. "-1 means to use vacuum_cost_limit" => "-1 means
\"vacuum_cost_limit\" is used."

~~

4. When there are multiple special values, it seems more natural if
the values are ordered. Maybe just personal preference.

e.g. "0 means to log all files. -1 disables temporary file logs." =>
"-1 disables temporary file logs. 0 means log all files."

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

On Mon, Feb 10, 2025 at 09:13:26AM +1100, Peter Smith wrote:
> +1 for improving consistency.

Thanks for reviewing.

> 1. IMO all places wording as "XXX means to YYY" should be just "XXX
> means YYY" (e.g. remove the "to")
>
> e.g. "-1 means to wait forever." => "-1 means wait forever."
> e.g. ""-1 means to log values in full." => "-1 means log values in full."

I think this works in some cases, but IMHO it sounds awkward with the
"means to use" ones (e.g., "0 means use the system default"). So I would
probably leave the "to" in those.

> 2. GUC names in messages should always be double quoted.

Will fix.

> 3. Wording for the automatic selections.
>
> Some of the special values get calculated and assigned *automatically*
> on your behalf. The patch currently seems to be using "means to use"
> for these:
>
> I felt all these should be written as:
> "XXX means to use YYY" => "XXX means YYY is used."
>
> e.g. "0 means to use a suitable default value." => "0 means a suitable
> default value is used."
> e.g. "0 means to use a fraction of \"shared_buffers\"." => "0 means a
> fraction of \"shared_buffers\" is used".
> e.g. "-1 means to use vacuum_cost_limit" => "-1 means
> \"vacuum_cost_limit\" is used."

I'm also not tremendously happy with "means to use," but I'd like to avoid
passive voice if possible.

> 4. When there are multiple special values, it seems more natural if
> the values are ordered. Maybe just personal preference.
>
> e.g. "0 means to log all files. -1 disables temporary file logs." =>
> "-1 disables temporary file logs. 0 means log all files."

Seems reasonable to me.

--
nathan

On Mon, Feb 10, 2025 at 9:02 AM Nathan Bossart <nathandbossart(at)gmail(dot)com>
wrote:

> On Mon, Feb 10, 2025 at 09:13:26AM +1100, Peter Smith wrote:
> > +1 for improving consistency.
>
> Thanks for reviewing.
>
> > 1. IMO all places wording as "XXX means to YYY" should be just "XXX
> > means YYY" (e.g. remove the "to")
> >
> > e.g. "-1 means to wait forever." => "-1 means wait forever."
> > e.g. ""-1 means to log values in full." => "-1 means log values in full."
>
> I think this works in some cases, but IMHO it sounds awkward with the
> "means to use" ones (e.g., "0 means use the system default"). So I would
> probably leave the "to" in those.
>
> > 2. GUC names in messages should always be double quoted.
>
> Will fix.
>
> > 3. Wording for the automatic selections.
> >
> > Some of the special values get calculated and assigned *automatically*
> > on your behalf. The patch currently seems to be using "means to use"
> > for these:
> >
> > I felt all these should be written as:
> > "XXX means to use YYY" => "XXX means YYY is used."
> >
> > e.g. "0 means to use a suitable default value." => "0 means a suitable
> > default value is used."
> > e.g. "0 means to use a fraction of \"shared_buffers\"." => "0 means a
> > fraction of \"shared_buffers\" is used".
> > e.g. "-1 means to use vacuum_cost_limit" => "-1 means
> > \"vacuum_cost_limit\" is used."
>
> I'm also not tremendously happy with "means to use," but I'd like to avoid
> passive voice if possible.
>
>
For most of these "is used" is just noise anyway and we can simplify things
to:

X means [no "to"] <verb: log, compute, sample, disable> thing

-1 means log values in full
0 means sample all statements
-1 means disable sampling
-1 means wait forever

-1 means use vacuum_cost_limit
0 means compute a fraction of "shared_buffers" (0 means use a fraction of
"shared_buffers" also works)

I get the argument for avoiding saying what the fraction used is; but at
the same time it seems like it should be documented.

David J.

On Tue, Feb 11, 2025 at 3:22 AM David G. Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>
> On Mon, Feb 10, 2025 at 9:02 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>>
>> On Mon, Feb 10, 2025 at 09:13:26AM +1100, Peter Smith wrote:
>> > +1 for improving consistency.
>>
>> Thanks for reviewing.
>>
>> > 1. IMO all places wording as "XXX means to YYY" should be just "XXX
>> > means YYY" (e.g. remove the "to")
>> >
>> > e.g. "-1 means to wait forever." => "-1 means wait forever."
>> > e.g. ""-1 means to log values in full." => "-1 means log values in full."
>>
>> I think this works in some cases, but IMHO it sounds awkward with the
>> "means to use" ones (e.g., "0 means use the system default"). So I would
>> probably leave the "to" in those.
>>
>> > 2. GUC names in messages should always be double quoted.
>>
>> Will fix.
>>
>> > 3. Wording for the automatic selections.
>> >
>> > Some of the special values get calculated and assigned *automatically*
>> > on your behalf. The patch currently seems to be using "means to use"
>> > for these:
>> >
>> > I felt all these should be written as:
>> > "XXX means to use YYY" => "XXX means YYY is used."
>> >
>> > e.g. "0 means to use a suitable default value." => "0 means a suitable
>> > default value is used."
>> > e.g. "0 means to use a fraction of \"shared_buffers\"." => "0 means a
>> > fraction of \"shared_buffers\" is used".
>> > e.g. "-1 means to use vacuum_cost_limit" => "-1 means
>> > \"vacuum_cost_limit\" is used."
>>
>> I'm also not tremendously happy with "means to use," but I'd like to avoid
>> passive voice if possible.
>>
>
> For most of these "is used" is just noise anyway and we can simplify things to:
>
> X means [no "to"] <verb: log, compute, sample, disable> thing
>

+1 for this. Your wording examples below look good to me..

> -1 means log values in full
> 0 means sample all statements
> -1 means disable sampling
> -1 means wait forever
>
> -1 means use vacuum_cost_limit
> 0 means compute a fraction of "shared_buffers" (0 means use a fraction of "shared_buffers" also works)
>
> I get the argument for avoiding saying what the fraction used is; but at the same time it seems like it should be documented.
>

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Feb 11, 2025 at 08:29:28AM +1100, Peter Smith wrote:
> +1 for this. Your wording examples below look good to me..

Okay, how does this look?

--
nathan

On Tue, Feb 11, 2025 at 9:25 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> On Tue, Feb 11, 2025 at 08:29:28AM +1100, Peter Smith wrote:
> > +1 for this. Your wording examples below look good to me..
>
> Okay, how does this look?
>
> --

v2 mostly looked good to me. Just a couple of questions.

~~~

1.
{"shared_memory_size_in_huge_pages", PGC_INTERNAL, PRESET_OPTIONS,
gettext_noop("Shows the number of huge pages needed for the main
shared memory area."),
- gettext_noop("-1 indicates that the value could not be determined."),
+ gettext_noop("-1 means the value could not be determined."),

I didn't know what that meant. And the docs seem worded differently. More like:
"-1 means huge pages are not supported."

~~~

2.
- gettext_noop("An empty string indicates that \"archive_command\"
should be used.")
+ gettext_noop("An empty string means \"archive_command\" should be used.")

Should that be worded more like other ones that delegate to other GUCs:

"An empty string means use \"archive_command\"."

~~~

3.

What is the difference between "system setting" and "system default",
or should those be made the same?

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

On Mon, Feb 10, 2025 at 4:53 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:

> On Tue, Feb 11, 2025 at 9:25 AM Nathan Bossart <nathandbossart(at)gmail(dot)com>
> wrote:
> >
> > On Tue, Feb 11, 2025 at 08:29:28AM +1100, Peter Smith wrote:
> > > +1 for this. Your wording examples below look good to me..
> >
> > Okay, how does this look?
> >
> > --
>
> v2 mostly looked good to me. Just a couple of questions.
>
> ~~~
>
> 1.
> {"shared_memory_size_in_huge_pages", PGC_INTERNAL, PRESET_OPTIONS,
> gettext_noop("Shows the number of huge pages needed for the main
> shared memory area."),
> - gettext_noop("-1 indicates that the value could not be determined."),
> + gettext_noop("-1 means the value could not be determined."),
>
> I didn't know what that meant. And the docs seem worded differently. More
> like:
> "-1 means huge pages are not supported."
>

Agreed

> ~~~
>
> 2.
> - gettext_noop("An empty string indicates that \"archive_command\"
> should be used.")
> + gettext_noop("An empty string means \"archive_command\" should be used.")
>
> Should that be worded more like other ones that delegate to other GUCs:
>
> "An empty string means use \"archive_command\"."
>

Agreed

> ~~~
>
> 3.
>
> What is the difference between "system setting" and "system default",
> or should those be made the same?
>
>
Looking at the documentation it seems to be communicating the difference
between a PostgreSQL default (system default) and a value taken from the
operating environment (system setting). Maybe making that more clear by
saying "operating system setting" is warranted.

A couple of more items.

Minor typo, trailing whitespace in message:

"lost before a connection is considered dead. "

And then these two don't seem worded symmetrically enough:

"An empty string means don't load CRL file unless \"ssl_crl_dir\" is set."
"An empty string means don't use CRLs unless \"ssl_crl_file\" is set."

The first one also needs to be "the CRL file".

But I'm thinking something more like:

"An empty string disables the directory-based CRL auto-load mechanism."
(ssl_crl_dir)
"An empty string disables the fixed-file CRL reload mechanism."
(ssl_crl_file)

I don't see much benefit trying to shoe-horn a cross-reference to the other
setting in these brief usage messages. Though if we wanted to a simple
(see also ssr_crl_<file|dir>) would suffice. It seems unworthy of the
limited space to try and communicate the fact that if both are empty
strings no CRL files will be loaded (or that both can be used at the same
time). Even trying to fit in the "auto-load versus reload" dynamic of
these two choices seems awkward.

David J.

Thank you for the reviews.

On Mon, Feb 10, 2025 at 05:25:42PM -0700, David G. Johnston wrote:
> On Mon, Feb 10, 2025 at 4:53 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>> {"shared_memory_size_in_huge_pages", PGC_INTERNAL, PRESET_OPTIONS,
>> gettext_noop("Shows the number of huge pages needed for the main
>> shared memory area."),
>> - gettext_noop("-1 indicates that the value could not be determined."),
>> + gettext_noop("-1 means the value could not be determined."),
>>
>> I didn't know what that meant. And the docs seem worded differently. More
>> like:
>> "-1 means huge pages are not supported."
>>
>
> Agreed

Fixed.

>> - gettext_noop("An empty string indicates that \"archive_command\"
>> should be used.")
>> + gettext_noop("An empty string means \"archive_command\" should be used.")
>>
>> Should that be worded more like other ones that delegate to other GUCs:
>>
>> "An empty string means use \"archive_command\"."
>>
>
> Agreed

Fixed.

>> What is the difference between "system setting" and "system default",
>> or should those be made the same?
>>
>>
> Looking at the documentation it seems to be communicating the difference
> between a PostgreSQL default (system default) and a value taken from the
> operating environment (system setting). Maybe making that more clear by
> saying "operating system setting" is warranted.

I've used "operating system setting" in v3.

> Minor typo, trailing whitespace in message:
>
> "lost before a connection is considered dead. "

This is intentional so that there is a space between the sentences in the
long description.

> And then these two don't seem worded symmetrically enough:
>
> "An empty string means don't load CRL file unless \"ssl_crl_dir\" is set."
> "An empty string means don't use CRLs unless \"ssl_crl_file\" is set."
>
> The first one also needs to be "the CRL file".
>
> But I'm thinking something more like:
>
> "An empty string disables the directory-based CRL auto-load mechanism."
> (ssl_crl_dir)
> "An empty string disables the fixed-file CRL reload mechanism."
> (ssl_crl_file)
>
> I don't see much benefit trying to shoe-horn a cross-reference to the other
> setting in these brief usage messages. Though if we wanted to a simple
> (see also ssr_crl_<file|dir>) would suffice. It seems unworthy of the
> limited space to try and communicate the fact that if both are empty
> strings no CRL files will be loaded (or that both can be used at the same
> time). Even trying to fit in the "auto-load versus reload" dynamic of
> these two choices seems awkward.

I thought about this one a bit, and I actually came to the opposite
conclusion. IMHO it's reasonably obvious that an empty string means that
the file isn't loaded, so there's not much point in stating it in the GUC
description. Instead, I think we should follow the
archive_command/archive_library example and use this space _only_ as a
cross-reference to each other. There's certainly some nuances missed with
this strategy, but that's not unique to this GUC.

--
nathan

I don't have an opinion about the ssl_crl stuff. Everything else looks
good to me.

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

> On 11 Feb 2025, at 19:11, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:

> I thought about this one a bit, and I actually came to the opposite
> conclusion. IMHO it's reasonably obvious that an empty string means that
> the file isn't loaded, so there's not much point in stating it in the GUC
> description. Instead, I think we should follow the
> archive_command/archive_library example and use this space _only_ as a
> cross-reference to each other. There's certainly some nuances missed with
> this strategy, but that's not unique to this GUC.

I don't necessarily disagree with this, but the proposed wording makes it sound
sort of like users have to select one or the other. Could it be softened a
little like perhaps "An empty string disables, \"ssl_crl_foo\" is still used"?

--
Daniel Gustafsson

On Tue, Feb 11, 2025 at 11:41:51PM +0100, Daniel Gustafsson wrote:
>> On 11 Feb 2025, at 19:11, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
>> I thought about this one a bit, and I actually came to the opposite
>> conclusion. IMHO it's reasonably obvious that an empty string means that
>> the file isn't loaded, so there's not much point in stating it in the GUC
>> description. Instead, I think we should follow the
>> archive_command/archive_library example and use this space _only_ as a
>> cross-reference to each other. There's certainly some nuances missed with
>> this strategy, but that's not unique to this GUC.
>
> I don't necessarily disagree with this, but the proposed wording makes it sound
> sort of like users have to select one or the other. Could it be softened a
> little like perhaps "An empty string disables, \"ssl_crl_foo\" is still used"?

Hm. I'm starting to lean towards considering these as too complicated to
include in the GUC description.

--
nathan

Here is what I have staged for commit, which I intend to do within the next
couple of days unless there is additional feedback. In v4, I've added a
commit message, removed the changes to the ssl_crl_* parameters, and fixed
a couple of very small mistakes.

--
nathan

> On 12 Feb 2025, at 22:04, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> Here is what I have staged for commit, which I intend to do within the next
> couple of days unless there is additional feedback. In v4, I've added a
> commit message, removed the changes to the ssl_crl_* parameters, and fixed
> a couple of very small mistakes.

LGTM, thanks for tackling.

--
Daniel Gustafsson

One last thing...

- gettext_noop("Zero logs all files. The default is -1 (turning this
feature off)."),
+ gettext_noop("-1 disables temporary file logs. 0 means log all
temporary files."),

The first sentence could be ambiguous. E.g. "temporary file logs"
might be interpreted as meaning logs about temporary files, or about
temporary logs of files.

I think it should be worded more like the second sentence.
e.g. "-1 disables logging temporary files. 0 means log all temporary files."

And doing this will be consistent with others you already have:
+ gettext_noop("-1 disables logging statement durations. 0 means log
all statement durations."),
+ gettext_noop("-1 disables logging autovacuum actions. 0 means log
all autovacuum actions."),

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Feb 13, 2025 at 09:02:30AM +1100, Peter Smith wrote:
> One last thing...
>
> - gettext_noop("Zero logs all files. The default is -1 (turning this
> feature off)."),
> + gettext_noop("-1 disables temporary file logs. 0 means log all
> temporary files."),
>
> The first sentence could be ambiguous. E.g. "temporary file logs"
> might be interpreted as meaning logs about temporary files, or about
> temporary logs of files.
>
> I think it should be worded more like the second sentence.
> e.g. "-1 disables logging temporary files. 0 means log all temporary files."
>
> And doing this will be consistent with others you already have:
> + gettext_noop("-1 disables logging statement durations. 0 means log
> all statement durations."),
> + gettext_noop("-1 disables logging autovacuum actions. 0 means log
> all autovacuum actions."),

Good catch. I've fixed that in v5.

--
nathan

On Wed, Feb 12, 2025 at 3:47 PM Nathan Bossart <nathandbossart(at)gmail(dot)com>
wrote:

> Good catch. I've fixed that in v5.
>
>
I presume it doesn't affect the actual output which just concatenates the
fragments together but the source placement probably should be made
consistent; the line containing the initial default value specification
begins its own quoted fragment. The following violate that convention.

gettext_noop("Write a message to the server log if checkpoints "
"caused by the filling of WAL segment files happen more "
- "frequently than this amount of time. "
- "Zero turns off the warning."),
+ "frequently than this amount of time. 0 disables the "
+ "warning."),

//move beginning with 0 to the last line

gettext_noop("Number of consecutive keepalive retransmits that can be "
- "lost before a connection is considered dead. A value of 0 uses the "
- "system default."),
+ "lost before a connection is considered dead. 0 "
+ "means use the system default."),

//just move the 0 to the last line

gettext_noop("Replication slots will be marked as failed, and segments
released "
"for deletion or recycling, if this much space is occupied by WAL "
- "on disk."),
+ "on disk. -1 means no maximum."),

//move on disk to the prior line, unless there is some kind of length
limitation that should be documented as well.

gettext_noop("Write a message to the server log if checkpoints "
"caused by the filling of WAL segment files happen more "
- "frequently than this amount of time. "
- "Zero turns off the warning."),
+ "frequently than this amount of time. 0 disables the "
+ "warning."),

//move beginning with zero to the last line.

gettext_noop("The owning user of the socket is always the user "
- "that starts the server.")
+ "that starts the server. An empty string means use "
+ "the user's default group.")

//two lines probably...second begins 'An empty string'

Also, maybe put the rules in the commit message into a comment in the file,
or a README, instead.

David J.

On Wed, Feb 12, 2025 at 04:10:53PM -0700, David G. Johnston wrote:
> I presume it doesn't affect the actual output which just concatenates the
> fragments together but the source placement probably should be made
> consistent; the line containing the initial default value specification
> begins its own quoted fragment. The following violate that convention.

Eh, most of the other descriptions with multiple sentences don't do that,
so IMHO there's no need for the special values to go in their own fragment.
You are correct that there should be no difference in the actual output.

> Also, maybe put the rules in the commit message into a comment in the file,
> or a README, instead.

I added a comment to guc_tables.h in v6.

--
nathan

On Thu, Feb 13, 2025 at 3:18 PM Nathan Bossart <nathandbossart(at)gmail(dot)com>
wrote:

> On Wed, Feb 12, 2025 at 04:10:53PM -0700, David G. Johnston wrote:
> > I presume it doesn't affect the actual output which just concatenates the
> > fragments together but the source placement probably should be made
> > consistent; the line containing the initial default value specification
> > begins its own quoted fragment. The following violate that convention.
>
> Eh, most of the other descriptions with multiple sentences don't do that,
>

Apples and oranges.

so IMHO there's no need for the special values to go in their own fragment.
>

The examples shown look sloppy, IMHO; standing out since the other 50,
mostly due to the defaults being the only sentence in the gettext_noop, do
line up nicely with either a number or "The empty string" as the lead.

the worst offender is:

- "lost before a connection is considered dead. A value of 0 uses the "
- "system default."),
+ "lost before a connection is considered dead. 0 "
+ "means use the system default."),

Even if the diff has logic to it - only remove stuff from the first line,
only add stuff to the second, it isn't a big enough gain to offset leaving
the source ugly, IMHO.

David J.

On Thu, Feb 13, 2025 at 03:50:08PM -0700, David G. Johnston wrote:
> Even if the diff has logic to it - only remove stuff from the first line,
> only add stuff to the second, it isn't a big enough gain to offset leaving
> the source ugly, IMHO.

Okay, I took your suggestions in v7.

--
nathan

On Thu, Feb 13, 2025 at 05:01:59PM -0600, Nathan Bossart wrote:
> Okay, I took your suggestions in v7.

Committed. Thanks, David, Peter, and Daniel!

--
nathan

On 14.02.2025 19:47, Nathan Bossart wrote:
> On Thu, Feb 13, 2025 at 05:01:59PM -0600, Nathan Bossart wrote:
>> Okay, I took your suggestions in v7.
> Committed. Thanks, David, Peter, and Daniel!
>

Hi,

Maybe I'm being picky, but in auto_explain, the parameters
log_min_duration and log_parameter_max_length do not follow the
conventions we have adopted. I mean we should use numerals instead of
words. I'm attaching a patch to fix this.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.

On Mon, Feb 17, 2025 at 8:50 PM Ilia Evdokimov
<ilya(dot)evdokimov(at)tantorlabs(dot)com> wrote:
>
>
> On 14.02.2025 19:47, Nathan Bossart wrote:
> > On Thu, Feb 13, 2025 at 05:01:59PM -0600, Nathan Bossart wrote:
> >> Okay, I took your suggestions in v7.
> > Committed. Thanks, David, Peter, and Daniel!
> >
>
> Hi,
>
> Maybe I'm being picky, but in auto_explain, the parameters
> log_min_duration and log_parameter_max_length do not follow the
> conventions we have adopted. I mean we should use numerals instead of
> words. I'm attaching a patch to fix this.
>

+1 for numerals, but I think there are still some problems with the v8 patch

- 0 and -1 are the wrong way around.
- We should separate the special values using a period (.) instead of
a comma (,) for consistency with all the others.
- It should adopt new wording that is more similar to the other GUCs.

For example.

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.",
+ "0 prints all plans. -1 turns this feature off.",

SUGGESTION
"-1 disables logging plans. 0 means log all plans."

~~~

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.".

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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

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.

--
nathan

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.

On 18.02.2025 01:13, Ilia Evdokimov wrote:
>
> Thank you for reviewing! I agree with all of them. I updated patch v9
> with these changes.
>
> --
> Best regards,
> Ilia Evdokimov,
> Tantor Labs LLC.

Hi hackers,

I’ve addressed all your comments on the v9 patch for updating the
descriptions of auto_explain.log_min_duration and
auto_explain.log_parameter_max_length. Just wanted to check if you had a
chance to take another look. Let me know if there’s anything else that
needs to be adjusted.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.

On Tue, Feb 25, 2025 at 02:42:17PM +0300, Ilia Evdokimov wrote:
> I´ve addressed all your comments on the v9 patch for updating the
> descriptions of auto_explain.log_min_duration and
> auto_explain.log_parameter_max_length. Just wanted to check if you had a
> chance to take another look. Let me know if there´s anything else that needs
> to be adjusted.

Committed, sorry for the delay.

--
nathan