On 5 March 2013 13:46, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2013/3/5 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>:
>> Hello,
>>
>>> > I think that the only other behavioural glitch I spotted was that it
>>> > fails to catch one overflow case, which should generate an "out of
>>> > ranger" error:
>>> >
>>> > select format('|%*s|', -2147483648, 'foo');
>>> > Result: |foo|
>>> >
>>> > because -(-2147483648) = 0 in signed 32-bit integers.
>>
>> Ouch. Thanks for pointing out.
>>
>>> fixed - next other overflow check added
>>
>> Your change shown below seems assuming that the two's complement
>> of the most negative number in integer types is identical to
>> itself, and looks working as expected at least on
>> linux/x86_64. But C standard defines it as undefined, (As far as
>> I hear :-).
>>
>> | if (width != 0)
>> | {
>> | int32 _width = -width;
>> |
>> | if (SAMESIGN(width, _width))
>> | ereport(ERROR,
>>
>
> this pattern was used elsewhere in pg
>
>> Instead, I think we can deny it by simply comparing with
>> INT_MIN. I modified the patch like so and put some modifications
>> on styling.
>
> ook - I have not enough expirience with this topic and I cannot say
> what is preferred.
>
>>
>> Finally, enlargeStringInfo fails just after for large numbers. We
>> might should keep it under certain length to get rid of memory
>> exhaustion.
>
> I though about it, but I don't know a correct value - probably any
> width specification higher 1MB will be bogus and can be blocked ?? Our
> VARLENA max size is 1GB so with should not be higher than 1GB ever.
>
> what do you thinking about these limits?
>
I think it's fine as it is.
It's no different from repeat() for example. We allow repeat('a',
1000000000) so allowing format('%1000000000s', '') seems reasonable,
although probably not very useful.
Upping either beyond 1GB generates an out of memory error, which also
seems reasonable -- I can't imagine why you would want such a long
string.
Regards,
Dean
