Re: Add generate_series(numeric, numeric)

On Fri, Nov 7, 2014 at 3:19 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Oct 14, 2014 at 3:22 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>
>>
>> On Tue, Oct 7, 2014 at 8:38 AM, Ali Akbar <the(dot)apaan(at)gmail(dot)com> wrote:
>>>
>>> 2014-10-06 22:51 GMT+07:00 Marti Raudsepp <marti(at)juffo(dot)org>:
>>>
>>>
>>>>
>>>> > the one that tests values just before numeric overflow
>>>>
>>>> Actually I don't know if that's too useful. I think you should add a
>>>> test case that causes an error to be thrown.
>>>
>>>
>>> Actually i added the test case because in the code, when adding step into
>>> current for the last value, i expected it to overflow:
>>>
>>> /* increment current in preparation for next iteration */
>>> add_var(&fctx->current, &fctx->step, &fctx->current);
>>>
>>> where in the last calculation, current is 9 * 10^131071. Plus 10^131071,
>>> it will be 10^131072, which i expected to overflow numeric type (in the doc,
>>> numeric's range is "up to 131072 digits before the decimal point").
>>>
>>> In attached patch, i narrowed the test case to produce smaller result.
>>
>>
>> Well, as things stand now, the logic relies on cmp_var and the sign of the
>> stop and current values. it is right that it would be better to check for
>> overflow before going through the next iteration, and the cleanest and
>> cheapest way to do so would be to move the call to make_result after add_var
>> and save the result variable in the function context, or something similar,
>> but honestly I am not sure it is worth the complication as it would mean
>> some refactoring on what make_result actually already does.
>>
>> I looked at this patch again a bit and finished with the attached, adding an
>> example in the docs, refining the error messages and improving a bit the
>> regression tests. I have nothing more to comment, so I am marking this patch
>> as "ready for committer".
>
> The patch looks good to me. Barring any objection I will commit the patch.
> Memo for me: CATALOG_VERSION_NO must be changed at the commit.

Pushed.

Regards,

--
Fujii Masao