Re: proposal: rounding up time value less than its unit.

On 9/23/14, 1:21 AM, David Johnston wrote:
> This patch should fix the round-to-zero issue.  If someone wants to 
> get rid of zero as a special case let them supply a separate patch for 
> that "improvement".

I am going to wander into this fresh after just reading everything once 
(but with more than a little practice with real-world GUC mucking) and 
say that, no, it should not even do that.  The original complaint here 
is real and can be straightforward to fix, but I don't like any of the 
proposals so far.

My suggestion:  fix the one really bad one in 9.5 with an adjustment to 
the units of log_rotation_age.  That's a small commit that should thank 
Tomonari Katsumata for discovering the issue and even suggesting a fix 
(that we don't use) and a release note; sample draft below.  Stop there.

Let someone with a broader objection take on the fact that zero (and -1) 
values have special properties, and that sucks, as a fully reasoned 
redesign.  I have a larger idea for minimizing rounding issues of 
timestamps in particular at the bottom of this too.  I wouldn't dare 
take on changes to rounding of integers without sorting out the special 
flag value issue first, because the variety of those in the database is 
large compared to the time ones.

== log_rotation_age ==

Back to where this started at 
http://www.postgresql.org/message-id/53992FF8.2060702@po.ntts.co.jp : 
log_rotation_age "would be disabled if we set it less than one minute", 
with this example of a surprising behavior:

log_rotation_age = 10s

postgres=# show log_rotation_age ;
log_rotation_age
------------------
0

That's bad and the GUC system is not being friendly; fully agreed. But 
this is just one where the resolution for the parameter is poor.  
log_rotation_age is the *only* GUC with a time interval that's set in 
minutes:

postgres=# SELECT name, unit FROM pg_settings WHERE unit ='min';       name       | unit
------------------+------ log_rotation_age | min

For comparison, there are 10 GUCs set in seconds and 13 in ms in HEAD.

We could just change the units for log_rotation_age to seconds, then let 
the person who asks for a 10s rotation time suffer for that decision 
only with many log files.  The person who instead chooses 10ms may find 
log rotation disabled altogether because that rounds to zero, but now we 
are not talking about surprises on something that seems sane--that's a 
fully unreasonable user request.

Following that style of fix all the way through to the sort of release 
notes needed would give something like this:

log_rotation_age is now set in units of seconds instead of minutes. 
Earlier installations of PostgreSQL that set this value directly, to a 
value in minutes, should change that setting to use a time unit before 
migrating to this version.

And we could be done for now.

== Flag values and error handling ==

I would like to see using 0 and -1 as special values in GUCs overhauled 
one day, with full disregard for backward compatibility as a 
straightjacket on doing the right thing.  This entire class of behavior 
is annoying.  But I am skeptical anything less than such an overhaul 
will really be useful.  To me it's not worth adding new code, 
documentation, and associated release notes to guide migration all to 
try and describe new rounding trivia--which I don't see as better nor 
worse than the trade-offs of what happens today.

I can't take the idea of throwing errors for something this minor 
seriously at all.  There are a lot more precedents for spreading 
settings around in a few places now, from include_dir to 
postgresql.auto.conf.  There are so many ways to touch a config now and 
not notice the error message now, I really don't want to see any more 
situations appear where trying to change a setting will result in a 
broken file added into that mix.  None of this broken units due to 
rounding stuff that I've found, now that I went out of my way looking 
for some, even comes close to rising to that level of serious to me.  
Only this log rotation one is so badly out of line that it will act 
quite badly.

== Overhauling all time unit GUCs ==

Here's the complete list of potential ugly time settings to review in 
the future, if my suggestion of only fixing log_rotation_age were followed:

gsmith=# SELECT name,setting, unit, min_val FROM pg_settings where unit 
in ('s','ms') and (min_val::integer)<=0 order by unit,min_val,name;             name             | setting | unit |
min_val
------------------------------+---------+------+--------- autovacuum_vacuum_cost_delay | 20      | ms   | -1
log_autovacuum_min_duration | -1      | ms   | -1 log_min_duration_statement   | -1      | ms   | -1
max_standby_archive_delay   | 30000   | ms   | -1 max_standby_streaming_delay  | 30000   | ms   | -1 lock_timeout
         | 0       | ms   | 0 statement_timeout            | 0       | ms   | 0 vacuum_cost_delay            | 0
|ms   | 0 wal_receiver_timeout         | 60000   | ms   | 0 wal_sender_timeout           | 60000   | ms   | 0
archive_timeout             | 0       | s    | 0 checkpoint_warning           | 30      | s    | 0 post_auth_delay
       | 0       | s    | 0 pre_auth_delay               | 0       | s    | 0 tcp_keepalives_idle          | 0       |
s   | 0 tcp_keepalives_interval      | 0       | s    | 0 wal_receiver_status_interval | 10      | s    | 0
 

I already had my eye on doing something about the vacuum ones, and I may 
even get to that in time for 9.5.

If I were feeling ambitious about breaking configurations with a 
long-term eye toward improvement, I'd be perfectly happy converting 
everything on this list to ms.  We live in 64 bit integer land now; who 
cares if the numbers are bigger?

Then the rounding issues only impact sub-millisecond values, making all 
them squarely in nonsense setting land.  Users will be pushed very 
clearly to always use time units in their postgresql.conf files instead 
of guessing whether something is set in ms vs. seconds.  Seeing the 
reaction to a units change on log_rotation_age might be a useful test 
for how that sort of change plays out in the real world.

-- 
Greg Smith greg.smith@crunchydatasolutions.com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/