Re: [PATCH v2] Add overflow checks to money type input function - Mailing list pgsql-hackers

Hello Peter,

My 0.02€ (not $0.02:-) comments on this patch:

Patch applies and "make check" is ok. I see no issue with the code.

A few comments below.

The regression tests are clearer & commented, it is an improvement.

While you are at it, maybe you could consider adding tests for more 
features, eg ',' skipping and '(' as negative sign:
 SELECT '(1)'::MONEY; SELECT '($123,456.78)'::MONEY;

The code does what it advertises. Note that this patch only tests 
overflows when parsing a string. It does not detect overflows during 
operations.


> The money type input function did not have any overflow checks at all.
> There were some regression tests that purported to check for overflow,
> but they actually checked for the overflow behavior of the int8 type
> before casting to money.  Remove those unnecessary checks and add some
> that actually check the money input function.

I think that the lack of generality of the MONEY type makes it near 
unusable (I do not think that it is the place of the database to 
prettyprint the currency, especially with a '$' sign which happen not to 
be the currency of 95% of the world population, the precision is hardwired 
to 2 figures after the unit, the convention to use '(' for negative 
numbers is rather an anglo-saxon accounting one, ...), so I would not have 
bothered. This type should really be named "DOLLAR" or "USD".

> +    /*
> +     * We accumulate the absolute amount in "value" and then apply the sign at
> +     * the end.  (The sign can appear before or after the digits, so it would
> +     * be more complicated to do otherwise.)  Because of the larger range of
> +     * negative signed integers, we build "value" in the negative and then
> +     * flip the sign at the end,

Argh. A trick!

> catching most-negative-number overflow if
> +     * necessary.
> +     */
> +
>     for (; *s; s++)
>     {
>         /* we look for digits as long as we have found less */
>         /* than the required number of decimal places */
>         if (isdigit((unsigned char) *s) && (!seen_dot || dec < fpoint))
>         {
> -            value = (value * 10) + (*s - '0');
> +            Cash newvalue = (value * 10) - (*s - '0');
> +
> +            if (newvalue / 10 != value)

I would have done "if (newvalue > 0)" because / used to be expensive and 
the overflow materializes as a sign inversion, but I understand Tom 
commented against that, so this is fine.

>     /* round off if there's another digit */
>     if (isdigit((unsigned char) *s) && *s >= '5')
> -        value++;
> +        value--;

Positive/negative trick again. A reminder comment?

> +    if (value > 0)

Trick again...

Ok, this test seems to be necessary just for a min int value that would 
have been badly rounded down by the preceding increment.


> +        ereport(ERROR,
> +                (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> +                 errmsg("value \"%s\" is out of range for type money",
> +                        str)));
>
>     /* adjust for less than required decimal places */
>     for (; dec < fpoint; dec++)
> -        value *= 10;
> +    {
> +        Cash newvalue = value * 10;
> +
> +        if (newvalue / 10 != value)
> +            ereport(ERROR,
> +                    (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> +                     errmsg("value \"%s\" is out of range for type money",
> +                            str)));
> +
> +        value = newvalue;
> +    }
>
>     /*
>      * should only be trailing digits followed by whitespace, right paren,
> @@ -247,7 +280,17 @@ cash_in(PG_FUNCTION_ARGS)
>                             str)));
>     }
>
> -    result = value * sgn;
> +    if (sgn > 0)
> +    {
> +        result = -value;

The code looks a little bit strange because of the above negative value 
trick. Maybe there could be a reminder comment?

-- 
Fabien.