units in postgresql.conf comments

On Wed, May 29, 2013 at 09:59:10PM -0400, Peter Eisentraut wrote:

I think these sort of entries don't make much sense:

#wal_sender_timeout = 60s # in milliseconds; 0 disables

I think we should remove units from the comments when it's clear from
the name or the default value that time units are accepted.

We are documenting what happens when there are no units. Are people are
going to change '60s' to '50' and assume that is '50s'? Hopefully not.
I do like the clutter avoidance of removing the units from the comments.

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

+ It's impossible for everything to be true. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 30.05.2013 06:43, Bruce Momjian wrote:

On Wed, May 29, 2013 at 09:59:10PM -0400, Peter Eisentraut wrote:

I think these sort of entries don't make much sense:

#wal_sender_timeout = 60s # in milliseconds; 0 disables

I think we should remove units from the comments when it's clear from
the name or the default value that time units are accepted.

We are documenting what happens when there are no units. Are people are
going to change '60s' to '50' and assume that is '50s'? Hopefully not.
I do like the clutter avoidance of removing the units from the comments.

We could make it mandatory to specify the unit in the value. Ie. throw
an error on "wal_sender_timeout = 50":

ERROR: unit required for option "wal_sender_timeout"
HINT: Valid units for this parameter are "ms", "s", "min", "h", and "d".

Then you wouldn't need a comment to explain what the unit of a naked
value is. The only problem I see with that is backwards-compatibility.
Old postgresql.conf files containing naked values would no longer work.
But all you'd need to do is to add in the units, which would work on
older versions too, and would be good for readability anyway.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 05/30/2013 12:01 AM, Heikki Linnakangas wrote:

We could make it mandatory to specify the unit in the value. Ie. throw
an error on "wal_sender_timeout = 50":

ERROR: unit required for option "wal_sender_timeout"
HINT: Valid units for this parameter are "ms", "s", "min", "h", and "d".

Then you wouldn't need a comment to explain what the unit of a naked
value is. The only problem I see with that is backwards-compatibility.
Old postgresql.conf files containing naked values would no longer work.
But all you'd need to do is to add in the units, which would work on
older versions too, and would be good for readability anyway.

I like this idea with one addition. We should have a default unit for
each. For wal_sender_timeout seconds makes sense, but for
checkpoint_timeout minutes makes sense (for example).

JD

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, May 30, 2013 at 3:52 AM, Joshua D. Drake <jd@commandprompt.com> wrote:

On 05/30/2013 12:01 AM, Heikki Linnakangas wrote:

We could make it mandatory to specify the unit in the value. Ie. throw
an error on "wal_sender_timeout = 50":

ERROR: unit required for option "wal_sender_timeout"
HINT: Valid units for this parameter are "ms", "s", "min", "h", and "d".

Then you wouldn't need a comment to explain what the unit of a naked
value is. The only problem I see with that is backwards-compatibility.
Old postgresql.conf files containing naked values would no longer work.
But all you'd need to do is to add in the units, which would work on
older versions too, and would be good for readability anyway.

In general, I like this. Requiring "full specification" is never
wrong. Except possibly for thje backwards compatible thing.

I like this idea with one addition. We should have a default unit for each.
For wal_sender_timeout seconds makes sense, but for checkpoint_timeout
minutes makes sense (for example).

This sounds like a good way to make things even more confusing. Right
now the confusion is only in the comments - this would make it
confusing in the actual values.

Requiring a unit seems like a much better idea. That way, there is no
way for confusion.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 05/30/2013 12:55 AM, Magnus Hagander wrote:

I like this idea with one addition. We should have a default unit for each.
For wal_sender_timeout seconds makes sense, but for checkpoint_timeout
minutes makes sense (for example).

This sounds like a good way to make things even more confusing. Right
now the confusion is only in the comments - this would make it
confusing in the actual values.

Requiring a unit seems like a much better idea. That way, there is no
way for confusion.

I can buy into that.

JD

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 30.05.2013 10:52, Joshua D. Drake wrote:

On 05/30/2013 12:01 AM, Heikki Linnakangas wrote:

We could make it mandatory to specify the unit in the value. Ie. throw
an error on "wal_sender_timeout = 50":

ERROR: unit required for option "wal_sender_timeout"
HINT: Valid units for this parameter are "ms", "s", "min", "h", and "d".

Then you wouldn't need a comment to explain what the unit of a naked
value is. The only problem I see with that is backwards-compatibility.
Old postgresql.conf files containing naked values would no longer work.
But all you'd need to do is to add in the units, which would work on
older versions too, and would be good for readability anyway.

I like this idea with one addition. We should have a default unit for
each. For wal_sender_timeout seconds makes sense, but for
checkpoint_timeout minutes makes sense (for example).

Uh, if specifying the unit is mandatory, what exactly would the default
unit mean?

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 05/30/2013 01:14 AM, Heikki Linnakangas wrote:

On 30.05.2013 10:52, Joshua D. Drake wrote:

On 05/30/2013 12:01 AM, Heikki Linnakangas wrote:

We could make it mandatory to specify the unit in the value. Ie. throw
an error on "wal_sender_timeout = 50":

ERROR: unit required for option "wal_sender_timeout"
HINT: Valid units for this parameter are "ms", "s", "min", "h", and "d".

Then you wouldn't need a comment to explain what the unit of a naked
value is. The only problem I see with that is backwards-compatibility.
Old postgresql.conf files containing naked values would no longer work.
But all you'd need to do is to add in the units, which would work on
older versions too, and would be good for readability anyway.

I like this idea with one addition. We should have a default unit for
each. For wal_sender_timeout seconds makes sense, but for
checkpoint_timeout minutes makes sense (for example).

Uh, if specifying the unit is mandatory, what exactly would the default
unit mean?

Yeah, see my other email. I missed that part. It is late for me. Sorry
for the noise.

JD

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, May 29, 2013 at 09:59:10PM -0400, Peter Eisentraut wrote:

I think these sort of entries don't make much sense:

#wal_sender_timeout = 60s # in milliseconds; 0 disables

I think we should remove units from the comments when it's clear from
the name or the default value that time units are accepted.

So, is anyone doing this? Should it be a TODO item?

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

+ Everyone has their own god. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Bruce Momjian <bruce@momjian.us> writes:

On Wed, May 29, 2013 at 09:59:10PM -0400, Peter Eisentraut wrote:

I think these sort of entries don't make much sense:

#wal_sender_timeout = 60s # in milliseconds; 0 disables

I think we should remove units from the comments when it's clear from
the name or the default value that time units are accepted.

So, is anyone doing this? Should it be a TODO item?

I think Peter's wrong here, for two reasons:

* The comment tells you what undecorated "wal_sender_timeout = 60" will
do.

* The comment tells you what the precision of the setting is. For
instance, archive_timeout is in seconds; you can try setting it to "10ms"
if you like, but that won't do much for you.

We could imagine making these points moot, by disallowing inputs that lack
units and converting all time GUCs into some common scale (requiring
wider-than-int storage) ... but that seems sufficiently non backward
compatible that I don't see it happening. It's not clear that it'd be a
usability improvement anyway.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 01/11/2014 11:06 AM, Bruce Momjian wrote:

On Wed, May 29, 2013 at 09:59:10PM -0400, Peter Eisentraut wrote:

I think these sort of entries don't make much sense:

#wal_sender_timeout = 60s # in milliseconds; 0 disables

I think we should remove units from the comments when it's clear from
the name or the default value that time units are accepted.

So, is anyone doing this? Should it be a TODO item?

I don't agree, actually, unless we take the next step and actually clean
all the documentation garbage out of the file and leave it in the main
docs and pg_settings where it belongs.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers