Thread: Re: describe special values in GUC descriptions more consistently

On Sat, Feb 8, 2025 at 9:27 AM Nathan Bossart <nathandbossart@gmail.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 Tue, Feb 11, 2025 at 3:22 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Mon, Feb 10, 2025 at 9:02 AM Nathan Bossart <nathandbossart@gmail.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 9:25 AM Nathan Bossart <nathandbossart@gmail.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.

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@gmail.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@gmail.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

> On 12 Feb 2025, at 22:04, Nathan Bossart <nathandbossart@gmail.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 05:01:59PM -0600, Nathan Bossart wrote:
> Okay, I took your suggestions in v7.

Committed.  Thanks, David, Peter, and Daniel!

-- 
nathan

On Mon, Feb 17, 2025 at 8:50 PM Ilia Evdokimov
<ilya.evdokimov@tantorlabs.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