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(dot)smith(at)crunchydatasolutions(dot)com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/