Thread: 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.tomonari@po.ntts.co.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

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

> hlinnakangas@

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

Tomonari Katsumata <t.katsumata1122@gmail.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 <

> t.katsumata1122@

> > 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.g.johnston@gmail.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@sss.pgh.pa.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 <

> david.g.johnston@

> > 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@mit.edu> wrote:
> On Sat, Aug 23, 2014 at 9:38 PM, Tom Lane <tgl@sss.pgh.pa.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@mit.edu> wrote:
> > On Sat, Aug 23, 2014 at 9:38 PM, Tom Lane <tgl@sss.pgh.pa.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@gmail.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 <

> robertmhaas@

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

Tomonari,

* Tomonari Katsumata (t.katsumata1122@gmail.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@snowman.net> writes:
> * Tomonari Katsumata (t.katsumata1122@gmail.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@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tomonari Katsumata (t.katsumata1122@gmail.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.3090201@gmx.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@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.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.3090201@gmx.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.g.johnston@gmail.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@sss.pgh.pa.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@snowman.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

David G Johnston <david.g.johnston@gmail.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@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.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.g.johnston@gmail.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

On Tue, Sep 23, 2014 at 1:23 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Stephen Frost <sfrost@snowman.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@gmail.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@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.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@gmail.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 09/23/2014 06:23 PM, Andrew Dunstan wrote:
>
> On 09/23/2014 10:29 AM, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.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

<p dir="ltr">Fwiw I agree with TL2. The simplest, least surprising behaviour to explain to users is to say we round to
nearestand if that means we rounded to zero (or another special value) we throw an error. We could list the minimum
valuein pg_settings and maybe in documentation.<p dir="ltr">-- <br /> greg 

<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><span
style="font-family:arial">OnTue, Sep 23, 2014 at 3:05 PM, Greg Stark </span><span dir="ltr"
style="font-family:arial"><<ahref="mailto:stark@mit.edu" target="_blank">stark@mit.edu</a>></span><span
style="font-family:arial">wrote:</span><br /></div><div class="gmail_extra"><div class="gmail_quote"><blockquote
class="gmail_quote"style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><p
dir="ltr">FwiwI agree with TL2. The simplest, least surprising behaviour to explain to users is to say we round to
nearestand if that means we rounded to zero (or another special value) we throw an error. We could list the minimum
valuein pg_settings and maybe in documentation.</blockquote><div class="gmail_default"
style="font-family:arial,helvetica,sans-serif">​I'mnot sure TL2 would agree that you are agreeing with him...</div><div
class="gmail_default"style="font-family:arial,helvetica,sans-serif"><br /></div><div class="gmail_default"
style="font-family:arial,helvetica,sans-serif">Tothe other point the minimum unit-less value is 1.  The unit that is
appliedis already listed in pg_settings​ and the documentation.  While 0 is allowed it is more of a flag than a
value.</div><divclass="gmail_default" style="font-family:arial,helvetica,sans-serif"><br /></div><div
class="gmail_default"style="font-family:arial,helvetica,sans-serif">David J.</div><div class="gmail_default"
style="font-family:arial,helvetica,sans-serif"><br/></div></div></div></div> 

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@gmx.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.1153976618@sss.pgh.pa.us#29818.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.smith@crunchydatasolutions.com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

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.smith@crunchydatasolutions.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.smith@crunchydatasolutions.com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

Gregory Smith <gregsmithpgsql@gmail.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

David Johnston <david.g.johnston@gmail.com> writes:
> On Thu, Sep 25, 2014 at 12:46 AM, Tom Lane <tgl@sss.pgh.pa.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 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.smith@crunchydatasolutions.com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

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, [...]

On Thu, Sep 25, 2014 at 12:46 AM, Tom Lane <tgl@sss.pgh.pa.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@gmail.com) wrote:
> On Thu, Sep 25, 2014 at 12:46 AM, Tom Lane <tgl@sss.pgh.pa.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@gmail.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@snowman.net> wrote:
> * Gregory Smith (gregsmithpgsql@gmail.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@gmail.com) wrote:
> On Thu, Sep 25, 2014 at 4:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Gregory Smith (gregsmithpgsql@gmail.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@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> On Thu, Sep 25, 2014 at 4:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> > * Gregory Smith (gregsmithpgsql@gmail.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@gmail.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@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.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@agliodbs.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@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.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.g.johnston@gmail.com> writes:
> On Fri, Sep 26, 2014 at 1:22 PM, Tom Lane <tgl@sss.pgh.pa.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

* Tom Lane (tgl@sss.pgh.pa.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@snowman.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

David,

* David Johnston (david.g.johnston@gmail.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 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.smith@crunchydatasolutions.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@gmail.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@sss.pgh.pa.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

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.

David Johnston wrote:
> On Friday, September 26, 2014, Alvaro Herrera <alvherre@2ndquadrant.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*.

David Johnston wrote:
> On Friday, September 26, 2014, Alvaro Herrera <alvherre@2ndquadrant.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.

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.smith@crunchydatasolutions.com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/