Обсуждение: BUG #15912: The units of `autovacuum_vacuum_cost_delay` setting should be documented

The following bug has been logged on the website:

Bug reference:      15912
Logged by:          Basil Bourque
Email address:      basil.bourque@gmail.com
PostgreSQL version: 12beta2
Operating system:   macOS
Description:

The `autovacuum_vacuum_cost_delay` setting:

https://www.postgresql.org/docs/devel/runtime-config-autovacuum.html#GUC-AUTOVACUUM-VACUUM-COST-DELAY

…does not specify its units (seconds? milliseconds?) explicitly.  

Previously, one could guess that the units were milliseconds. 

But now in Postgres 12 the argument type changed from `integer` to `floating
point`. So now the units are quite unclear. I am guessing a whole number
means milliseconds, while the fractional part represents microseconds. But
that is only a guess. Should be documented.

> On 16 Jul 2019, at 18:26, PG Bug reporting form <noreply@postgresql.org> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference:      15912
> Logged by:          Basil Bourque
> Email address:      basil.bourque@gmail.com
> PostgreSQL version: 12beta2
> Operating system:   macOS
> Description:
>
> The `autovacuum_vacuum_cost_delay` setting:
>
> https://www.postgresql.org/docs/devel/runtime-config-autovacuum.html#GUC-AUTOVACUUM-VACUUM-COST-DELAY
>
> …does not specify its units (seconds? milliseconds?) explicitly.

The description of the setting contains "The default value is 2 milliseconds.”
which to me implies that the unit is measured in milliseconds.  If we want to
spell it out, we could add the unit like how many other on the same page does:

-        Specifies the cost delay value that will be used in automatic
+        Specifies the cost delay value (in milliseconds) that will be used in automatic

cheers ./daniel

On 2019-Jul-17, Daniel Gustafsson wrote:

> > The `autovacuum_vacuum_cost_delay` setting:
> > 
> > https://www.postgresql.org/docs/devel/runtime-config-autovacuum.html#GUC-AUTOVACUUM-VACUUM-COST-DELAY
> > 
> > …does not specify its units (seconds? milliseconds?) explicitly.  
> 
> The description of the setting contains "The default value is 2 milliseconds.”
> which to me implies that the unit is measured in milliseconds.  If we want to
> spell it out, we could add the unit like how many other on the same page does:
> 
> -        Specifies the cost delay value that will be used in automatic
> +        Specifies the cost delay value (in milliseconds) that will be used in automatic

But you can specify other units, so this would be wrong.  Example:

$ postmaster -c autovacuum_vacuum_cost_delay=100us

55432 13devel 17056=# show autovacuum_vacuum_cost_delay ;
 autovacuum_vacuum_cost_delay 
──────────────────────────────
 100us
(1 fila)


I guess you could suggest to say "in milliseconds unless some other unit
is specified" but that seems overly verbose and it would have to be
added to every single parameter's description.  Section 19.1.1 already
contains this:

    Numeric with Unit: Some numeric parameters have an implicit unit,
    because they describe quantities of memory or time. The unit might
    be bytes, kilobytes, blocks (typically eight kilobytes),
    milliseconds, seconds, or minutes. An unadorned numeric value for
    one of these settings will use the setting's default unit, which can
    be learned from pg_settings.unit. For convenience, settings can be
    given with a unit specified explicitly, for example '120 ms' for a
    time value, and they will be converted to whatever the parameter's
    actual unit is. Note that the value must be written as a string
    (with quotes) to use this feature. The unit name is case-sensitive,
    and there can be whitespace between the numeric value and the unit.

    Valid memory units are B (bytes), kB (kilobytes), MB (megabytes), GB
    (gigabytes), and TB (terabytes). The multiplier for memory units is
    1024, not 1000.

    Valid time units are us (microseconds), ms (milliseconds), s
    (seconds), min (minutes), h (hours), and d (days).

    If a fractional value is specified with a unit, it will be rounded
    to a multiple of the next smaller unit if there is one. For example,
    30.1 GB will be converted to 30822 MB not 32319628902 B. If the
    parameter is of integer type, a final rounding to integer occurs
    after any units conversion.


Now you could complain that this is inconsistent with other
descriptions; for example, log_autovacuum_min_duration talks about
milliseconds, which sounds a bit archaic to me:

   Causes each action executed by autovacuum to be logged if it ran for
   at least the specified number of milliseconds. Setting this to zero
   logs all autovacuum actions. -1 (the default) disables logging
   autovacuum actions. For example, if you set this to 250ms then all
   automatic vacuums and analyzes that run 250ms or longer will be
   logged. In addition, when this parameter is set to any value other
   than -1, a message will be logged if an autovacuum action is skipped
   due to a conflicting lock or a concurrently dropped relation.
   Enabling this parameter can be helpful in tracking autovacuum
   activity. This parameter can only be set in the postgresql.conf file
   or on the server command line; but the setting can be overridden for
   individual tables by changing table storage parameters.


I'm not really sure what's a good way to attack this problem, but I
doubt that focusing on just one description is a sufficient solution.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jul 26, 2019 at 06:02:42PM -0400, Alvaro Herrera wrote:
> Now you could complain that this is inconsistent with other
> descriptions; for example, log_autovacuum_min_duration talks about
> milliseconds, which sounds a bit archaic to me:
> 
>    Causes each action executed by autovacuum to be logged if it ran for
>    at least the specified number of milliseconds. Setting this to zero
>    logs all autovacuum actions. -1 (the default) disables logging
>    autovacuum actions. For example, if you set this to 250ms then all
>    automatic vacuums and analyzes that run 250ms or longer will be
>    logged. In addition, when this parameter is set to any value other
>    than -1, a message will be logged if an autovacuum action is skipped
>    due to a conflicting lock or a concurrently dropped relation.
>    Enabling this parameter can be helpful in tracking autovacuum
>    activity. This parameter can only be set in the postgresql.conf file
>    or on the server command line; but the setting can be overridden for
>    individual tables by changing table storage parameters.
> 
> 
> I'm not really sure what's a good way to attack this problem, but I
> doubt that focusing on just one description is a sufficient solution.

Yes, I looked at this earlier in the week and had the same conclusion. 
I went over config.sgml and saw many inconsistencies of the same type
being complained about here.

I went through the file and found a number of cases using milliseconds
and kilobytes that were unclear, and adjusted them.  I dealt only with
the cases where the base unit (seconds/bytes) was not the default unit. 
Patch attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Sat, Jul 27, 2019 at 05:41:30PM -0400, Bruce Momjian wrote:
> On Fri, Jul 26, 2019 at 06:02:42PM -0400, Alvaro Herrera wrote:
> > Now you could complain that this is inconsistent with other
> > descriptions; for example, log_autovacuum_min_duration talks about
> > milliseconds, which sounds a bit archaic to me:
> > 
> >    Causes each action executed by autovacuum to be logged if it ran for
> >    at least the specified number of milliseconds. Setting this to zero
> >    logs all autovacuum actions. -1 (the default) disables logging
> >    autovacuum actions. For example, if you set this to 250ms then all
> >    automatic vacuums and analyzes that run 250ms or longer will be
> >    logged. In addition, when this parameter is set to any value other
> >    than -1, a message will be logged if an autovacuum action is skipped
> >    due to a conflicting lock or a concurrently dropped relation.
> >    Enabling this parameter can be helpful in tracking autovacuum
> >    activity. This parameter can only be set in the postgresql.conf file
> >    or on the server command line; but the setting can be overridden for
> >    individual tables by changing table storage parameters.
> > 
> > 
> > I'm not really sure what's a good way to attack this problem, but I
> > doubt that focusing on just one description is a sufficient solution.
> 
> Yes, I looked at this earlier in the week and had the same conclusion. 
> I went over config.sgml and saw many inconsistencies of the same type
> being complained about here.
> 
> I went through the file and found a number of cases using milliseconds
> and kilobytes that were unclear, and adjusted them.  I dealt only with
> the cases where the base unit (seconds/bytes) was not the default unit. 
> Patch attached.

I applied a modified version of this patch.  I didn't backpatch it past
PG 12 because earlier releases were just too different.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

[ cc'ing pgsql-docs as well ]

Bruce Momjian <bruce@momjian.us> writes:
> I applied a modified version of this patch.  I didn't backpatch it past
> PG 12 because earlier releases were just too different.

I happened to stumble across the effects of this patch while trying
to improve our explanation of statement_timeout.  I follow the goal
of explaining what the default units are, but I really dislike the
way you did it:

+        Abort any statement that takes more than the specified duration
+        (defaults to milliseconds), starting from the time the command arrives at the server
         from the client. ...

I think this is confusingly bad English, and it's poor exposition
because a minor detail (it must be pretty minor, if we got away
without mentioning it at all for years) is injected into the middle
of the basic statement of the variable's purpose.  I think what we'd
be better off doing is to write a separate sentence mentioning the
units, in more or less the same way that we generally handle the
default value.  In <14850.1571941169@sss.pgh.pa.us> I suggested
this revision for statement_timeout:

        Abort any statement that takes more than the specified duration.
        If <varname>log_min_error_statement</varname> is set
        to <literal>ERROR</literal> or lower, the statement that timed out
        will also be logged.
        If the value is specified as a plain number, it is measured in
        milliseconds by default.
        A value of zero (the default) disables the timeout.

(I'm not quite sure whether the ending "by default" is worth writing
or not.)

Barring objections, I'll run around and make them all look like that.

            regards, tom lane

[ cc'ing pgsql-docs as well ]

Bruce Momjian <bruce@momjian.us> writes:
> I applied a modified version of this patch.  I didn't backpatch it past
> PG 12 because earlier releases were just too different.

I happened to stumble across the effects of this patch while trying
to improve our explanation of statement_timeout.  I follow the goal
of explaining what the default units are, but I really dislike the
way you did it:

+        Abort any statement that takes more than the specified duration
+        (defaults to milliseconds), starting from the time the command arrives at the server
         from the client. ...

I think this is confusingly bad English, and it's poor exposition
because a minor detail (it must be pretty minor, if we got away
without mentioning it at all for years) is injected into the middle
of the basic statement of the variable's purpose.  I think what we'd
be better off doing is to write a separate sentence mentioning the
units, in more or less the same way that we generally handle the
default value.  In <14850.1571941169@sss.pgh.pa.us> I suggested
this revision for statement_timeout:

        Abort any statement that takes more than the specified duration.
        If <varname>log_min_error_statement</varname> is set
        to <literal>ERROR</literal> or lower, the statement that timed out
        will also be logged.
        If the value is specified as a plain number, it is measured in
        milliseconds by default.
        A value of zero (the default) disables the timeout.

(I'm not quite sure whether the ending "by default" is worth writing
or not.)

Barring objections, I'll run around and make them all look like that.

            regards, tom lane

On Thu, Oct 24, 2019 at 02:37:03PM -0400, Tom Lane wrote:
> I think this is confusingly bad English, and it's poor exposition
> because a minor detail (it must be pretty minor, if we got away
> without mentioning it at all for years) is injected into the middle
> of the basic statement of the variable's purpose.  I think what we'd
> be better off doing is to write a separate sentence mentioning the
> units, in more or less the same way that we generally handle the
> default value.  In <14850.1571941169@sss.pgh.pa.us> I suggested
> this revision for statement_timeout:
> 
>         Abort any statement that takes more than the specified duration.
>         If <varname>log_min_error_statement</varname> is set
>         to <literal>ERROR</literal> or lower, the statement that timed out
>         will also be logged.
>         If the value is specified as a plain number, it is measured in
>         milliseconds by default.
>         A value of zero (the default) disables the timeout.
> 
> (I'm not quite sure whether the ending "by default" is worth writing
> or not.)
> 
> Barring objections, I'll run around and make them all look like that.

Thanks for applying the improvement in your patch cfb7559083.  I was
torn between it being a minor issue and not wanting to devote a new
sentence about it, which is why I awkwardly used parentheses.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Thu, Oct 24, 2019 at 02:37:03PM -0400, Tom Lane wrote:
> I think this is confusingly bad English, and it's poor exposition
> because a minor detail (it must be pretty minor, if we got away
> without mentioning it at all for years) is injected into the middle
> of the basic statement of the variable's purpose.  I think what we'd
> be better off doing is to write a separate sentence mentioning the
> units, in more or less the same way that we generally handle the
> default value.  In <14850.1571941169@sss.pgh.pa.us> I suggested
> this revision for statement_timeout:
> 
>         Abort any statement that takes more than the specified duration.
>         If <varname>log_min_error_statement</varname> is set
>         to <literal>ERROR</literal> or lower, the statement that timed out
>         will also be logged.
>         If the value is specified as a plain number, it is measured in
>         milliseconds by default.
>         A value of zero (the default) disables the timeout.
> 
> (I'm not quite sure whether the ending "by default" is worth writing
> or not.)
> 
> Barring objections, I'll run around and make them all look like that.

Thanks for applying the improvement in your patch cfb7559083.  I was
torn between it being a minor issue and not wanting to devote a new
sentence about it, which is why I awkwardly used parentheses.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +