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

Hi,

Several couple weeks ago, I posted a mail to pgsql-doc.
http://www.postgresql.org/message-id/53992FF8.2060702@po.ntts.co.jp

In this thread, I concluded that it's better to
round up the value which is less than its unit.
Current behavior (rounding down) has undesirable setting risk,
because some GUCs have special meaning for 0.

And then I made a patch for this.
Please check the attached patch.

regards,
-----------
Tomonari Katsumata

On Thu, Jul 10, 2014 at 2:52 AM, Tomonari Katsumata
<katsumata(dot)tomonari(at)po(dot)ntts(dot)co(dot)jp> wrote:
> Several couple weeks ago, I posted a mail to pgsql-doc.
> http://www.postgresql.org/message-id/53992FF8.2060702@po.ntts.co.jp
>
> In this thread, I concluded that it's better to
> round up the value which is less than its unit.
> Current behavior (rounding down) has undesirable setting risk,
> because some GUCs have special meaning for 0.
>
> And then I made a patch for this.
> Please check the attached patch.

Thanks for the patch. Please add it here:

https://commitfest.postgresql.org/action/commitfest_view/open

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Robert,

Thank you for checking this!

I've added it to commitfest.
https://commitfest.postgresql.org/action/patch_view?id=1507

regards,
------------
Tomonari Katsumata

2014-07-12 6:07 GMT+09:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Thu, Jul 10, 2014 at 2:52 AM, Tomonari Katsumata
> <katsumata(dot)tomonari(at)po(dot)ntts(dot)co(dot)jp> wrote:
> > Several couple weeks ago, I posted a mail to pgsql-doc.
> > http://www.postgresql.org/message-id/53992FF8.2060702@po.ntts.co.jp
> >
> > In this thread, I concluded that it's better to
> > round up the value which is less than its unit.
> > Current behavior (rounding down) has undesirable setting risk,
> > because some GUCs have special meaning for 0.
> >
> > And then I made a patch for this.
> > Please check the attached patch.
>
> Thanks for the patch. Please add it here:
>
> https://commitfest.postgresql.org/action/commitfest_view/open
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On 07/10/2014 09:52 AM, Tomonari Katsumata wrote:
> Hi,
>
> Several couple weeks ago, I posted a mail to pgsql-doc.
> http://www.postgresql.org/message-id/53992FF8.2060702@po.ntts.co.jp
>
> In this thread, I concluded that it's better to
> round up the value which is less than its unit.
> Current behavior (rounding down) has undesirable setting risk,
> because some GUCs have special meaning for 0.
>
> And then I made a patch for this.
> Please check the attached patch.

The patch also rounds a zero up to one. A naked zero with no unit is not
affected, but e.g if you have "log_rotation_age=0s", it will not disable
the feature as you might expect, but set it to 1 minute. Should we do
something about that?

If we're going to explain the rounding up in the manual, we also need to
explain the normal rule, which is to round down. How about this:

--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -44,6 +44,15 @@
(seconds), <literal>min</literal> (minutes), <literal>h</literal>
(hours), and <literal>d</literal> (days). Note that the multiplier
for memory units is 1024, not 1000.
+
+ <para>
+ If a memory or time setting is specified with more precision than the
+ implicit unit of the setting, it is rounded down. However, if
rounding
+ down would yield a zero, it is rounded up to one instead. For
example,
+ the implicit unit of <varname>log_rotation_age</varname is
minutes, so if
+ you set it to <literal>150s</literal>, it will be rounded down to two
+ minutes. However, if you set it to <literal>10s</literal>, it will be
+ rounded up to one minute.
</para>

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> The patch also rounds a zero up to one. A naked zero with no unit is not
> affected, but e.g if you have "log_rotation_age=0s", it will not disable
> the feature as you might expect, but set it to 1 minute. Should we do
> something about that?

That sounds like a dealbreaker to me. There are enough places where zero
has special meaning that we should not *ever* change zero to non-zero
silently.

regards, tom lane

On 8/21/14 11:16 AM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>> The patch also rounds a zero up to one. A naked zero with no unit is not
>> affected, but e.g if you have "log_rotation_age=0s", it will not disable
>> the feature as you might expect, but set it to 1 minute. Should we do
>> something about that?
>
> That sounds like a dealbreaker to me. There are enough places where zero
> has special meaning that we should not *ever* change zero to non-zero
> silently.

I don't think I like this idea anyway. If something has units of an
hour and the user (perhaps misunderstanding the setting) sets it to one
second, then we shouldn't silently change that to one hour.

If there is a problem with rounding it to zero, then we should perhaps
raise an error. (And stop treating zero specially. It's a terrible idea.)

Peter Eisentraut-2 wrote
> On 8/21/14 11:16 AM, Tom Lane wrote:
>> Heikki Linnakangas &lt;

> hlinnakangas@

> &gt; writes:
>>> The patch also rounds a zero up to one. A naked zero with no unit is not
>>> affected, but e.g if you have "log_rotation_age=0s", it will not disable
>>> the feature as you might expect, but set it to 1 minute. Should we do
>>> something about that?
>>
>> That sounds like a dealbreaker to me. There are enough places where zero
>> has special meaning that we should not *ever* change zero to non-zero
>> silently.
>
> I don't think I like this idea anyway. If something has units of an
> hour and the user (perhaps misunderstanding the setting) sets it to one
> second, then we shouldn't silently change that to one hour.
>
> If there is a problem with rounding it to zero, then we should perhaps
> raise an error. (And stop treating zero specially. It's a terrible
> idea.)

I'm on board, from the original thread, for errors if the input cannot be
converted to the parameter measurement unit cleanly. By which I mean the
specified value should result in an integer being recorded without rounding.
Specifying a precision less than the default unit thus becomes impossible.

I don't have a problem with zero meaning disabled when appropriate since it
avoids having a separate on/off GUC.

That said the complaint here just seems like a bug in the supplied patch -
zero is zero regardless of whether a unit is specified. The only obvious
exception would be temperature but that isn't relevant here.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/proposal-rounding-up-time-value-less-than-its-unit-tp5811102p5815770.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

Thank you for the comments.

It was a bug in my patch as another developer says.
I've not considered about the value 'zero', sorry.

I attached new patch.
This patch rounds up the value when only it's less than required unit.
Like below.
(unit: min)
0->0
0s->0
10s->1
70s->1

Although my original complaint is fixed, I'm worried about this change will
make users confusing.
Is it better to raise a message(ex. INFO) when a value less than required
unit is set?

2014-08-21 21:00 GMT+09:00 Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>:

> On 07/10/2014 09:52 AM, Tomonari Katsumata wrote:
>
>> Hi,
>>
>> Several couple weeks ago, I posted a mail to pgsql-doc.
>> http://www.postgresql.org/message-id/53992FF8.2060702@po.ntts.co.jp
>>
>> In this thread, I concluded that it's better to
>> round up the value which is less than its unit.
>> Current behavior (rounding down) has undesirable setting risk,
>> because some GUCs have special meaning for 0.
>>
>> And then I made a patch for this.
>> Please check the attached patch.
>>
>
> The patch also rounds a zero up to one. A naked zero with no unit is not
> affected, but e.g if you have "log_rotation_age=0s", it will not disable
> the feature as you might expect, but set it to 1 minute. Should we do
> something about that?
>
> If we're going to explain the rounding up in the manual, we also need to
> explain the normal rule, which is to round down. How about this:
>
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -44,6 +44,15 @@
> (seconds), <literal>min</literal> (minutes), <literal>h</literal>
> (hours), and <literal>d</literal> (days). Note that the multiplier
> for memory units is 1024, not 1000.
> +
> + <para>
> + If a memory or time setting is specified with more precision than the
> + implicit unit of the setting, it is rounded down. However, if
> rounding
> + down would yield a zero, it is rounded up to one instead. For
> example,
> + the implicit unit of <varname>log_rotation_age</varname is minutes,
> so if
> + you set it to <literal>150s</literal>, it will be rounded down to two
> + minutes. However, if you set it to <literal>10s</literal>, it will be
> + rounded up to one minute.
> </para>
>
> - Heikki
>
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Tomonari Katsumata <t(dot)katsumata1122(at)gmail(dot)com> writes:
> This patch rounds up the value when only it's less than required unit.
> ..
> Although my original complaint is fixed, I'm worried about this change will
> make users confusing.

Indeed. I have not understood why you are insisting on "round up"
semantics. Wouldn't it make more sense for the behavior to be "round to
nearest"? That would get rid of any worries about treating zero specially.

> Is it better to raise a message(ex. INFO) when a value less than required
> unit is set?

No. Non-error messages are probably completely useless in this area:
users will typically never see such messages for settings made in
postgresql.conf, because it will not occur to them to look in the
postmaster log. So the behavior needs to be self-explanatory without
any messages; and that means it had better not be very complicated.

regards, tom lane

Tom Lane-2 wrote
> Tomonari Katsumata &lt;

> t.katsumata1122@

> &gt; writes:
>> This patch rounds up the value when only it's less than required unit.
>> ..
>> Although my original complaint is fixed, I'm worried about this change
>> will
>> make users confusing.
>
> Indeed. I have not understood why you are insisting on "round up"
> semantics. Wouldn't it make more sense for the behavior to be "round to
> nearest"? That would get rid of any worries about treating zero
> specially.

Wasn't the goal that all non-zero values result in the feature being
enabled? With round nearest there will still be some values that are
non-zero but that round to zero and thus disable the feature.

Values failing in the range (0, 1) in the canonical unit must be treated
specially otherwise we might as well just leave the current behavior as-is
since floor is likely just as good a rule as round-nearest.

For fractions greater than one round nearest is probably fine and indeed on
average results in the least amount of potential adjustment magnitude.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/proposal-rounding-up-time-value-less-than-its-unit-tp5811102p5816007.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

David G Johnston <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> Tom Lane-2 wrote
>> Indeed. I have not understood why you are insisting on "round up"
>> semantics. Wouldn't it make more sense for the behavior to be "round to
>> nearest"? That would get rid of any worries about treating zero
>> specially.

> Wasn't the goal that all non-zero values result in the feature being
> enabled? With round nearest there will still be some values that are
> non-zero but that round to zero and thus disable the feature.

Ah. Okay, but then what's wrong with the original proposal of "use ceil()
instead of floor()"? Basically I think the idea of treating fractions
less than one differently from fractions greater than one is bogus; nobody
will ever find that intuitive.

Or we could adopt Peter's idea that zero shouldn't be special (instead
using, say, -1 to turn things off). But I'm afraid that would break way
too many peoples' configuration choices.

regards, tom lane

On Sat, Aug 23, 2014 at 9:38 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Ah. Okay, but then what's wrong with the original proposal of "use ceil()
> instead of floor()"? Basically I think the idea of treating fractions
> less than one differently from fractions greater than one is bogus; nobody
> will ever find that intuitive.

Or make it an error to specify a value that rounds to 0 but isn't 0.

--
greg

Tom Lane-2 wrote
> David G Johnston &lt;

> david.g.johnston@

> &gt; writes:
>> Tom Lane-2 wrote
>>> Indeed. I have not understood why you are insisting on "round up"
>>> semantics. Wouldn't it make more sense for the behavior to be "round to
>>> nearest"? That would get rid of any worries about treating zero
>>> specially.
>
>> Wasn't the goal that all non-zero values result in the feature being
>> enabled? With round nearest there will still be some values that are
>> non-zero but that round to zero and thus disable the feature.
>
> Ah. Okay, but then what's wrong with the original proposal of "use ceil()
> instead of floor()"? Basically I think the idea of treating fractions
> less than one differently from fractions greater than one is bogus; nobody
> will ever find that intuitive.
>
> Or we could adopt Peter's idea that zero shouldn't be special (instead
> using, say, -1 to turn things off). But I'm afraid that would break way
> too many peoples' configuration choices.

Yes, using ceil() is the most acceptable, for me, solution that does not
involve throwing an error.

Are there any examples of where treating zero specially is a problem or is
this concern theoretical? We've already decided to risk enabling disabled
features by applying this patch since the likelihood of someone relying on
the rounding to keep the feature disabled (or at its default value in some
instances) is unlikely.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/proposal-rounding-up-time-value-less-than-its-unit-tp5811102p5816018.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On Sat, Aug 23, 2014 at 6:39 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> On Sat, Aug 23, 2014 at 9:38 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>> Ah. Okay, but then what's wrong with the original proposal of "use ceil()
>> instead of floor()"? Basically I think the idea of treating fractions
>> less than one differently from fractions greater than one is bogus; nobody
>> will ever find that intuitive.
>
> Or make it an error to specify a value that rounds to 0 but isn't 0.

I liked David Johnston's even stronger suggestion upthread: make it an
error to specify a value requires rounding of any kind. In other
words, if the minimum granularity is 1 minute, you can specify that as
60 seconds instead, but if you write 59 seconds, we error out. Maybe
that seems pedantic, but I don't think users will much appreciate the
discovery that 30 seconds means 60 seconds. They'll be happier to be
told that up front than having to work it out afterward.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-08-26 16:16:32 -0400, Robert Haas wrote:
> On Sat, Aug 23, 2014 at 6:39 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> > On Sat, Aug 23, 2014 at 9:38 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>
> >> Ah. Okay, but then what's wrong with the original proposal of "use ceil()
> >> instead of floor()"? Basically I think the idea of treating fractions
> >> less than one differently from fractions greater than one is bogus; nobody
> >> will ever find that intuitive.
> >
> > Or make it an error to specify a value that rounds to 0 but isn't 0.
>
> I liked David Johnston's even stronger suggestion upthread: make it an
> error to specify a value requires rounding of any kind. In other
> words, if the minimum granularity is 1 minute, you can specify that as
> 60 seconds instead, but if you write 59 seconds, we error out. Maybe
> that seems pedantic, but I don't think users will much appreciate the
> discovery that 30 seconds means 60 seconds. They'll be happier to be
> told that up front than having to work it out afterward.

Is the whole topic actually practically relevant? Afaics there's no
guc's with a time unit bigger than GUC_UNIT_S except log_rotation_age
where it surely doesn't matter and no space unit bigger than
GUC_UNIT_BLOCKS/GUC_UNIT_XBLOCKS.
In theory I agree with you/$subject, but I don't really see this worth
much effort.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 8/23/14 6:39 PM, Greg Stark wrote:
> Or make it an error to specify a value that rounds to 0 but isn't 0.

That's what I would prefer.

On 8/26/14 4:22 PM, Andres Freund wrote:
> Is the whole topic actually practically relevant?

It's clearly not all that important, or otherwise we'd have heard about
before now.

I suppose someone could do something like

wal_receiver_status_interval = 10ms

and end up silently turning the whole thing off instead of making it
very aggressive.

The mistake here is that the mathematically appropriate turn-off value
in this and similar cases is infinity, not zero.

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I liked David Johnston's even stronger suggestion upthread: make it an
> error to specify a value requires rounding of any kind. In other
> words, if the minimum granularity is 1 minute, you can specify that as
> 60 seconds instead, but if you write 59 seconds, we error out. Maybe
> that seems pedantic, but I don't think users will much appreciate the
> discovery that 30 seconds means 60 seconds. They'll be happier to be
> told that up front than having to work it out afterward.

I think this is totally wrong. The entire point of the GUC units system,
or at least a large part of the point, is that users should not have to
be intimately aware of the units in which a given value is measured
internally. And that in turn means that the units had better be such
that users won't find them overly coarse. If it matters a lot whether
59 seconds gets rounded to 60, then we didn't make a good choice of units
for the GUC in question; and we should fix that choice, not mess with the
rounding rules.

The case where this argument falls down is for "special" values, such as
where zero means something quite different from the smallest nonzero
value. Peter suggested upthread that we should redefine any GUC values
for which that is true, but (a) I think that loses on backwards
compatibility grounds, and (b) ISTM zero is probably always special to
some extent. A zero time delay for example is not likely to work.

Maybe we should leave the rounding behavior alone (there's not much
evidence that rounding in one direction is worse than another; although
I'd also be okay with changing to round-to-nearest), and confine ourselves
to throwing an error for the single case that an apparently nonzero input
value is truncated/rounded to zero as a result of units conversion.

regards, tom lane

Tom Lane-2 wrote
> Robert Haas &lt;

> robertmhaas@

> &gt; writes:
>> I liked David Johnston's even stronger suggestion upthread: make it an
>> error to specify a value requires rounding of any kind. In other
>> words, if the minimum granularity is 1 minute, you can specify that as
>> 60 seconds instead, but if you write 59 seconds, we error out. Maybe
>> that seems pedantic, but I don't think users will much appreciate the
>> discovery that 30 seconds means 60 seconds. They'll be happier to be
>> told that up front than having to work it out afterward.
>
> I think this is totally wrong. The entire point of the GUC units system,
> or at least a large part of the point, is that users should not have to
> be intimately aware of the units in which a given value is measured
> internally. And that in turn means that the units had better be such
> that users won't find them overly coarse. If it matters a lot whether
> 59 seconds gets rounded to 60, then we didn't make a good choice of units
> for the GUC in question; and we should fix that choice, not mess with the
> rounding rules.
>
> The case where this argument falls down is for "special" values, such as
> where zero means something quite different from the smallest nonzero
> value. Peter suggested upthread that we should redefine any GUC values
> for which that is true, but (a) I think that loses on backwards
> compatibility grounds, and (b) ISTM zero is probably always special to
> some extent. A zero time delay for example is not likely to work.
>
> Maybe we should leave the rounding behavior alone (there's not much
> evidence that rounding in one direction is worse than another; although
> I'd also be okay with changing to round-to-nearest), and confine ourselves
> to throwing an error for the single case that an apparently nonzero input
> value is truncated/rounded to zero as a result of units conversion.

To Andres' point:

SELECT unit, count(*) FROM pg_settings WHERE unit <> '' GROUP BY unit; (9.3
/ Ubuntu)

min (1 - log_rotation_age)
s (10)
ms (13)

kb (7)
8kb (6)

I don't know about the size implications but they seem to be non-existent.
That any setting critically matters at +/- 1s or 1ms doesn't seem likely in
practice. Even +/- 1min for a setting, if it did matter at extreme scale,
would be recognizable by the user in practice as a rounding artifact and
compensated for.

At this point throwing an error for any precision that results in less than
the default precision is my preference. I would not change the rounding
rules for the simple reason that there is no obvious improvement to be had
and so why introduce pointless change that - however marginal and unlikely -
will be user-visible.

The complaint to overcome is avoiding an interpretation of "zero" when the
precision of the input is less than the GUC unit. Lacking any concrete
complaints about our round-down policy I don't see where a change there is
worthwhile.

Fixing zero as a special value falls under the same category. As
mathematically pure as using infinity may be the trade-off for practicality
and usability seems, even in light of this complaint, like the correct one
to have made.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/proposal-rounding-up-time-value-less-than-its-unit-tp5811102p5816409.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

Hi,

I'm sorry for slow reaction.

I don't care whether rounding up or down it, although this title has
'rounding up'.
(I just only come up with it. I'm sorry for my imprudence)

I'm thinking about a method which users get quick awareness it.
Now, it's okay not to change current behavior except non-zero value yields
a zero. A zero rounded down from non-zero gets an error.

I attached new patch.
This includes a document about above behavior as Heikki suggested.

regards,
--------------
Tomonari Katsumata

2014-08-27 6:49 GMT+09:00 David G Johnston <david(dot)g(dot)johnston(at)gmail(dot)com>:

> Tom Lane-2 wrote
> > Robert Haas &lt;
>
> > robertmhaas@
>
> > &gt; writes:
> >> I liked David Johnston's even stronger suggestion upthread: make it an
> >> error to specify a value requires rounding of any kind. In other
> >> words, if the minimum granularity is 1 minute, you can specify that as
> >> 60 seconds instead, but if you write 59 seconds, we error out. Maybe
> >> that seems pedantic, but I don't think users will much appreciate the
> >> discovery that 30 seconds means 60 seconds. They'll be happier to be
> >> told that up front than having to work it out afterward.
> >
> > I think this is totally wrong. The entire point of the GUC units system,
> > or at least a large part of the point, is that users should not have to
> > be intimately aware of the units in which a given value is measured
> > internally. And that in turn means that the units had better be such
> > that users won't find them overly coarse. If it matters a lot whether
> > 59 seconds gets rounded to 60, then we didn't make a good choice of units
> > for the GUC in question; and we should fix that choice, not mess with the
> > rounding rules.
> >
> > The case where this argument falls down is for "special" values, such as
> > where zero means something quite different from the smallest nonzero
> > value. Peter suggested upthread that we should redefine any GUC values
> > for which that is true, but (a) I think that loses on backwards
> > compatibility grounds, and (b) ISTM zero is probably always special to
> > some extent. A zero time delay for example is not likely to work.
> >
> > Maybe we should leave the rounding behavior alone (there's not much
> > evidence that rounding in one direction is worse than another; although
> > I'd also be okay with changing to round-to-nearest), and confine
> ourselves
> > to throwing an error for the single case that an apparently nonzero input
> > value is truncated/rounded to zero as a result of units conversion.
>
> To Andres' point:
>
> SELECT unit, count(*) FROM pg_settings WHERE unit <> '' GROUP BY unit; (9.3
> / Ubuntu)
>
> min (1 - log_rotation_age)
> s (10)
> ms (13)
>
> kb (7)
> 8kb (6)
>
> I don't know about the size implications but they seem to be non-existent.
> That any setting critically matters at +/- 1s or 1ms doesn't seem likely in
> practice. Even +/- 1min for a setting, if it did matter at extreme scale,
> would be recognizable by the user in practice as a rounding artifact and
> compensated for.
>
> At this point throwing an error for any precision that results in less than
> the default precision is my preference. I would not change the rounding
> rules for the simple reason that there is no obvious improvement to be had
> and so why introduce pointless change that - however marginal and unlikely
> -
> will be user-visible.
>
> The complaint to overcome is avoiding an interpretation of "zero" when the
> precision of the input is less than the GUC unit. Lacking any concrete
> complaints about our round-down policy I don't see where a change there is
> worthwhile.
>
> Fixing zero as a special value falls under the same category. As
> mathematically pure as using infinity may be the trade-off for practicality
> and usability seems, even in light of this complaint, like the correct one
> to have made.
>
> David J.
>
>
>
>
>
>
>
>
> --
> View this message in context:
> http://postgresql.1045698.n5.nabble.com/proposal-rounding-up-time-value-less-than-its-unit-tp5811102p5816409.html
> Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Tomonari,

* Tomonari Katsumata (t(dot)katsumata1122(at)gmail(dot)com) wrote:
> I'm thinking about a method which users get quick awareness it.
> Now, it's okay not to change current behavior except non-zero value yields
> a zero. A zero rounded down from non-zero gets an error.
>
> I attached new patch.
> This includes a document about above behavior as Heikki suggested.

Thanks for the updated patch! I was going through it and there's a few
things-

- Documentation change no longer applies

- Why aren't we worried about a specification of '7kB' being rounded down
to zero for GUCs which expect at least BLCKSZ? If the reason is
"everything that works that way will error on zero anyway today", then
I don't buy into that argument- there's no sense leaving it
inconsistent and it would be a potential land-mine to hit later.

- The hint messages look like they need some rewording, at least.

In general, I'm a fan of this change and will move forward with it,
with changes for the above, unless folks object. Based on the thread so
far, this sounds like the right solution and it'd be great to get this
simple change done to help move the CF along.

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tomonari Katsumata (t(dot)katsumata1122(at)gmail(dot)com) wrote:
>> I'm thinking about a method which users get quick awareness it.
>> Now, it's okay not to change current behavior except non-zero value yields
>> a zero. A zero rounded down from non-zero gets an error.

> In general, I'm a fan of this change and will move forward with it,
> with changes for the above, unless folks object. Based on the thread so
> far, this sounds like the right solution and it'd be great to get this
> simple change done to help move the CF along.

Hm, I object to a patch that behaves as stated above. I'm okay with
silently rounding *up* to the lowest possible nonzero value, eg rounding
up 7kB to 8kB if the variable's unit is 8kB. But if we throw an error for
7kB and not 8kB, then we are exposing the unit size in a way that the user
can't ignore. That seems to me to be antithetical to the entire design
rationale for GUC units. More, it doesn't fix the original complaint here,
which is that the user would be surprised if he specifies 7kB and gets
some special behavior instead because it's "too close to zero but not
exactly zero". 7kB should act as though it's not zero. If the difference
between 7kB and 8kB is actually user-meaningful, then we chose too coarse
a unit for the particular GUC, which is not something that a rounding rule
can paper over. But the difference between zero and not-zero is
meaningful regardless of unit choices.

This argument doesn't say anything much about which way to round for
values that are fractional but larger than the unit size. I'd probably
prefer a "round away from zero" behavior since that seems to be the most
consistent rule if we want to avoid silently creating zero values; but
I could be talked into something else.

regards, tom lane

Hey Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Tomonari Katsumata (t(dot)katsumata1122(at)gmail(dot)com) wrote:
> >> I'm thinking about a method which users get quick awareness it.
> >> Now, it's okay not to change current behavior except non-zero value yields
> >> a zero. A zero rounded down from non-zero gets an error.
>
> > In general, I'm a fan of this change and will move forward with it,
> > with changes for the above, unless folks object. Based on the thread so
> > far, this sounds like the right solution and it'd be great to get this
> > simple change done to help move the CF along.
>
> Hm, I object to a patch that behaves as stated above. I'm okay with
> silently rounding *up* to the lowest possible nonzero value, eg rounding
> up 7kB to 8kB if the variable's unit is 8kB. But if we throw an error for
> 7kB and not 8kB, then we are exposing the unit size in a way that the user
> can't ignore. That seems to me to be antithetical to the entire design
> rationale for GUC units. More, it doesn't fix the original complaint here,
> which is that the user would be surprised if he specifies 7kB and gets
> some special behavior instead because it's "too close to zero but not
> exactly zero". 7kB should act as though it's not zero. If the difference
> between 7kB and 8kB is actually user-meaningful, then we chose too coarse
> a unit for the particular GUC, which is not something that a rounding rule
> can paper over. But the difference between zero and not-zero is
> meaningful regardless of unit choices.

I agree that the difference between 7kB and 8kB shouldn't be meaningful,
but that it can be if it means we end up at zero. Having different
rounding rules at "near-zero" than in other cases doesn't strike me as
great either though, which is why I thought the error-if-rounded-to-zero
made sense. As for the user, I'd aruge that they haven't understood the
GUC if they're setting the value down to something which rounds to zero
and an error (yes, even one in the logs that we know few enough folks
read) would be better than where we are currently.

> This argument doesn't say anything much about which way to round for
> values that are fractional but larger than the unit size. I'd probably
> prefer a "round away from zero" behavior since that seems to be the most
> consistent rule if we want to avoid silently creating zero values; but
> I could be talked into something else.

PeterE argued that rounding away from zero didn't make sense either
(53F6501B(dot)3090201(at)gmx(dot)net). I feel like we're getting trapped by
examples.

Here's another proposal- how about we support a 'minimum-if-not-zero'
option for GUCs more generally, and then throw an error if the user sets
the value to a value below that minimum unless they explicitly use zero
(to indicate whatever the special meaning of zero for that GUC is)?
It'd be a larger change, certainly, but I feel like that combined with
the current behavior would address this and possibly other issues
(setting to a value which is low enough to be accepted, but too low to
allow the system to function properly..).

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> This argument doesn't say anything much about which way to round for
>> values that are fractional but larger than the unit size. I'd probably
>> prefer a "round away from zero" behavior since that seems to be the most
>> consistent rule if we want to avoid silently creating zero values; but
>> I could be talked into something else.

> PeterE argued that rounding away from zero didn't make sense either
> (53F6501B(dot)3090201(at)gmx(dot)net). I feel like we're getting trapped by
> examples.

I don't find anything compelling in Peter's argument. I do agree that
if the GUC unit is hours, and the user tries to set it to 1 second or 1
microsecond, then he probably didn't understand the GUC ... but by that
argument, if the unit is hours and he tries to set it to 1.001 hours, we
should still throw an error. The point of the GUC units system is to
not be too picky about what the variable's units are, and that that should
be possible as long as the unit is small enough that the user shouldn't
care about the difference between N and N+1 units. Therefore, I am
entirely unimpressed by examples that hinge on the assumption that the
user *does* care about that; any such example can be rejected on the
grounds that it was our error to use too large a unit for that GUC.
However, as long as we have any cases with special behavior for zero,
we should expect that the user cares about the difference between 0 units
and 1 unit.

> Here's another proposal- how about we support a 'minimum-if-not-zero'
> option for GUCs more generally, and then throw an error if the user sets
> the value to a value below that minimum unless they explicitly use zero
> (to indicate whatever the special meaning of zero for that GUC is)?

I don't think that the extra complexity in that is worth the effort.

You could maybe talk me into "round to nearest, and then complain if that
produced zero from nonzero" (in effect, your proposal with the minimum
always exactly half a unit). But that seems like mostly extra complexity
and an extra error case for not much. Again, *the user shouldn't care*
what our rounding rule is exactly; if he does, it means the particular
GUC was misdesigned.

We could adopt a straightforward "round to nearest" rule if we changed
things around enough so that zero was never special, which I think is
what Peter was really arguing for in the post you cite. But that strikes
me as a large amount of work, and a large loss of backwards compatibility,
in return for (again) not much.

regards, tom lane

Tom Lane-2 wrote
> The case where this argument falls down is for "special" values, such as
> where zero means something quite different from the smallest nonzero
> value. Peter suggested upthread that we should redefine any GUC values
> for which that is true, but (a) I think that loses on backwards
> compatibility grounds, and (b) ISTM zero is probably always special to
> some extent. A zero time delay for example is not likely to work.
>
> Maybe we should leave the rounding behavior alone (there's not much
> evidence that rounding in one direction is worse than another; although
> I'd also be okay with changing to round-to-nearest), and confine ourselves
> to throwing an error for the single case that an apparently nonzero input
> value is truncated/rounded to zero as a result of units conversion.

Tom,

Can you either change your mind back to this opinion you held last month or
commit something you find acceptable - its not like anyone would revert
something you commit... :)

Everyone agrees non-zero must not round to zero; as long as that happens I'm
not seeing anyone willing to spending any more effort on the details.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/proposal-rounding-up-time-value-less-than-its-unit-tp5811102p5820024.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

David G Johnston <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> Can you either change your mind back to this opinion you held last month or
> commit something you find acceptable - its not like anyone would revert
> something you commit... :)

Hey, am I not allowed to change my mind :-) ?

Seriously though, the main point I was making before stands: if the
details of the rounding rule matter, then we messed up on choosing the
units of the particular GUC. The question is what are we going to do
about zero being a special case.

> Everyone agrees non-zero must not round to zero; as long as that happens I'm
> not seeing anyone willing to spending any more effort on the details.

I'm not entirely sure Peter agrees; he wanted to get rid of zero being
a special case, rather than worry about making the rounding rule safe
for that case. But assuming that that's a minority position:
it seems to me that adding a new error condition is more work for us,
and more work for users too, and not an especially sane decision when
viewed from a green-field perspective. My proposal last month was in
response to some folk who were arguing for a very narrow-minded
definition of backwards compatibility ... but I don't think that's
really where we should go.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > Here's another proposal- how about we support a 'minimum-if-not-zero'
> > option for GUCs more generally, and then throw an error if the user sets
> > the value to a value below that minimum unless they explicitly use zero
> > (to indicate whatever the special meaning of zero for that GUC is)?
>
> I don't think that the extra complexity in that is worth the effort.

Alright.

> You could maybe talk me into "round to nearest, and then complain if that
> produced zero from nonzero" (in effect, your proposal with the minimum
> always exactly half a unit). But that seems like mostly extra complexity
> and an extra error case for not much. Again, *the user shouldn't care*
> what our rounding rule is exactly; if he does, it means the particular
> GUC was misdesigned.

I agree that the user shouldn't have to care, and I agree with your
earlier comment on this thread that having the rounding rules be
different when near zero vs. not-near-zero could be quite confusing.

> We could adopt a straightforward "round to nearest" rule if we changed
> things around enough so that zero was never special, which I think is
> what Peter was really arguing for in the post you cite. But that strikes
> me as a large amount of work, and a large loss of backwards compatibility,
> in return for (again) not much.

Agreed- I'm a bit concerned about backwards compatibility for all of the
proposals which change the existing rounding rules, but, if the
consensus is that N vs. N+1 shouldn't actually matter for values of
N < X < N+1 (as a one-unit step should be small enough to be not
terribly noticeable), then perhaps we can still move forward with the
ceil() approach as that looks to be the only argument against it.

To clarify- we'll simply swap from (essentially) floor() to ceil() for
handling all GUC_with_unit to internal_unit conversions, document that,
and note it in the release notes as a possible behavior change and move
on.

Thoughts? Objections?

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> To clarify- we'll simply swap from (essentially) floor() to ceil() for
> handling all GUC_with_unit to internal_unit conversions, document that,
> and note it in the release notes as a possible behavior change and move
> on.

Worksforme.

regards, tom lane

On Tuesday, September 23, 2014, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> David G Johnston <david(dot)g(dot)johnston(at)gmail(dot)com <javascript:;>> writes:
> > Can you either change your mind back to this opinion you held last month
> or
> > commit something you find acceptable - its not like anyone would revert
> > something you commit... :)
>
> Hey, am I not allowed to change my mind :-) ?
>
> Seriously though, the main point I was making before stands: if the
> details of the rounding rule matter, then we messed up on choosing the
> units of the particular GUC. The question is what are we going to do
> about zero being a special case.
>
> > Everyone agrees non-zero must not round to zero; as long as that happens
> I'm
> > not seeing anyone willing to spending any more effort on the details.
>
> I'm not entirely sure Peter agrees; he wanted to get rid of zero being
> a special case, rather than worry about making the rounding rule safe
> for that case. But assuming that that's a minority position:
> it seems to me that adding a new error condition is more work for us,
> and more work for users too, and not an especially sane decision when
> viewed from a green-field perspective. My proposal last month was in
> response to some folk who were arguing for a very narrow-minded
> definition of backwards compatibility ... but I don't think that's
> really where we should go.
>
> regards, tom lane
>

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".

My original concern was things that are rounded to zero now will not be in
9.5 if the non-error solution is chosen. The risk profile is extremely
small but it is not theoretically zero.

David J.

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > To clarify- we'll simply swap from (essentially) floor() to ceil() for
> > handling all GUC_with_unit to internal_unit conversions, document that,
> > and note it in the release notes as a possible behavior change and move
> > on.
>
> Worksforme.

Great, thanks. I'll wait a couple days to see if anyone else wants to
voice a concern.

Tomonari, don't worry about updating the patch (unless you really want
to) as I suspect I'll be changing around the wording anyway. No
offense, please, but I suspect English isn't your first language and
it'll be simpler for me if I just handle it. Thanks a lot for the bug
report and discussion and I'll be sure to credit you for it in the
commit.

Thanks again!

Stephen

David Johnston <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> My original concern was things that are rounded to zero now will not be in
> 9.5 if the non-error solution is chosen. The risk profile is extremely
> small but it is not theoretically zero.

This is exactly the position I was characterizing as an excessively
narrow-minded attachment to backwards compatibility. We are trying to
make the behavior better (as in less confusing), not guarantee that it's
exactly the same. If we are only allowed to change the behavior by
throwing errors in cases where we previously didn't, then we are
voluntarily donning a straitjacket that will pretty much ensure Postgres
doesn't improve any further.

It's important here that this behavior change is being proposed for a
new major release, not for back-patching. We have never supposed that
postgresql.conf files had to work without any change in new
major releases.

regards, tom lane

Hi Stephen,

As you said, I'm not good at English, so I'm glad you handle this thread.
I'll wait for the good changing.

Thank you very very much!

regards,
------------------
Tomonari Katsumata

2014-09-23 14:23 GMT+09:00 Stephen Frost <sfrost(at)snowman(dot)net>:

> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > To clarify- we'll simply swap from (essentially) floor() to ceil() for
> > > handling all GUC_with_unit to internal_unit conversions, document that,
> > > and note it in the release notes as a possible behavior change and move
> > > on.
> >
> > Worksforme.
>
> Great, thanks. I'll wait a couple days to see if anyone else wants to
> voice a concern.
>
> Tomonari, don't worry about updating the patch (unless you really want
> to) as I suspect I'll be changing around the wording anyway. No
> offense, please, but I suspect English isn't your first language and
> it'll be simpler for me if I just handle it. Thanks a lot for the bug
> report and discussion and I'll be sure to credit you for it in the
> commit.
>
> Thanks again!
>
> Stephen
>

On Tue, Sep 23, 2014 at 1:23 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> > To clarify- we'll simply swap from (essentially) floor() to ceil() for
>> > handling all GUC_with_unit to internal_unit conversions, document that,
>> > and note it in the release notes as a possible behavior change and move
>> > on.
>>
>> Worksforme.
>
> Great, thanks. I'll wait a couple days to see if anyone else wants to
> voice a concern.
>
> Tomonari, don't worry about updating the patch (unless you really want
> to) as I suspect I'll be changing around the wording anyway. No
> offense, please, but I suspect English isn't your first language and
> it'll be simpler for me if I just handle it. Thanks a lot for the bug
> report and discussion and I'll be sure to credit you for it in the
> commit.

Three people have voted for making it an *error* to supply a value
that needs to be rounded, instead of changing the rounding behavior.
There are not three votes for any other proposal. (This may still be
an improvement over the current behavior, though.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Three people have voted for making it an *error* to supply a value
> that needs to be rounded, instead of changing the rounding behavior.

Votes or no votes, that's a horrible idea; it breaks the design goal
that users shouldn't need to remember the precise unit size when making
postgresql.conf entries.

And I'm not sure what votes you're counting, anyway. People's opinions
have changed as the discussion proceeded ...

regards, tom lane

On Tue, Sep 23, 2014 at 10:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Three people have voted for making it an *error* to supply a value
>> that needs to be rounded, instead of changing the rounding behavior.
>
> Votes or no votes, that's a horrible idea; it breaks the design goal
> that users shouldn't need to remember the precise unit size when making
> postgresql.conf entries.

Not at all. You can still supply the value in another unit as long as
it converts exactly. If it doesn't, shouldn't you care about that?

> And I'm not sure what votes you're counting, anyway. People's opinions
> have changed as the discussion proceeded ...

David Johnston, Peter Eisentraut, myself. I don't see any indication
that any of those three people have reversed their opinion at any
point.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/23/2014 10:29 AM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Three people have voted for making it an *error* to supply a value
>> that needs to be rounded, instead of changing the rounding behavior.
> Votes or no votes, that's a horrible idea; it breaks the design goal
> that users shouldn't need to remember the precise unit size when making
> postgresql.conf entries.
>
> And I'm not sure what votes you're counting, anyway. People's opinions
> have changed as the discussion proceeded ...
>
>

I don't think I've weighed in on this, and yes, I'm very late to the
party. The "round away from zero" suggestion seemed the simplest and
most easily understood rule to me.

As Tom, I think, remarked, if that seems silly because 1 second gets
rounded up to 1 hour or whatever, then we've chosen the wrong units in
the first place.

cheers

andrew

On Tue, Sep 23, 2014 at 1:30 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> David Johnston <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> > My original concern was things that are rounded to zero now will not be
> in
> > 9.5 if the non-error solution is chosen. The risk profile is extremely
> > small but it is not theoretically zero.
>
> This is exactly the position I was characterizing as an excessively
> narrow-minded attachment to backwards compatibility. We are trying to
> make the behavior better (as in less confusing), not guarantee that it's
> exactly the same.

​I am going to assume that the feature designers were focused on wanting to
avoid:

1000 * 60 * 5

to get a 5-minute value set on a millisecond unit parameter.

The designer of the variable, in choosing a unit, has specified the minimum
value that they consider sane. Attempting to record an insane value should
throw an error.

I do not support throwing an error on all attempts to round but specifying
a value less than 1 in the variable's unit should not be allowed. If such
a value is proposed the user either made an error OR they misunderstand the
variable they are using. In either case telling them of their error is
more friendly than allowing them to discover the problem on their own.

If we are only allowed to change the behavior by
> throwing errors in cases where we previously didn't, then we are
> voluntarily donning a straitjacket that will pretty much ensure Postgres
> doesn't improve any further.

​I'm not proposing project-level policy here.

David J.

On 09/23/2014 06:23 PM, Andrew Dunstan wrote:
>
> On 09/23/2014 10:29 AM, Tom Lane wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> Three people have voted for making it an *error* to supply a value
>>> that needs to be rounded, instead of changing the rounding behavior.
>> Votes or no votes, that's a horrible idea; it breaks the design goal
>> that users shouldn't need to remember the precise unit size when making
>> postgresql.conf entries.
>>
>> And I'm not sure what votes you're counting, anyway. People's opinions
>> have changed as the discussion proceeded ...
>
> I don't think I've weighed in on this, and yes, I'm very late to the
> party. The "round away from zero" suggestion seemed the simplest and
> most easily understood rule to me.

+1, rounding up seems better to me as well. Although throwing an error
wouldn't be that bad either.

- Heikki

Fwiw I agree with TL2. The simplest, least surprising behaviour to explain
to users is to say we round to nearest and if that means we rounded to zero
(or another special value) we throw an error. We could list the minimum
value in pg_settings and maybe in documentation.

--
greg

On Tue, Sep 23, 2014 at 3:05 PM, Greg Stark <stark(at)mit(dot)edu> wrote:

> Fwiw I agree with TL2. The simplest, least surprising behaviour to explain
> to users is to say we round to nearest and if that means we rounded to zero
> (or another special value) we throw an error. We could list the minimum
> value in pg_settings and maybe in documentation.
>
​I'm not sure TL2 would agree that you are agreeing with him...

To the other point the minimum unit-less value is 1. The unit that is
applied is already listed in pg_settings​ and the documentation. While 0
is allowed it is more of a flag than a value.

David J.

On 9/23/14 10:29 AM, Tom Lane wrote:
> Votes or no votes, that's a horrible idea; it breaks the design goal
> that users shouldn't need to remember the precise unit size when making
> postgresql.conf entries.

I think this is not historically correct.

The original motivation was that you had to remember what the units on

shared_buffers = 37

were, and that it was different units than

work_mem = 37

That's independent of the question whether

shared_buffers = 250kB

might be rejected or not.

On 9/23/14 1:13 AM, Stephen Frost wrote:
> To clarify- we'll simply swap from (essentially) floor() to ceil() for
> handling all GUC_with_unit to internal_unit conversions, document that,
> and note it in the release notes as a possible behavior change and move
> on.

That'll probably work, as long as we don't invent any other-than-zero
values that are somehow treated special.

One might be able to construct a case where something gets rounded to -1
when it didn't before or the other way around, but that's clearly a
silly case.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 9/23/14 10:29 AM, Tom Lane wrote:
>> Votes or no votes, that's a horrible idea; it breaks the design goal
>> that users shouldn't need to remember the precise unit size when making
>> postgresql.conf entries.

> I think this is not historically correct.

> The original motivation was that you had to remember what the units on
> shared_buffers = 37
> were, and that it was different units than
> work_mem = 37

Right, but the issue of not requiring the specified value to be an exact
multiple of the underlying unit did come up in the early discussion,
and we were pretty clear that we didn't want to throw an error for that:

http://www.postgresql.org/message-id/flat/29818(dot)1153976618(at)sss(dot)pgh(dot)pa(dot)us#29818(dot)1153976618@sss.pgh.pa.us

regards, tom lane

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/

On Tue, Sep 23, 2014 at 10:11 PM, Gregory Smith <gregsmithpgsql(at)gmail(dot)com>
wrote:

> 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.
>
>
​+1​

I'm not convinced "minute" is wrong but it does imply a level of
user-friendliness (or over-parenting) that we can do away with.

>
> 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.
>

​Given the following why not just pick "ms" for log_rotation_age now?
​

>
> 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.
>
>
? are there any special considerations for people using pg_dump vs. those
using pg_upgrade?​

> 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.

​If we are going to go that far why not simply modify the GUC input routine
to require unit-values on these particular parameters? Direct manipulation
of pg_settings probably would need some attention but everything else could
reject integers and non-unit-specifying text. Allow the same input routine
to accept the constants "on|off|default" and convert them internally into
whatever the given GUC requires and you get the UI benefits without mucking
around with the implementation details. Modify pg_settings accordingly to
hide those now-irrelevant pieces. For UPDATE a trigger can be used to
enforce the text-only input requirement.

As long as we do not make microsecond "µs" a valid time-unit it is
impossible currently to directly specify a fraction of a millisecond.

David J.
​

On 9/23/14, 10:52 PM, David Johnston wrote:
> ​Given the following why not just pick "ms" for log_rotation_age now?

Right now there are people out there who have configurations that look
like this:

log_rotation_age=60

In order to get hourly rotation. These users will suffer some trauma
should they upgrade to a version where this parameter now means a new
log file pops every 60 seconds instead. If they didn't catch the
warning in the release notes and it happens, I'm pretty comfortable
they'll survive though, just with a bunch of cursing until it's
resolved. I'd even wager a beer that more than 10% of PostgreSQL
installs that get hit won't even notice.

I'd prefer not to make that experience into one where they get a log
file every 60ms though. That's seriously bad. I'm not opposed to
making major changes like that, I just like them to be as part of
updates big enough that people are unlikely to miss that something is
different. Just doing this log_rotation_age thing is small enough that
I expect people to miss the change in the release notes. That limits
how much I think we can change the magnitude of an easy to miss setting
with a large unit adjustment.

> ? are there any special considerations for people using pg_dump vs.
> those using pg_upgrade?​

I don't know that there's any code in pg_upgrade aiming at this sort of
job. I'd prefer not to add "find parameters that have changed in
meaning and flag them" to pg_upgrade's job requirements. It has enough
problems to worry about, and we really don't do this very often.

> If we are going to go that far why not simply modify the GUC input
> routine to require unit-values on these particular parameters? Direct
> manipulation of pg_settings probably would need some attention but
> everything else could reject integers and non-unit-specifying text.

That would be fine by me as a long-term direction, but it's more of a
two to three version release level of change. To keep that from being
terrible for users, we'd need to provide a transition release that
helped people find the problem ones without units. After that was in
the wild for a while, then could we have some confidence that enough
people had flushed the issue out enough to turn it into a hard requirement.

The project went through this exact sort of exercise with the
standard_conforming_strings GUC being the way we flagged the bad
constructs for people. That was a much harder job because it touched
everyone's application code too. But it took many years to play out.
I'd be shocked if we could herd our flock of old time users fast enough
to remove unitless GUC values from PostgreSQL in less than 3 years worth
of new releases.

--
Greg Smith greg(dot)smith(at)crunchydatasolutions(dot)com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

On 9/23/14 11:55 PM, Gregory Smith wrote:
> Right now there are people out there who have configurations that look
> like this:
>
> log_rotation_age=60
>
> In order to get hourly rotation. These users will suffer some trauma
> should they upgrade to a version where this parameter now means a new
> log file pops every 60 seconds instead.

But then this proposal is just one of several others that break backward
compatibility, and does so in a more or less silent way. Then we might
as well pick another approach that gets closer to the root of the problem.

On 9/24/14, 6:45 PM, Peter Eisentraut wrote:
> But then this proposal is just one of several others that break backward
> compatibility, and does so in a more or less silent way. Then we might
> as well pick another approach that gets closer to the root of the problem.

I was responding to some desire to get a quick fix that cut off the
worst of the problem, and the trade-offs of the other ideas bothered me
even more. Obvious breakage is annoying, but small and really subtle
version to version incompatibilities drive me really crazy. A 60X scale
change to log_rotation_age will be big enough that impacted users should
definitely notice, while not being so big it's scary. Rounding
differences are small enough to miss. And I do see whacking the sole
GUC that's set in minutes, which I was surprised to find is a thing that
existed at all, as a useful step forward.

I can't agree with Stephen's optimism that some wording cleanup is all
that's needed here. I predict it will be an annoying, multiple commit
job just to get the docs right, describing both the subtle rounding
change and how it will impact users. (Cry an extra tear for the
translators)

Meanwhile, I'd wager the entire work of my log_rotation_scale unit
change idea, from code to docs to release notes, will take less time
than just getting the docs done on any rounding change. Probably cause
less field breakage and confusion in the end too.

The bad case I threw out is a theorized one that shows why we can't go
completely crazy with jerking units around, why 1000:1 adjustments are
hard. I'm not actually afraid of that example being in the wild in any
significant quantity. The resulting regression from a 60X scale change
should not be so bad that people will suffer greatly from it either.
Probably be like the big 10:1 change in default_statistics_target.
Seemed scary that some people might be nailed by it; didn't turn out to
be such a big deal.

I don't see any agreement on the real root of a problem here yet. That
makes gauging whether any smaller change leads that way or not fuzzy. I
personally would be fine doing nothing right now, instead waiting until
that's charted out--especially if the alternative is applying any of the
rounding or error throwing ideas suggested so far. I'd say that I hate
to be that guy who tells everyone else they're wrong, but we all know I
enjoy it.

--
Greg Smith greg(dot)smith(at)crunchydatasolutions(dot)com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

Gregory Smith <gregsmithpgsql(at)gmail(dot)com> writes:
> I don't see any agreement on the real root of a problem here yet. That
> makes gauging whether any smaller change leads that way or not fuzzy. I
> personally would be fine doing nothing right now, instead waiting until
> that's charted out--especially if the alternative is applying any of the
> rounding or error throwing ideas suggested so far. I'd say that I hate
> to be that guy who tells everyone else they're wrong, but we all know I
> enjoy it.

TBH I've also been wondering whether any of these proposed cures are
better than the disease. The changes that can be argued to make the
behavior more sane are also ones that introduce backwards compatibility
issues of one magnitude or another. And I do not have a lot of sympathy
for "let's not change anything except to throw an error in a case that
seems ambiguous". That's mostly being pedantic, not helpful, especially
seeing that the number of field complaints about it is indistinguishable
from zero.

I am personally not as scared of backwards-compatibility problems as some
other commenters: I do not think that there's ever been a commitment that
postgresql.conf contents will carry forward blindly across major releases.
So I'd be willing to break strict compatibility in the name of making the
behavior less surprising. But the solutions that have been proposed that
hold to strict backwards compatibility requirements are not improvements
IMHO.

regards, tom lane

On Thu, Sep 25, 2014 at 12:11 AM, Gregory Smith <gregsmithpgsql(at)gmail(dot)com>
wrote:

> On 9/24/14, 6:45 PM, Peter Eisentraut wrote:
>
>> But then this proposal is just one of several others that break backward
>> compatibility, and does so in a more or less silent way. Then we might
>> as well pick another approach that gets closer to the root of the problem.
>>
>
> I was responding to some desire to get a quick fix that cut off the worst
> of the problem, and the trade-offs of the other ideas bothered me even
> more. Obvious breakage is annoying, but small and really subtle version to
> version incompatibilities drive me really crazy. A 60X scale change to
> log_rotation_age will be big enough that impacted users should definitely
> notice, while not being so big it's scary. Rounding differences are small
> enough to miss. And I do see whacking the sole GUC that's set in minutes,
> which I was surprised to find is a thing that existed at all, as a useful
> step forward.
>

​Why? Do you agree that a log_rotation_age value defined in seconds is
sane? If your reasoning is that everything else is defined in "s" and "ms"
then that is a developer, not a user, perspective. I'll buy into the
"everything is defined in the smallest possible unit" approach - in which
case the whole rounding things becomes a non-issue - but if you leave some
of these in seconds then we should still add an error if someone defines an
insane millisecond value for those parameters.​

>
> I can't agree with Stephen's optimism that some wording cleanup is all
> that's needed here. I predict it will be an annoying, multiple commit job
> just to get the docs right, describing both the subtle rounding change and
> how it will impact users. (Cry an extra tear for the translators)
> ​​
>
>
​Maybe I'm nieve but I'm seriously doubting this. From what I can tell the
rounding isn't currently documented and really doesn't need much if any to
be added. An error instead of rounding down to zero ​would be sufficient
and self-contained. "The value specified is less than 1 in the parameter's
base unit"

>
> Meanwhile, I'd wager the entire work of my log_rotation_scale unit change
> idea, from code to docs to release notes, will take less time than just
> getting the docs done on any rounding change. Probably cause less field
> breakage and confusion in the end too.
>

​You've already admitted there will be breakage. Your argument is that it
will be obvious enough to notice. Why not just make it impossible?
​

>
> The bad case I threw out is a theorized one that shows why we can't go
> completely crazy with jerking units around, why 1000:1 adjustments are
> hard. I'm not actually afraid of that example being in the wild in any
> significant quantity. The resulting regression from a 60X scale change
> should not be so bad that people will suffer greatly from it either.
> Probably be like the big 10:1 change in default_statistics_target. Seemed
> scary that some people might be nailed by it; didn't turn out to be such a
> big deal.
>
> I don't see any agreement on the real root of a problem here yet. That
> makes gauging whether any smaller change leads that way or not fuzzy. I
> personally would be fine doing nothing right now, instead waiting until
> that's charted out--especially if the alternative is applying any of the
> rounding or error throwing ideas suggested so far. I'd say that I hate to
> be that guy who tells everyone else they're wrong, but we all know I enjoy
> it.
>
>
Maybe not: I contend the root problem is that while we provide sane unit
specifications we've assumed that people will always be providing values
greater than 1 - but if they don't we silently use zero (a special value)
instead of telling them they messed up (made an error). If the presence of
config.d and such make this untenable then I'd say those features have a
problem.- not our current choice to define what sane is.

David J.

On Thu, Sep 25, 2014 at 12:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Gregory Smith <gregsmithpgsql(at)gmail(dot)com> writes:
> > I don't see any agreement on the real root of a problem here yet. That
> > makes gauging whether any smaller change leads that way or not fuzzy. I
> > personally would be fine doing nothing right now, instead waiting until
> > that's charted out--especially if the alternative is applying any of the
> > rounding or error throwing ideas suggested so far. I'd say that I hate
> > to be that guy who tells everyone else they're wrong, but we all know I
> > enjoy it.
>
> TBH I've also been wondering whether any of these proposed cures are
> better than the disease. The changes that can be argued to make the
> behavior more sane are also ones that introduce backwards compatibility
> issues of one magnitude or another. And I do not have a lot of sympathy
> for "let's not change anything except to throw an error in a case that
> seems ambiguous". That's mostly being pedantic, not helpful, especially
> seeing that the number of field complaints about it is indistinguishable
> from zero.
>
>
​Then what does it matter that we'd choose to error-out?​

> I am personally not as scared of backwards-compatibility problems as some
> other commenters: I do not think that there's ever been a commitment that
> postgresql.conf contents will carry forward blindly across major releases.
> So I'd be willing to break strict compatibility in the name of making the
> behavior less surprising. But the solutions that have been proposed that
> hold to strict backwards compatibility requirements are not improvements
> IMHO.
>

​Or, put differently, the pre-existing behavior is fine so don't fix what
isn't broken.

This patch simply fixes an oversight in the original implementation - that
someone might try to specify an invalid value (i.e., between 0 and 1). if
0 and -1 are flags, then the minimum allowable value is 1. The logic
should have been: range [1, something]; 0 (optionally); -1 (optionally).
Values abs(x) between 0 and 1 (exclusive) should be disallowed and, like an
attempt to specify 0.5 (without units), should throw an error.

David J.
​

David Johnston <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> On Thu, Sep 25, 2014 at 12:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> TBH I've also been wondering whether any of these proposed cures are
>> better than the disease. The changes that can be argued to make the
>> behavior more sane are also ones that introduce backwards compatibility
>> issues of one magnitude or another. And I do not have a lot of sympathy
>> for "let's not change anything except to throw an error in a case that
>> seems ambiguous". That's mostly being pedantic, not helpful, especially
>> seeing that the number of field complaints about it is indistinguishable
>> from zero.

> Then what does it matter that we'd choose to error-out?

The number of complaints about the *existing* behavior is indistinguishable
from zero (AFAIR anyway). It does not follow that deciding to throw an
error where we did not before will draw no complaints.

regards, tom lane

On Thu, Sep 25, 2014 at 1:04 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> David Johnston <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> > On Thu, Sep 25, 2014 at 12:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> TBH I've also been wondering whether any of these proposed cures are
> >> better than the disease. The changes that can be argued to make the
> >> behavior more sane are also ones that introduce backwards compatibility
> >> issues of one magnitude or another. And I do not have a lot of sympathy
> >> for "let's not change anything except to throw an error in a case that
> >> seems ambiguous". That's mostly being pedantic, not helpful, especially
> >> seeing that the number of field complaints about it is indistinguishable
> >> from zero.
>
> > ​Then what does it matter that we'd choose to error-out?​
>
> The number of complaints about the *existing* behavior is indistinguishable
> from zero (AFAIR anyway). It does not follow that deciding to throw an
> error where we did not before will draw no complaints.
>
>
Any change has the potential to draw complaints. For you it seems that
"hey, I upgraded to 9.5 and my logs are being rotated out every minute
now. I thought I had that turned off" is the desired complaint. Greg
wants: "hey, my 1 hour log rotation is now happening every minute". If the
error message is written correctly most people upon seeing the error will
simply fix their configuration and move on - regardless of whether they
were proactive in doing so having read the release notes.

​And, so what if we get some complaints? We already disallow the specified
values (instead, turning them into zeros) so it isn't like we are further
restricting user capabilities.

If I was to commit a patch that didn't throw an error I'd ask the author to
provide an outline for each of the affected parameters and what it would
mean (if possible) for a setting currently rounded to zero to instead be
rounded to 1.

Its the same language in the release notes to get someone to avoid the
error as it is to get them to not be impacted by the rounding change so the
difference is those people who would not read the release notes. The
"error" outcome is simple, direct, and fool-proof - and conveys the fact
that what they are asking for is invalid. It may be a little scary but at
least we can be sure nothing bad is happening in the system. If the
argument is that there are two few possible instances out there to expend
the effort to catalog every parameter then there isn't enough surface area
to care about throwing an error. And I still maintain that anyone setting
values for a clean installation would rather be told their input is not
valid instead of silently making it so.

​Changing the unit ​for log_rotate_age when their is basically no one
asking for that seems like the worse solution at face value; my +1 not
withstanding. I gave that mostly because if you are going to overhaul the
system then making everything "ms" seems like the right thing to do. I
think that such an effort would be a waste of resources.

I don't have clients so maybe my acceptance of errors is overly optimistic
- but in this case I truly don't see enough people even realizing that
there was a change and those few that do should be able to read the error
and make the same change that they would need to make anyway if the
rounding option is chosen.

My only concern here, and it probably applies to any solution, is that the
change is implemented properly so that all possible user input areas throw
the error correctly and as appropriate to the provided interface. That is,
interactive use immediately errors out without changing the default values
while explicit/manual file changes cause the system to error at startup. I
haven't looked into the code parts of this but I have to imagine this is
already covered since even with units and such invalid input is still
possible and needs to be addressed; this only add one more check to that
existing routine.

David J.

On 9/25/14, 1:41 AM, David Johnston wrote:
> If the error message is written correctly most people upon seeing the
> error will simply fix their configuration and move on - regardless of
> whether they were proactive in doing so having read the release notes.

The important part to realize here is that most people will never see
such an error message. There is a person/process who breaks the
postgresql.conf, a process that asks for a configuration restart/reload
(probably via pg_ctl, and then the postmaster program process creating a
server log entry that shows the error (maybe in pgstartup.log, maybe in
pg_log, maybe in syslog, maybe in the Windows Event Log)

In practice, the top of that food chain never knows what's happening at
the bottom unless something goes so seriously wrong the system is down,
and they are forced to drill down into all of these log destinations.
That's why a subset of us consider any error message based approaches to
GUC guidance a complete waste of time. I won't even address the rest of
your comments; you're focusing on trivia around something that just
fundamentally isn't useful at all.

My challenge to anyone who things error checking has value for this
issue is to demonstrate how that would play out usefully on a mainstream
Postgres system like RHEL/CentOS, Debian, or even Windows Put your
bogus setting in the config file, activate that config file in a
Postgres that looks for the round errors people dislike, and show me how
that mistake is made apparent to the user who made it. I've done
similar exercises myself, and my guess is that if the system is up at
all, those error messages went by completely unnoticed.

--
Greg Smith greg(dot)smith(at)crunchydatasolutions(dot)com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

On Thursday, September 25, 2014, Gregory Smith <gregsmithpgsql(at)gmail(dot)com>
wrote:

> On 9/25/14, 1:41 AM, David Johnston wrote:
>
>> If the error message is written correctly most people upon seeing the
>> error will simply fix their configuration and move on - regardless of
>> whether they were proactive in doing so having read the release notes.
>>
>
> The important part to realize here is that most people will never see such
> an error message. There is a person/process who breaks the
> postgresql.conf, a process that asks for a configuration restart/reload
> (probably via pg_ctl, and then the postmaster program process creating a
> server log entry that shows the error (maybe in pgstartup.log, maybe in
> pg_log, maybe in syslog, maybe in the Windows Event Log)
>
> In practice, the top of that food chain never knows what's happening at
> the bottom unless something goes so seriously wrong the system is down,
> [...]
>

And if the GUC setting here is wrong the system will be down, right?
Otherwise the attempt at changing the setting will fail and so even if the
message itself is not seen the desired behavior of the system will remain
as it was - which is just as valid a decision rounding to zero or 1.

Just like we don't take responsibility for people not reading release notes
or checking their configuration if the DBA is not aware of where the GUCs
are being set and the logging destinations that not our problem.

David J.

On Thu, Sep 25, 2014 at 12:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> TBH I've also been wondering whether any of these proposed cures are
> better than the disease.

I couldn't agree more. There's something to be said for just leaving
this alone.

> The changes that can be argued to make the
> behavior more sane are also ones that introduce backwards compatibility
> issues of one magnitude or another.

But on this point I think David Johnston said it best:

# Any change has the potential to draw complaints. For you it seems that "hey,
# I upgraded to 9.5 and my logs are being rotated out every minute now. I
# thought I had that turned off" is the desired complaint. Greg wants: "hey, my
# 1 hour log rotation is now happening every minute". If the error message is
# written correctly most people upon seeing the error will simply fix their
# configuration and move on - regardless of whether they were proactive in
# doing so having read the release notes.

I particularly agree with his first sentence - any change can
potentitally draw complaints. But I also agree with his last one - of
those three possible complaints, I certainly prefer "I had to fix my
configuration file for the new, stricter validation" over any variant
of "my configuration file still worked but it did something
surprisingly different from what it used to do.".

YMMV.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Sep 25, 2014 at 12:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > TBH I've also been wondering whether any of these proposed cures are
> > better than the disease.
>
> I couldn't agree more. There's something to be said for just leaving
> this alone.

I've been coming around to this also. I had thought earlier that there
was consensus happening, but clearly that's not the case.

> > The changes that can be argued to make the
> > behavior more sane are also ones that introduce backwards compatibility
> > issues of one magnitude or another.
>
> But on this point I think David Johnston said it best:
>
> # Any change has the potential to draw complaints. For you it seems that "hey,
> # I upgraded to 9.5 and my logs are being rotated out every minute now. I
> # thought I had that turned off" is the desired complaint. Greg wants: "hey, my
> # 1 hour log rotation is now happening every minute". If the error message is
> # written correctly most people upon seeing the error will simply fix their
> # configuration and move on - regardless of whether they were proactive in
> # doing so having read the release notes.
>
> I particularly agree with his first sentence - any change can
> potentitally draw complaints. But I also agree with his last one - of
> those three possible complaints, I certainly prefer "I had to fix my
> configuration file for the new, stricter validation" over any variant
> of "my configuration file still worked but it did something
> surprisingly different from what it used to do.".

I'll agree with this also (which is why I had suggested moving forward
with the idea that I thought had consensus- keep things the way
they are, but toss an error if we round down a non-zero value to zero).

As with Tom, I'm not against being argued to a different position, such
as rounding up instead of down, but I still don't like the "near-zero
goes to zero" situation we currently have. I'd be much happier if we'd
pick one or the other and move forward with it, or agree that we can't
reach a consensus and leave well enough alone. Not entirely sure what
the best way to get to one of the above is, but I don't feel like we're
really making much more progress at this point.

Thanks,

Stephen

On 9/25/14 11:03 AM, Robert Haas wrote:
> I couldn't agree more. There's something to be said for just leaving
> this alone.

I agree.

> potentitally draw complaints. But I also agree with his last one - of
> those three possible complaints, I certainly prefer "I had to fix my
> configuration file for the new, stricter validation" over any variant
> of "my configuration file still worked but it did something
> surprisingly different from what it used to do.".

Yes. I don't mind that we rename parameters from time to time when
semantics changed or are refined. But having the same parameter setting
mean different things in different versions is the path to complete madness.

On 9/25/14, 2:02 PM, Peter Eisentraut wrote:
> But having the same parameter setting mean different things in
> different versions is the path to complete madness.

Could we go so far as to remove support for unitless time settings
eventually? The fact that people are setting raw numbers in the
configuration file and have to know the unit to understand what they
just did has never been something I like.

* Gregory Smith (gregsmithpgsql(at)gmail(dot)com) wrote:
> On 9/25/14, 2:02 PM, Peter Eisentraut wrote:
> >But having the same parameter setting mean different things in
> >different versions is the path to complete madness.
>
> Could we go so far as to remove support for unitless time settings
> eventually? The fact that people are setting raw numbers in the
> configuration file and have to know the unit to understand what they
> just did has never been something I like.

I could certainly get behind that idea... Tho I do understand that
people will complain about backwards compatibility, etc, etc.

Thanks,

Stephen

On Thu, Sep 25, 2014 at 4:49 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Gregory Smith (gregsmithpgsql(at)gmail(dot)com) wrote:
>> On 9/25/14, 2:02 PM, Peter Eisentraut wrote:
>> >But having the same parameter setting mean different things in
>> >different versions is the path to complete madness.
>>
>> Could we go so far as to remove support for unitless time settings
>> eventually? The fact that people are setting raw numbers in the
>> configuration file and have to know the unit to understand what they
>> just did has never been something I like.
>
> I could certainly get behind that idea... Tho I do understand that
> people will complain about backwards compatibility, etc, etc.

There's also the fact that it doesn't fix the originally complained-of
problem. It does fix a problem with one of the fixes proposed for
that original problem, though.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Sep 25, 2014 at 4:49 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Gregory Smith (gregsmithpgsql(at)gmail(dot)com) wrote:
> >> On 9/25/14, 2:02 PM, Peter Eisentraut wrote:
> >> >But having the same parameter setting mean different things in
> >> >different versions is the path to complete madness.
> >>
> >> Could we go so far as to remove support for unitless time settings
> >> eventually? The fact that people are setting raw numbers in the
> >> configuration file and have to know the unit to understand what they
> >> just did has never been something I like.
> >
> > I could certainly get behind that idea... Tho I do understand that
> > people will complain about backwards compatibility, etc, etc.
>
> There's also the fact that it doesn't fix the originally complained-of
> problem. It does fix a problem with one of the fixes proposed for
> that original problem, though.

That's certainly an excellent point- this is really orthogonal to the
actual issue of setting a value smaller than a single unit for that
setting. Even if you have units attached to every GUC, an individual
could take a setting which is understaood at the '1 minute' level and
attempt to set it to '30 seconds', for example.

On the other hand, if we're making it an error to set values without
units, I'd again be behind the idea of throwing an error on the
smaller-than-one-unit case- people are going to have to go update their
configurations to deal with the errors from the lack-of-units, this
would just be another item to fix during that process. It'd certainly
be worse to change to require units and then wait anothe release to fix
or change things to address the original complaint.

Thanks,

Stephen

On Fri, Sep 26, 2014 at 10:58 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> On Thu, Sep 25, 2014 at 4:49 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > * Gregory Smith (gregsmithpgsql(at)gmail(dot)com) wrote:
>> >> On 9/25/14, 2:02 PM, Peter Eisentraut wrote:
>> >> >But having the same parameter setting mean different things in
>> >> >different versions is the path to complete madness.
>> >>
>> >> Could we go so far as to remove support for unitless time settings
>> >> eventually? The fact that people are setting raw numbers in the
>> >> configuration file and have to know the unit to understand what they
>> >> just did has never been something I like.
>> >
>> > I could certainly get behind that idea... Tho I do understand that
>> > people will complain about backwards compatibility, etc, etc.
>>
>> There's also the fact that it doesn't fix the originally complained-of
>> problem. It does fix a problem with one of the fixes proposed for
>> that original problem, though.
>
> That's certainly an excellent point- this is really orthogonal to the
> actual issue of setting a value smaller than a single unit for that
> setting. Even if you have units attached to every GUC, an individual
> could take a setting which is understaood at the '1 minute' level and
> attempt to set it to '30 seconds', for example.
>
> On the other hand, if we're making it an error to set values without
> units, I'd again be behind the idea of throwing an error on the
> smaller-than-one-unit case- people are going to have to go update their
> configurations to deal with the errors from the lack-of-units, this
> would just be another item to fix during that process. It'd certainly
> be worse to change to require units and then wait anothe release to fix
> or change things to address the original complaint.

Yep, I'll buy that.

If we want the narrowest possible fix for this, I think it's "complain
if a non-zero value would round to zero". That fixes the original
complaint and changes absolutely nothing else. But I think that's
kind of wussy. Yeah, rounding 29 seconds down to a special magic
value of 0 is more surprising than rounding 30 seconds up to a minute,
but the latter is still surprising. We're generally not averse to
tighter validation, so why here? We're not talking about preventing
the use of 60s to mean 1min, just the use of 42s to mean 1min, which
is no different than not letting 2/29/93 mean 3/1/93, a decision about
which we have no compunctions.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> If we want the narrowest possible fix for this, I think it's "complain
> if a non-zero value would round to zero". That fixes the original
> complaint and changes absolutely nothing else. But I think that's
> kind of wussy. Yeah, rounding 29 seconds down to a special magic
> value of 0 is more surprising than rounding 30 seconds up to a minute,
> but the latter is still surprising. We're generally not averse to
> tighter validation, so why here?

So in other words, if I set "shared_buffers = 100KB", you are proposing
that that be rejected because it's not an exact multiple of 8KB? This
seems like it's throwing away one of the fundamental reasons why we
invented GUC units in the first place.

I apparently have got to make this point one more time: if the user
cares about the difference between 30sec and 1min, then we erred in
designing the GUC in question; it should have had a smaller unit.
I am completely not impressed by arguments based on such cases.
The right fix for such a case is to choose a different unit for the GUC.

regards, tom lane

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > If we want the narrowest possible fix for this, I think it's "complain
> > if a non-zero value would round to zero". That fixes the original
> > complaint and changes absolutely nothing else. But I think that's
> > kind of wussy. Yeah, rounding 29 seconds down to a special magic
> > value of 0 is more surprising than rounding 30 seconds up to a minute,
> > but the latter is still surprising. We're generally not averse to
> > tighter validation, so why here?
>
> So in other words, if I set "shared_buffers = 100KB", you are proposing
> that that be rejected because it's not an exact multiple of 8KB? This
> seems like it's throwing away one of the fundamental reasons why we
> invented GUC units in the first place.

I don't believe that's what was suggested at all (it's not what I was
suggesting, in any case), but rather "shared_buffers = 1KB" would error
(erm, won't it error *anyway*? so this wouldn't be a change there..)

"shared_buffers = 100KB" would be round the same as today.

> I apparently have got to make this point one more time: if the user
> cares about the difference between 30sec and 1min, then we erred in
> designing the GUC in question; it should have had a smaller unit.
> I am completely not impressed by arguments based on such cases.
> The right fix for such a case is to choose a different unit for the GUC.

I don't think anyone is argueing that we should do away with the
rounding rules entirely, only that we should a) require units to be
specified, and b) error if the value specified is below '1 unit', but
still non-zero, as it would then be rounded to zero.

Thanks,

Stephen

On 09/26/2014 10:27 AM, Stephen Frost wrote:
> I don't think anyone is argueing that we should do away with the
> rounding rules entirely, only that we should a) require units to be
> specified, and b) error if the value specified is below '1 unit', but
> still non-zero, as it would then be rounded to zero.

That would not be a back-portable fix.

There are many 3rd-party tools out there (AWS RDS, for one) which do not
use the units.

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

* Josh Berkus (josh(at)agliodbs(dot)com) wrote:
> On 09/26/2014 10:27 AM, Stephen Frost wrote:
> > I don't think anyone is argueing that we should do away with the
> > rounding rules entirely, only that we should a) require units to be
> > specified, and b) error if the value specified is below '1 unit', but
> > still non-zero, as it would then be rounded to zero.
>
> That would not be a back-portable fix.

No, certainly not. Sorry, thought that was clear..

> There are many 3rd-party tools out there (AWS RDS, for one) which do not
> use the units.

Of course. Those would need to be updated for whichever major release
introduced this requirement.

Thanks,

Stephen

On Fri, Sep 26, 2014 at 1:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > If we want the narrowest possible fix for this, I think it's "complain
> > if a non-zero value would round to zero". That fixes the original
> > complaint and changes absolutely nothing else. But I think that's
> > kind of wussy. Yeah, rounding 29 seconds down to a special magic
> > value of 0 is more surprising than rounding 30 seconds up to a minute,
> > but the latter is still surprising. We're generally not averse to
> > tighter validation, so why here?
>
> So in other words, if I set "shared_buffers = 100KB", you are proposing
> that that be rejected because it's not an exact multiple of 8KB? This
> seems like it's throwing away one of the fundamental reasons why we
> invented GUC units in the first place.
>
>
​Both Robert and myself at one point made suggestions to this effect but I
believe at this point we are both good simply solving the <1 rounding and
calling it a day.​

> I apparently have got to make this point one more time: if the user
> cares about the difference between 30sec and 1min, then we erred in
> designing the GUC in question; it should have had a smaller unit.
> I am completely not impressed by arguments based on such cases.
> The right fix for such a case is to choose a different unit for the GUC.
>
>
You are right - both those values are probably quite stupid to set
log_rotation_age to...but we cannot error if they choose "1min"

Stephen suggested changing the unit but that particular cure seems to be
considerably worse than simply changing floor() to ceil()

I'm out of arguments for why the minimally invasive solution is, for me,
the best of the currently proposed solutions. That option gets my +1. I
have to ask someone else to actually write such a patch but it seems
straight forward enough as no one has complained that the solution would be
hard to implement - only that they dislike the idea.

David J

On Fri, Sep 26, 2014 at 1:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> If we want the narrowest possible fix for this, I think it's "complain
>> if a non-zero value would round to zero". That fixes the original
>> complaint and changes absolutely nothing else. But I think that's
>> kind of wussy. Yeah, rounding 29 seconds down to a special magic
>> value of 0 is more surprising than rounding 30 seconds up to a minute,
>> but the latter is still surprising. We're generally not averse to
>> tighter validation, so why here?
>
> So in other words, if I set "shared_buffers = 100KB", you are proposing
> that that be rejected because it's not an exact multiple of 8KB?

Absolutely. And if anyone is inconvenienced by that, then they should
upgrade to a 386. Seriously, who is going to set a value of
shared_buffers that is not measured in MB? And if they do, why
shouldn't we complain if we can't honor the value exactly? If they
really put in a value that small, it's not stupid to think that the
difference between 96kB and 104kB is significant. Honestly, the most
likely explanation for that value is that it's a developer doing
testing.

> I apparently have got to make this point one more time: if the user
> cares about the difference between 30sec and 1min, then we erred in
> designing the GUC in question; it should have had a smaller unit.
> I am completely not impressed by arguments based on such cases.
> The right fix for such a case is to choose a different unit for the GUC.

The guy who wrote the GUC system doesn't seem to agree with you, and
neither do I.

Changing the unit for log_rotation_age from minutes to seconds doesn't
fix the original complaint, which was that a non-zero value like 1s
gets rounded down to zero. Even after you change the unit to seconds,
you still have the same problem with a specification of 1ms. You
could change the log rotation unit to 1ms, but that's silly, and as
soon as we get a unit measured in microseconds or nanoseconds, which
seems like just a matter of time, you have the same problem again.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

David Johnston <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> On Fri, Sep 26, 2014 at 1:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I apparently have got to make this point one more time: if the user
>> cares about the difference between 30sec and 1min, then we erred in
>> designing the GUC in question; it should have had a smaller unit.
>> I am completely not impressed by arguments based on such cases.
>> The right fix for such a case is to choose a different unit for the GUC.

> You are right - both those values are probably quite stupid to set
> log_rotation_age to...but we cannot error if they choose "1min"

I've been thinking more about this, and I think I'm about ready to
change my position on it: why shouldn't we error out if the value
is too small? If we believe that a GUC's unit is reasonably chosen,
then it's not sensible to try to set the value to less than 1 unit.
If the user tries then there's fairly good reason to assume that
either it's a typo or he fundamentally doesn't understand the GUC.
(This does not imply that he cares about the difference between
9999 and 9999.4 units.)

Or another way to say it is that if we had set the min_val to something
positive, he'd have gotten a "value out of range" type of error.
The only reason we don't do that, typically, is that we're allowing
zero as a special case. Well, ok, let's allow zero as a special case,
but it has to be written as "0" not something else. If you try to
set a positive value then we should act as though the min_val is 1 unit.

So I'm coming around to the idea that "throw an error if a nonzero
input would round (or truncate) to zero" is a reasonable solution.
I think it'd be even more reasonable if we also fixed the rounding
rule to be "round to nearest", but the two changes can be considered
independently.

regards, tom lane

On Fri, Sep 26, 2014 at 2:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Fri, Sep 26, 2014 at 1:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> If we want the narrowest possible fix for this, I think it's "complain
> >> if a non-zero value would round to zero". That fixes the original
> >> complaint and changes absolutely nothing else. But I think that's
> >> kind of wussy. Yeah, rounding 29 seconds down to a special magic
> >> value of 0 is more surprising than rounding 30 seconds up to a minute,
> >> but the latter is still surprising. We're generally not averse to
> >> tighter validation, so why here?
> >
> > So in other words, if I set "shared_buffers = 100KB", you are proposing
> > that that be rejected because it's not an exact multiple of 8KB?
>
> Absolutely. And if anyone is inconvenienced by that, then they should
> upgrade to a 386. Seriously, who is going to set a value of
> shared_buffers that is not measured in MB? And if they do, why
> shouldn't we complain if we can't honor the value exactly? If they
> really put in a value that small, it's not stupid to think that the
> difference between 96kB and 104kB is significant. Honestly, the most
> likely explanation for that value is that it's a developer doing
> testing.
>

​Related
thought - why don't we allow the user to specify "1.5MB" as a valid value?
​ Since we don't the rounding on the 8kb stuff makes sense because not
everyone wants to choose between 1MB and 2MB. A difference of 1 minute is
not as noticeable.​

In the thread Tom linked to earlier the whole idea of a unit being "8kb"
(instead "1 block") is problematic and this is just another symptom of that.

David J.

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I've been thinking more about this, and I think I'm about ready to
> change my position on it: why shouldn't we error out if the value
> is too small? If we believe that a GUC's unit is reasonably chosen,
> then it's not sensible to try to set the value to less than 1 unit.

Right, agreed.

> If the user tries then there's fairly good reason to assume that
> either it's a typo or he fundamentally doesn't understand the GUC.
> (This does not imply that he cares about the difference between
> 9999 and 9999.4 units.)

+1

> Or another way to say it is that if we had set the min_val to something
> positive, he'd have gotten a "value out of range" type of error.
> The only reason we don't do that, typically, is that we're allowing
> zero as a special case. Well, ok, let's allow zero as a special case,
> but it has to be written as "0" not something else. If you try to
> set a positive value then we should act as though the min_val is 1 unit.

Yes.

> So I'm coming around to the idea that "throw an error if a nonzero
> input would round (or truncate) to zero" is a reasonable solution.
> I think it'd be even more reasonable if we also fixed the rounding
> rule to be "round to nearest", but the two changes can be considered
> independently.

Agreed- they're independent considerations and the original concern was
about the nonzero-to-zero issue, so I'd suggest we address that first,
though in doing so we will need to consider what *actual* min values we
should have for some cases which currently allow going to zero for the
special case and that, I believe, makes this all 9.5 material and allows
us a bit more freedom in deciding how to hanlde things more generally.

As such, I'd also +1 the "round-to-nearest" suggestion as being quite
sensible too.

THanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Agreed- they're independent considerations and the original concern was
> about the nonzero-to-zero issue, so I'd suggest we address that first,
> though in doing so we will need to consider what *actual* min values we
> should have for some cases which currently allow going to zero for the
> special case and that, I believe, makes this all 9.5 material and allows
> us a bit more freedom in deciding how to hanlde things more generally.

Yeah, I was thinking the same: we should go through the GUCs having zero
as min_val and see if any of them could be tightened up. And I agree
that *all* of this is 9.5 material --- it's not a big enough deal to
risk changing behaviors in a minor release.

regards, tom lane

On Fri, Sep 26, 2014 at 2:27 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

>
> Agreed- they're independent considerations and the original concern was
> about the nonzero-to-zero issue, so I'd suggest we address that first,
> though in doing so we will need to consider what *actual* min values we
> should have for some cases which currently allow going to zero for the
> special case and that, I believe, makes this all 9.5 material and allows
> us a bit more freedom in deciding how to hanlde things more generally.
>

​This is 9.5 material because 1) it isn't all that critical and, 2) we DO
NOT want a system to not come up because of a GUC paring error after a
minor-release update.

​I don't get where we "need" to do anything else besides that...the whole
"actual min values" comment is unclear to me.

My thought on rounding is simply no-complaint, no-change; round-to-nearest
would be my first choice if implementing anew.

David J.

David,

* David Johnston (david(dot)g(dot)johnston(at)gmail(dot)com) wrote:
> ​This is 9.5 material because 1) it isn't all that critical and, 2) we DO
> NOT want a system to not come up because of a GUC paring error after a
> minor-release update.

Agreed.

> ​I don't get where we "need" to do anything else besides that...the whole
> "actual min values" comment is unclear to me.

Well, for cases that allow going to zero as an "off" option, we've
already decided, I believe, that sub-1-unit options are off the table
and so the min value is at *least* 1, but there could be cases where '1'
doesn't actually make any sense and it should be higher than that.

Consider the log file rotation bit. If it was in seconds, would it
actually make sense to support actually doing a rotation *every second*?

No.

In that case, perhaps we'd set the minimum to '60s', even though
technically we could represent less than that, it's not sensible to do
so. The point of having minimum (and maximum..) values is that typos
and other mistakes happen and we want the user to realize they've made a
mistake.

What needs to happen next is a review of all the GUCs which allow going
to zero and which treat zero as a special value, and consider what the
*actual* minimum value for those should be (excluding zero). I was
hoping you might be interested in doing that... :D

Thanks,

Stephen

On Fri, Sep 26, 2014 at 2:39 PM, Stephen Frost [via PostgreSQL] <
ml-node+s1045698n5820714h58(at)n5(dot)nabble(dot)com> wrote:

> David,
>
> * David Johnston ([hidden email]
> <http://user/SendEmail.jtp?type=node&node=5820714&i=0>) wrote:
> > ​This is 9.5 material because 1) it isn't all that critical and, 2) we
> DO
> > NOT want a system to not come up because of a GUC paring error after a
> > minor-release update.
>
> Agreed.
>
> > ​I don't get where we "need" to do anything else besides that...the
> whole
> > "actual min values" comment is unclear to me.
>
> Well, for cases that allow going to zero as an "off" option, we've
> already decided, I believe, that sub-1-unit options are off the table
> and so the min value is at *least* 1, but there could be cases where '1'
> doesn't actually make any sense and it should be higher than that.
>
> Consider the log file rotation bit. If it was in seconds, would it
> actually make sense to support actually doing a rotation *every second*?
>
> No.
>
> In that case, perhaps we'd set the minimum to '60s', even though
> technically we could represent less than that, it's not sensible to do
> so. The point of having minimum (and maximum..) values is that typos
> and other mistakes happen and we want the user to realize they've made a
> mistake.
>
> What needs to happen next is a review of all the GUCs which allow going
> to zero and which treat zero as a special value, and consider what the
> *actual* minimum value for those should be (excluding zero). I was
> hoping you might be interested in doing that... :D
>
>
Like I said I just want to fix the bug and call it a day :)

For me just enforcing 1 as the minimum for everything would be fine.

I'd rather mandate non-integer data entry than impose an actual minimum
that is greater than 1. Specifically a too-short/too-small value might be
used during exploration and testing by a new user even if the same value
would never be useful in production. That, in fact, is the one reason that
allowing "5s" for log rotation age would make sense - to allow people to
more easily checking their log rotation policies. But making it work
without disrupting people using "=60' (1hr) is impossible without simply
outlawing unitless values.

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/proposal-rounding-up-time-value-less-than-its-unit-tp5811102p5820717.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On 09/26/2014 11:17 AM, Tom Lane wrote:
> So I'm coming around to the idea that "throw an error if a nonzero
> input would round (or truncate) to zero" is a reasonable solution.
> I think it'd be even more reasonable if we also fixed the rounding
> rule to be "round to nearest", but the two changes can be considered
> independently.

I'm good with the error. We'll want to add stuff to both the docs and
pg_settings to *show* the minimum value, and an informative error
message would help, i.e.:

"Invalid value for log_rotation_interval. Minimum value is 1min"

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

On 9/26/14, 2:17 PM, Tom Lane wrote:
> Well, ok, let's allow zero as a special case, but it has to be written
> as "0" not something else. If you try to set a positive value then we
> should act as though the min_val is 1 unit. So I'm coming around to
> the idea that "throw an error if a nonzero input would round (or
> truncate) to zero" is a reasonable solution.

I expressed some distaste for throwing errors before, but I find this
specific rule reasonable. I'll even write the patch. I owe the GUC
system a re-match after my wal_buffers auto-scaling thing for 9.1
rippled to cause extra work for you (maybe Robert too).

I just changed this submission from "Ready for Committer" to "Returned
with Feedback", with the feedback being that we'd like to see special
values rounded to 0 treated differently altogether first, before
touching any rounding. And that's a whole new submission for the next CF.

> I think it'd be even more reasonable if we also fixed the rounding
> rule to be "round to nearest", but the two changes can be considered
> independently. regards, tom lane

Personally I'm still -1 on any rounding change, instead preferring to
work toward eliminating these 0 & -1 special values altogether. Outside
of these special cases, I feel you are fundamentally right that if the
rounding really matters, the unit is simply too large. And I believe
that is the case for the log rotation one right now.

I can see my idea for rescaling the rotation age parameter is an
unpopular one, but I still like it. That cleanly splits out into a
third thing though, where it can live or die without being tied to the
rest of these issues.

--
Greg Smith greg(dot)smith(at)crunchydatasolutions(dot)com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

On 9/26/14, 2:34 PM, David Johnston wrote:
>
> ​ I don't get where we "need" to do anything else besides that...the
> whole "actual min values" comment is unclear to me.

If you look at pg_settings, there is a minimum value exposed there as
min_val. For some of these parameters, that number would normally be
1. But since we have decided that 0 is a special flag value, min_val is
0 instead.

There are others where min_val is -1 for the same reason, where
functionally the minimum is really 0. Some of us would like to see
min_val reflect the useful minimum, period, and move all these special
case ones out of there. That is a multi-year battle to engage in
though, and there's little real value to the user community coming out
of it relative to that work scope.

Gregory Smith <gregsmithpgsql(at)gmail(dot)com> writes:
> On 9/26/14, 2:34 PM, David Johnston wrote:
>> I don't get where we "need" to do anything else besides that...the
>> whole "actual min values" comment is unclear to me.

> If you look at pg_settings, there is a minimum value exposed there as
> min_val. For some of these parameters, that number would normally be
> 1. But since we have decided that 0 is a special flag value, min_val is
> 0 instead.

Right.

> There are others where min_val is -1 for the same reason, where
> functionally the minimum is really 0. Some of us would like to see
> min_val reflect the useful minimum, period, and move all these special
> case ones out of there. That is a multi-year battle to engage in
> though, and there's little real value to the user community coming out
> of it relative to that work scope.

The impression I had was that Stephen was thinking of actually setting
min_val to 1 (or whatever) and handling zero or -1 in some out-of-band
fashion, perhaps by adding GUC flag bits showing those as allowable
special cases. I'm not sure how we would display such a state of affairs
in pg_settings, but other than that it doesn't sound implausible.

We could alternatively try to split up these cases into multiple GUCs,
which I guess is what you're imagining as a "multi-year battle". But
personally I think any such proposal will fail on the grounds that
it's too much compatibility loss for the value gained.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > There are others where min_val is -1 for the same reason, where
> > functionally the minimum is really 0. Some of us would like to see
> > min_val reflect the useful minimum, period, and move all these special
> > case ones out of there. That is a multi-year battle to engage in
> > though, and there's little real value to the user community coming out
> > of it relative to that work scope.
>
> The impression I had was that Stephen was thinking of actually setting
> min_val to 1 (or whatever) and handling zero or -1 in some out-of-band
> fashion, perhaps by adding GUC flag bits showing those as allowable
> special cases. I'm not sure how we would display such a state of affairs
> in pg_settings, but other than that it doesn't sound implausible.

Yes. I'm not 100% sure about how to deal with it in pg_settings, but
that is the general idea.

> We could alternatively try to split up these cases into multiple GUCs,
> which I guess is what you're imagining as a "multi-year battle". But
> personally I think any such proposal will fail on the grounds that
> it's too much compatibility loss for the value gained.

Agreed.

Thanks,

Stephen

Tom Lane wrote:

> The impression I had was that Stephen was thinking of actually setting
> min_val to 1 (or whatever) and handling zero or -1 in some out-of-band
> fashion, perhaps by adding GUC flag bits showing those as allowable
> special cases. I'm not sure how we would display such a state of affairs
> in pg_settings, but other than that it doesn't sound implausible.

I would think that if we're going to have "out of band" values, we
should just use "off", not 0 or -1.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Friday, September 26, 2014, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Tom Lane wrote:
>
> > The impression I had was that Stephen was thinking of actually setting
> > min_val to 1 (or whatever) and handling zero or -1 in some out-of-band
> > fashion, perhaps by adding GUC flag bits showing those as allowable
> > special cases. I'm not sure how we would display such a state of affairs
> > in pg_settings, but other than that it doesn't sound implausible.
>
> I would think that if we're going to have "out of band" values, we
> should just use "off", not 0 or -1.
>
>
>
Except "off" is not always semantically correct - some GUCs (not sure which
ones ATM) use zero to mean 'default'. I think -1 always means off.
Instead of 0 and -1 we'd need 'default' and 'off' with the ability to error
if there is no meaningful default defined or if a feature cannot be turned
off.

David J.

David Johnston wrote:
> On Friday, September 26, 2014, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
> wrote:
>
> > Tom Lane wrote:
> >
> > > The impression I had was that Stephen was thinking of actually setting
> > > min_val to 1 (or whatever) and handling zero or -1 in some out-of-band
> > > fashion, perhaps by adding GUC flag bits showing those as allowable
> > > special cases. I'm not sure how we would display such a state of affairs
> > > in pg_settings, but other than that it doesn't sound implausible.
> >
> > I would think that if we're going to have "out of band" values, we
> > should just use "off", not 0 or -1.
>
> Except "off" is not always semantically correct - some GUCs (not sure which
> ones ATM) use zero to mean 'default'. I think -1 always means off.
> Instead of 0 and -1 we'd need 'default' and 'off' with the ability to error
> if there is no meaningful default defined or if a feature cannot be turned
> off.

Sure, "off" (and other spellings of boolean false value) and "default"
where they make sense, and whatever other values that make sense. My
point is to avoid collapsing such logical values to integer/floating
point values just because the option uses a numeric scale.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 9/26/14, 2:50 PM, David G Johnston wrote:
>
> Like I said I just want to fix the bug and call it a day :)

I'm afraid you've come to the wrong project and mailing list for *that*.

On Friday, September 26, 2014, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> David Johnston wrote:
> > On Friday, September 26, 2014, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com
> <javascript:;>>
> > wrote:
> >
> > > Tom Lane wrote:
> > >
> > > > The impression I had was that Stephen was thinking of actually
> setting
> > > > min_val to 1 (or whatever) and handling zero or -1 in some
> out-of-band
> > > > fashion, perhaps by adding GUC flag bits showing those as allowable
> > > > special cases. I'm not sure how we would display such a state of
> affairs
> > > > in pg_settings, but other than that it doesn't sound implausible.
> > >
> > > I would think that if we're going to have "out of band" values, we
> > > should just use "off", not 0 or -1.
> >
> > Except "off" is not always semantically correct - some GUCs (not sure
> which
> > ones ATM) use zero to mean 'default'. I think -1 always means off.
> > Instead of 0 and -1 we'd need 'default' and 'off' with the ability to
> error
> > if there is no meaningful default defined or if a feature cannot be
> turned
> > off.
>
> Sure, "off" (and other spellings of boolean false value) and "default"
> where they make sense, and whatever other values that make sense. My
> point is to avoid collapsing such logical values to integer/floating
> point values just because the option uses a numeric scale.
>
>
My comment was more about storage than data entry. I'm not sure, though,
that we'd want to allow 0 as valid input even if it is acceptable for
Boolean.

David J.

On 9/26/14, 3:22 PM, Tom Lane wrote:
> We could alternatively try to split up these cases into multiple GUCs,
> which I guess is what you're imagining as a "multi-year battle".

No, I was just pointing out that even the cleanest major refactoring
possible here is going to result in broken config files complaints for
years. And no matter how well that goes, a second rewrite in the next
major version, addressing whatever things nobody saw coming in the first
redesign, is a very real possibility. The minute the GUC box is opened
that far, it will not close up again in a release, or likely even two,
and the griping from the field is going to take 3+ years to settle.

I have no desire to waste time on partial measures either though. I
think I've been clear that's my theme on this--if we're gonna mess with
this significantly, let's do it right and in a big way. If there's a
really trivial fix to apply, that's fine too. Throw an error when the
value is between the special one and the useful minimum, no other
changes; that I could see slipping into a single release without much
pain. Might even be possible to write as a useful sub-step toward a
bigger plan too. Wouldn't bother breaking that out as a goal unless it
just happened to fall out of the larger design though.

The rest of the rounding and error handling ideas wandering around seem
in this uncomfortable middle ground to me. They are neither large
enough to feel like a major improvement happened, nor trivial enough to
apply with minimal work/risk. And this is not a bug that must be fixed
ASAP--it's an unfriendly UI surprise.

--
Greg Smith greg(dot)smith(at)crunchydatasolutions(dot)com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/