Re: Add generate_series(numeric, numeric)

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.

Regards,

--
Fujii Masao