Thread: units in postgresql.conf comments
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.
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. +
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
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 > >
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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
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/ > >
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
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 >
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. +
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
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