log_min_duration_statement units

Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce@momjian.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@momjian.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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@momjian.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce@momjian.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