Re: log_min_duration_statement units

Someone complained that the default units for log_min_duration_statement
was not mentioned in postgresql.conf; this applied patch fixes that.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> ! #log_min_duration_statement = -1 # -1 is disabled, 0 logs all statements

> ! #log_min_duration_statement = -1ms # -1 is disabled, 0 logs all statements

This is not sensible -- there are no units for the "-1" setting. I
seem to recall that it won't even work; did you test uncommenting that?

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > ! #log_min_duration_statement = -1 # -1 is disabled, 0 logs all statements
>
> > ! #log_min_duration_statement = -1ms # -1 is disabled, 0 logs all statements
>
> This is not sensible -- there are no units for the "-1" setting. I
> seem to recall that it won't even work; did you test uncommenting that?

I tested it uncommented and manually and it worked fine:

test=> set log_min_duration_statement = '-1ms';
SET
test=> show log_min_duration_statement;
log_min_duration_statement
----------------------------
-1
(1 row)

It seems illogical to document it this way but a non-EDB trainer emailed
me saying students are getting confused about the default units, and I
can see how they would get confused.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian escreveu:
> Someone complained that the default units for log_min_duration_statement
> was not mentioned in postgresql.conf; this applied patch fixes that.
>
IMHO -1ms doesn't make much sense. Also, some other parameters don't have
units too. They are: archive_timeout, autovacuum_vacuum_cost_delay,
log_autovacuum_min_duration, log_temp_files, statement_timeout,
tcp_keepalives_idle, and tcp_keepalives_interval. What about mention the unit
in the comment that follows it? The last two already have it.

--
Euler Taveira de Oliveira
http://www.timbira.com/

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> It seems illogical to document it this way

Yeah, that's what I thought, so why did you do it that way?
You could do something like

#log_min_duration_statement = -1 # -1 is disabled, 0 logs all statements
# default unit is ms

regards, tom lane

Euler Taveira de Oliveira wrote:
> Bruce Momjian escreveu:
> > Someone complained that the default units for log_min_duration_statement
> > was not mentioned in postgresql.conf; this applied patch fixes that.
> >
> IMHO -1ms doesn't make much sense. Also, some other parameters don't have
> units too. They are: archive_timeout, autovacuum_vacuum_cost_delay,
> log_autovacuum_min_duration, log_temp_files, statement_timeout,
> tcp_keepalives_idle, and tcp_keepalives_interval. What about mention the unit
> in the comment that follows it? The last two already have it.

Thanks for the list! Here is an applied patch that cleans these up,
plus fixes log_min_duration_statement.
--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Euler Taveira de Oliveira wrote:
> > Bruce Momjian escreveu:
> > > Someone complained that the default units for log_min_duration_statement
> > > was not mentioned in postgresql.conf; this applied patch fixes that.
> > >
> > IMHO -1ms doesn't make much sense. Also, some other parameters don't have
> > units too. They are: archive_timeout, autovacuum_vacuum_cost_delay,
> > log_autovacuum_min_duration, log_temp_files, statement_timeout,
> > tcp_keepalives_idle, and tcp_keepalives_interval. What about mention the unit
> > in the comment that follows it? The last two already have it.
>
> Thanks for the list! Here is an applied patch that cleans these up,
> plus fixes log_min_duration_statement.

Oops, here is the patch; Euler, would you look to see if you can find
anything else missing; you seem to be good at that. :-)

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian escreveu:
> Oops, here is the patch; Euler, would you look to see if you can find
> anything else missing; you seem to be good at that. :-)
>
My English is not as good as yours but here is another try. Personally, I
prefer the second one but...

--
Euler Taveira de Oliveira
http://www.timbira.com/

Euler Taveira de Oliveira wrote:
> Bruce Momjian escreveu:
> > Oops, here is the patch; Euler, would you look to see if you can find
> > anything else missing; you seem to be good at that. :-)
> >
> My English is not as good as yours but here is another try. Personally, I
> prefer the second one but...

Great, I used your second version. I had already done some of the ones
you found, but you had many more. I also used your "0 disables" wording
consistently in the file. Thanks for the help. Committed.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Euler Taveira de Oliveira wrote:
> > Bruce Momjian escreveu:
> > > Oops, here is the patch; Euler, would you look to see if you can find
> > > anything else missing; you seem to be good at that. :-)
> > >
> > My English is not as good as yours but here is another try. Personally, I
> > prefer the second one but...
>
> Great, I used your second version. I had already done some of the ones
> you found, but you had many more. I also used your "0 disables" wording
> consistently in the file. Thanks for the help. Committed.

Huh ... I thought the units were removed from the descriptions when the
whole unit thing got introduced in guc.c. This seems more a regression
than anything else -- see guc.c rev 1.361.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > Euler Taveira de Oliveira wrote:
> > > Bruce Momjian escreveu:
> > > > Oops, here is the patch; Euler, would you look to see if you can find
> > > > anything else missing; you seem to be good at that. :-)
> > > >
> > > My English is not as good as yours but here is another try. Personally, I
> > > prefer the second one but...
> >
> > Great, I used your second version. I had already done some of the ones
> > you found, but you had many more. I also used your "0 disables" wording
> > consistently in the file. Thanks for the help. Committed.
>
> Huh ... I thought the units were removed from the descriptions when the
> whole unit thing got introduced in guc.c. This seems more a regression
> than anything else -- see guc.c rev 1.361.

Which is why I originally did:

#log_min_duration_statement = -1ms # -1 is disabled, 0 logs all statements

but people complained so I went with adding it to the description. The
units are only to document the default if no units are supplied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Euler Taveira de Oliveira wrote:
>> My English is not as good as yours but here is another try. Personally, I
>> prefer the second one but...

> Great, I used your second version. I had already done some of the ones
> you found, but you had many more. I also used your "0 disables" wording
> consistently in the file. Thanks for the help. Committed.

You know, it suddenly strikes me that this is going in largely the wrong
direction. Wasn't a key part of the reasoning for the GUC units support
to *eliminate* the need for people to know what the underlying
variable's unit is? I certainly think that putting the unit info into
the text descriptions is a seriously bad idea. It makes an already
overly wide view even wider, and the information is 100% redundant with
the "unit" column of the pg_settings view.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Euler Taveira de Oliveira wrote:
> >> My English is not as good as yours but here is another try. Personally, I
> >> prefer the second one but...
>
> > Great, I used your second version. I had already done some of the ones
> > you found, but you had many more. I also used your "0 disables" wording
> > consistently in the file. Thanks for the help. Committed.
>
> You know, it suddenly strikes me that this is going in largely the wrong
> direction. Wasn't a key part of the reasoning for the GUC units support
> to *eliminate* the need for people to know what the underlying
> variable's unit is? I certainly think that putting the unit info into
> the text descriptions is a seriously bad idea. It makes an already
> overly wide view even wider, and the information is 100% redundant with
> the "unit" column of the pg_settings view.

Right, the problem particularly is with the -1/special values that don't
have a real unit.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> You know, it suddenly strikes me that this is going in largely the wrong
>> direction.

> Right, the problem particularly is with the -1/special values that don't
> have a real unit.

I don't object to what you did to postgresql.conf.sample, but I think
you should revert the changes to the descriptions in guc.c.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> You know, it suddenly strikes me that this is going in largely the wrong
> >> direction.
>
> > Right, the problem particularly is with the -1/special values that don't
> > have a real unit.
>
> I don't object to what you did to postgresql.conf.sample, but I think
> you should revert the changes to the descriptions in guc.c.

Agreed; attached patch applied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > Tom Lane wrote:
> > >> You know, it suddenly strikes me that this is going in largely the wrong
> > >> direction.
> >
> > > Right, the problem particularly is with the -1/special values that don't
> > > have a real unit.
> >
> > I don't object to what you did to postgresql.conf.sample, but I think
> > you should revert the changes to the descriptions in guc.c.
>
> Agreed; attached patch applied.

However, keep in mind that units are _not_ displayed for zero values:

test=> show statement_timeout;
statement_timeout
-------------------
0
(1 row)

test=> set statement_timeout = 100;
SET
test=> show statement_timeout;
statement_timeout
-------------------
100ms
(1 row)

because of this test in guc.c:

else if (use_units && result > 0 &&
(record->flags & GUC_UNIT_TIME))

pg_settings does show the units. Changing this to display the units
might be odd since many zero values mean 'disabled'.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> However, keep in mind that units are _not_ displayed for zero values:

Well, zero is zero, so that seems fine.

regards, tom lane