Re: Add generate_series(numeric, numeric)

Hi,

I am newbie in postgresql development, so i took easy item in Todo list "
Add generate_series(numeric, numeric)". First, i changed function with
analogue funcionality (generate_series_timestamp) and added new object in
pg_proc (object id is 6000). My changes successfully was compiled.

I have found some problems when code was tested. (remark step=1.0)

1) STATEMENT: SELECT generate_series(1.0,6.1);
1: generate_series (typeid = 1700, len = -1, typmod = -1, byval = f)
----
1: generate_series = "1.0" (typeid = 1700, len = -1, typmod = -1, byval =
f)
----
1: generate_series = "2.0" (typeid = 1700, len = -1, typmod = -1, byval =
f)
----
1: generate_series = "3.0" (typeid = 1700, len = -1, typmod = -1, byval =
f)
----
1: generate_series = "4.0" (typeid = 1700, len = -1, typmod = -1, byval =
f)
----
1: generate_series = "5.0" (typeid = 1700, len = -1, typmod = -1, byval =
f)
----
1: generate_series = "6.0" (typeid = 1700, len = -1, typmod = -1, byval =
f)
----

Function work.

2) STATEMENT: SELECT * FROM generate_series(1.0, 6.1)
1: generate_series (typeid = 1700, len = -1, typmod = -1, byval = f)
----
make_result(): NUMERIC w=0 d=0 POS 0001
CURRENT:: NUMERIC w=0 d=1 POS 0001
FINISH:: NUMERIC w=0 d=1 POS 0006 1000
STEP:: NUMERIC w=0 d=0 POS 0001
make_result(): NUMERIC w=0 d=1 POS 0002
CURRENT:: NUMERIC w=32639 d=16255 NEG 0000 .... (more 0000)

And postgres was crashed.

Could you help to find mistakes?

Some questions:
1) Is correct using Numeric in generate_series_numeric_fctx instead of
NumericVar?
2) How do you determine object id for new function? Maybe you're looking
for last object id in catalog directory (src/include/catalog/pg_*.h) and
increase by one last object id.

P.S. Sorry, I have made mistakes in message, because english isn't native
language.

Hi,

Nice patch! And welcome here.

On Mon, Sep 29, 2014 at 12:42 PM, Платон Малюгин <malugin(dot)p(at)gmail(dot)com>
wrote:

> Could you help to find mistakes?
>
This implementation is rather broken, particularly when thinking that this
code could be used with a negative step... I also see no point in saving
explicitly the step sign in the function context.

> Some questions:
> 1) Is correct using Numeric in generate_series_numeric_fctx instead of
> NumericVar?
>
I'd rather go with NumericVar to facilitate the arithmetic operations at
each step between the current and to avoid recomputing at the finish and
current values all the time, saving a bit of process for each loop. It also
simplifies the calculation of each value. This way you could as well use
cmp_var with const_zero to be sure that a given NumericVar is positive or
negative, simplifying process.

2) How do you determine object id for new function? Maybe you're looking
> for last object id in catalog directory (src/include/catalog/pg_*.h) and
> increase by one last object id.
>
You can use the script unused_oids in src/include/catalog/ to find unused
oids. For example after applying with your patch:
$ cd src/include/catalog && ./unused_oids
2 - 9
32
86 - 88
90
3154
3156
3259 - 3453
3573 - 3591
3787 - 3801
3952
3954
3994 - 3999
4051 - 5999
6001 - 9999

Btw, while looking at your patch, I actually hacked it a bit and finished
with the attached:
- changed process to use NumericVar instead of Numeric
- addition of custom step values with a function
generate_series(numeric,numeric,numeric)
- some cleanup and some comments here and there
That's still WIP, but feel free to use it for future work. If you are able
to add documentation and regression tests to this patch, I would recommend
that you register it to the next commit fest, where it would get more
review, and hopefully it will get committed. The next commit fest begins on
the 15th of October:
https://commitfest.postgresql.org/action/commitfest_view?id=24

Regards,
--
Michael

On Mon, Sep 29, 2014 at 4:19 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

>
> Michael
> Btw, while looking at your patch, I actually hacked it a bit and finished
> with the attached:
> - changed process to use NumericVar instead of Numeric
> - addition of custom step values with a function
> generate_series(numeric,numeric,numeric)
> - some cleanup and some comments here and there
> That's still WIP, but feel free to use it for future work. If you are able
> to add documentation and regression tests to this patch, I would recommend
> that you register it to the next commit fest, where it would get more
> review, and hopefully it will get committed.
>
Oops, it seems that I have been too hasty here. With a fresh mind I looked
at my own patch again and found two bugs:
- Incorrect initialization of step variable with const_one
- Incorrect calculation of each step's value, making stuff crash, it is
necessary to switch to the context of the function to perform operations on
a temporary variable first.
Platon (am I writing your name correctly??), feel free to pick up the
attached version, it works for me:
=# SELECT * FROM generate_series(0.8, -4, -0.7);
generate_series
-----------------
0.8
0.1
-0.6
-1.3
-2.0
-2.7
-3.4
(7 rows)
=# SELECT * FROM generate_series(0.8, 5, 1.2);
generate_series
-----------------
0.8
2.0
3.2
4.4
(4 rows)
=# SELECT * FROM generate_series(0.8, 5);
generate_series
-----------------
0.8
1.8
2.8
3.8
4.8
(5 rows)
=# SELECT * FROM generate_series(0.8, 5, 0);
ERROR: 22023: step size cannot equal zero
LOCATION: generate_series_numeric, numeric.c:1271

This could be polished more, but I'm sure you'll deal with that well. If
you are able to have a more accompished version for the next commit fest
I'll have a look at it.
Regards,
--
Michael

On Tue, Sep 30, 2014 at 10:00 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:

>
>
> On Mon, Sep 29, 2014 at 4:19 PM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com> wrote:
>
>>
>> Michael
>> Btw, while looking at your patch, I actually hacked it a bit and finished
>> with the attached:
>> - changed process to use NumericVar instead of Numeric
>> - addition of custom step values with a function
>> generate_series(numeric,numeric,numeric)
>> - some cleanup and some comments here and there
>> That's still WIP, but feel free to use it for future work. If you are
>> able to add documentation and regression tests to this patch, I would
>> recommend that you register it to the next commit fest, where it would get
>> more review, and hopefully it will get committed.
>>
> Oops, it seems that I have been too hasty here. With a fresh mind I looked
> at my own patch again and found two bugs:
> - Incorrect initialization of step variable with const_one
> - Incorrect calculation of each step's value, making stuff crash, it is
> necessary to switch to the context of the function to perform operations on
> a temporary variable first.
>
And here is the patch...
--
Michael

Hi
Oops, it seems that I have been too hasty here. With a fresh mind I looked
at my own patch again and found two bugs:

>
>> - Incorrect calculation of each step's value, making stuff crash, it is
>> necessary to switch to the context of the function to perform operations on
>> a temporary variable first
>>
>
- i think you can use the fctx->current variable without temporary variable
(there's comment in the add_var function: Full version of add functionality
on variable level (handling signs). result might point to one of the
operands too without danger.). But you _must_ switch the context first
because add_var will allocate new array for the data and freeing the old
one.

- numeric can be NaN. We must reject it as first, finish and last parameter.
- numeric datatype is large, but there are limitations. According to doc,
the limit is: up to 131072 digits before the decimal point; up to 16383
digits after the decimal point. How can we check if the next step
overflows? As a comparison, in int.c, generate_series_step_int4 checks if
its overflows, and stop the next call by setting step to 0. Should we do
that?

~ will try to fix the patch later

--
Ali Akbar

2014-10-05 15:21 GMT+07:00 Ali Akbar <the(dot)apaan(at)gmail(dot)com>:

> Hi
> Oops, it seems that I have been too hasty here. With a fresh mind I looked
> at my own patch again and found two bugs:
>
>
>>> - Incorrect calculation of each step's value, making stuff crash, it is
>>> necessary to switch to the context of the function to perform operations on
>>> a temporary variable first
>>>
>>
> - i think you can use the fctx->current variable without temporary
> variable (there's comment in the add_var function: Full version of add
> functionality on variable level (handling signs). result might point to one
> of the operands too without danger.). But you _must_ switch the context
> first because add_var will allocate new array for the data and freeing the
> old one.
>
> - numeric can be NaN. We must reject it as first, finish and last
> parameter.
> - numeric datatype is large, but there are limitations. According to doc,
> the limit is: up to 131072 digits before the decimal point; up to 16383
> digits after the decimal point. How can we check if the next step
> overflows? As a comparison, in int.c, generate_series_step_int4 checks if
> its overflows, and stop the next call by setting step to 0. Should we do
> that?
>
> ~ will try to fix the patch later
>
attached the patch. Not checking if it overflows, but testing it with 9 *
10 ^ 131072 works (not resulting in an error). Other modifications:
- doc update
- regresssion tests
- while testing regression test, opr_sanity checks that the
generate_series_numeric function is used twice (once for 2 parameter and
once for the 3 parameter function), so i changed the name to
generate_series_step_numeric and created new function
generate_series_numeric that calls generate_series_step_numeric

--
Ali Akbar

Also, Платон Малюгин, can you add this patch to postgresql commitfest (
http://commitfest.postgresql.org)?

--
Ali Akbar

On Sun, Oct 5, 2014 at 7:39 PM, Ali Akbar <the(dot)apaan(at)gmail(dot)com> wrote:

>
> 2014-10-05 15:21 GMT+07:00 Ali Akbar <the(dot)apaan(at)gmail(dot)com>:
>
>> - i think you can use the fctx->current variable without temporary
>> variable (there's comment in the add_var function: Full version of add
>> functionality on variable level (handling signs). result might point to one
>> of the operands too without danger.). But you _must_ switch the context
>> first because add_var will allocate new array for the data and freeing the
>> old one.
>>
> Yep.

> - numeric can be NaN. We must reject it as first, finish and last
>> parameter.
>>
> Makes sense.

> - numeric datatype is large, but there are limitations. According to doc,
>> the limit is: up to 131072 digits before the decimal point; up to 16383
>> digits after the decimal point. How can we check if the next step
>> overflows? As a comparison, in int.c, generate_series_step_int4 checks if
>> its overflows, and stop the next call by setting step to 0. Should we do
>> that?
>>
> Yes we should.

> - while testing regression test, opr_sanity checks that the
> generate_series_numeric function is used twice (once for 2 parameter and
> once for the 3 parameter function), so i changed the name to
> generate_series_step_numeric and created new function
> generate_series_numeric that calls generate_series_step_numeric.
>
Yep.

It seems to me that this patch is heading in a good direction (haven't
tested or tried to break it, just looked at the code). However please be
careful of code format, particularly brackets for "if" blocks. For example
this thing:
if (foo) {
blah;
}
Should be that:
if (foo)
blah;
Then in the case of multiple lines, this thing:
if (foo) {
blah;
blah2;
}
Should be that:
if (foo)
{
blah;
blah2;
}
Code convention is detailed in the docs:
http://www.postgresql.org/docs/devel/static/source.html
Regards,
--
Michael

Thanks for the review. Attached the formatted patch according to your
suggestion.

- numeric datatype is large, but there are limitations. According to doc,
>> the limit is: up to 131072 digits before the decimal point; up to 16383
>> digits after the decimal point. How can we check if the next step
>> overflows? As a comparison, in int.c, generate_series_step_int4 checks if
>> its overflows, and stop the next call by setting step to 0. Should we do
>> that?
>>
> Yes we should.
>

how can we check the overflow after add_var?
(in int.c, the code checks for integer calculation overflow, that wraps the
result to negative value)

in numeric.sql regression test, i've added this query:
select (i / (10::numeric ^ 131071))::numeric(1,0)
from generate_series(-9*(10::numeric ^ 131071),
9*(10::numeric ^ 131071),
(10::numeric ^ 131071))
as a(i);

Because the doc notes that the maximum numeric digit before decimal point
is 131072, i hope this query covers the overflow case (in the last value it
will generate, if we add 9 x 10^13071 with 10^13071, add_var will
overflows). But in my tests, that isn't the case. The code works without
any error and returns the correct rows:

numeric
---------
-9
-8
-7
-6
-5
-4
-3
-2
-1
0
1
2
3
4
5
6
7
8
9
(19 rows)

Regards,
--
Ali Akbar

I'm a bit confused about who I should be replying to, but since you
were the last one with a patch...

On Mon, Oct 6, 2014 at 11:44 AM, Ali Akbar <the(dot)apaan(at)gmail(dot)com> wrote:
> Thanks for the review. Attached the formatted patch according to your
> suggestion.

+ select * from generate_series(0.1::numeric, 10.0::numeric, 0.1::numeric);
+ generate_series
+ -----------------
+ 0.1
...
+ 10.0
+ (100 rows)

Unless there is a good reason, can you please keep individual test
output fewer than 100 lines? I think the 41-line result is an overkill
as well. This just bloats the repository size unnecessarily.

Also, I see that this patch was added to the 2014-10 commitfest and
then deleted again (by user apaan). Why?

Regards,
Marti

> + select * from generate_series(0.1::numeric, 10.0::numeric, 0.1::numeric);
> + generate_series
> + -----------------
> + 0.1
> ...
> + 10.0
> + (100 rows)
>
> Unless there is a good reason, can you please keep individual test
> output fewer than 100 lines? I think the 41-line result is an overkill
> as well. This just bloats the repository size unnecessarily.
>

Okay. In this revision i cut the tests' result except the last one (the one
that tests values just before numeric overflow).

> Also, I see that this patch was added to the 2014-10 commitfest and
> then deleted again (by user apaan). Why?
>

User apaan is me. When i added to the commitfest, the patch is listed there
by me (apaan). I only added some bits from original patch by Платон Малюгин
that was revised further by Michael Paquier. So i think they should have
the credits for this patch, not me.
In this situation, what should i do?

Thanks for the review Manti!

Regards,
--
Ali Akbar

On Mon, Oct 6, 2014 at 6:12 PM, Ali Akbar <the(dot)apaan(at)gmail(dot)com> wrote:
> User apaan is me. When i added to the commitfest, the patch is listed there
> by me (apaan).

That's fine I think, it's just for tracking who made the changes in
the CommitFest app. What actually matters is what you write in the
"Author" field, which could contain all 3 names separated by commas.

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

Also, I noticed that there are a few trailing spaces in the patch that
should be removed:

+generate_series_numeric(PG_FUNCTION_ARGS)
...
+ if (NUMERIC_IS_NAN(start_num) || NUMERIC_IS_NAN(finish_num))
...
+ if (PG_NARGS() == 3)
...
+ if (NUMERIC_IS_NAN(step_num))

Regards,
Marti

On Tue, Oct 7, 2014 at 12:51 AM, Marti Raudsepp <marti(at)juffo(dot)org> wrote:

> On Mon, Oct 6, 2014 at 6:12 PM, Ali Akbar <the(dot)apaan(at)gmail(dot)com> wrote:
> > User apaan is me. When i added to the commitfest, the patch is listed
> there
> > by me (apaan).
> That's fine I think, it's just for tracking who made the changes in
> the CommitFest app. What actually matters is what you write in the
> "Author" field, which could contain all 3 names separated by commas.
>
It's fine not to add mine to the list of authors, I did a hack review. Feel
free to add it to the list of reviewers though.
--
Michael

2014-10-06 22:51 GMT+07:00 Marti Raudsepp <marti(at)juffo(dot)org>:

> That's fine I think, it's just for tracking who made the changes in
> the CommitFest app. What actually matters is what you write in the
> "Author" field, which could contain all 3 names separated by commas.
>

Ok. Added to commitfest:
https://commitfest.postgresql.org/action/patch_view?id=1591

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

Also, I noticed that there are a few trailing spaces in the patch that
> should be removed:
>
> +generate_series_numeric(PG_FUNCTION_ARGS)
> ...
> + if (NUMERIC_IS_NAN(start_num) ||
> NUMERIC_IS_NAN(finish_num))
> ...
> + if (PG_NARGS() == 3)
> ...
> + if (NUMERIC_IS_NAN(step_num))
>

Ah, didn't see it. Thanks. Fixed in this patch.

Regards,
--
Ali Akbar

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".
Regards,
--
Michael

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

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

>>>>> "Fujii" == Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:

Fujii> Pushed.

Bug found:

regression=# select count(*) from generate_series(1::numeric,10) v, generate_series(1,v) w;
count
-------
99990
(1 row)

regression=# select count(*) from generate_series(1::numeric,10) v, generate_series(1,v+0) w;
count
-------
55
(1 row)

The error is in the use of PG_GETARG_NUMERIC and init_var_from_num
when setting up the multi-call state; init_var_from_num points at the
original num's digits rather than copying them, but start_num and
stop_num have just been (potentially) detoasted in the per-call
context, in which case the storage will have been freed by the next
call.

Obviously this could also be fixed by not detoasting the input until
after switching to the multi-call context, but it looks to me like
that would be unnecessarily complex.

Suggested patch attached.

(Is it also worth putting an explicit warning about this kind of thing
in the SRF docs?)

--
Andrew (irc:RhodiumToad)

2014-12-15 10:25 GMT+07:00 Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>:
>
> >>>>> "Fujii" == Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>
> Fujii> Pushed.
>
> Bug found:
>
> regression=# select count(*) from generate_series(1::numeric,10) v,
> generate_series(1,v) w;
> count
> -------
> 99990
> (1 row)
>
> regression=# select count(*) from generate_series(1::numeric,10) v,
> generate_series(1,v+0) w;
> count
> -------
> 55
> (1 row)
>
> The error is in the use of PG_GETARG_NUMERIC and init_var_from_num
> when setting up the multi-call state; init_var_from_num points at the
> original num's digits rather than copying them, but start_num and
> stop_num have just been (potentially) detoasted in the per-call
> context, in which case the storage will have been freed by the next
> call.
>

Oops.

Obviously this could also be fixed by not detoasting the input until
> after switching to the multi-call context, but it looks to me like
> that would be unnecessarily complex.
>
> Suggested patch attached.
>

Thanks

> (Is it also worth putting an explicit warning about this kind of thing
> in the SRF docs?)
>

I think yes, it will be good. The alternative is restructuring this
paragraph in the SRF docs:

The memory context that is current when the SRF is called is a transient
> context that will be cleared between calls. This means that you do not need
> to call pfree on everything you allocated using palloc; it will go away
> anyway. However, if you want to allocate any data structures to live across
> calls, you need to put them somewhere else. The memory context referenced
> by multi_call_memory_ctx is a suitable location for any data that needs
> to survive until the SRF is finished running. In most cases, this means
> that you should switch into multi_call_memory_ctx while doing the
> first-call setup.
>

The important part "However, if you want to allocate any data structures to
live across calls, you need to put them somewhere else." is buried in the
docs.

But i think explicit warning is more noticeable for new developer like me.

Regards,
--
Ali Akbar

>>>>> "Ali" == Ali Akbar <the(dot)apaan(at)gmail(dot)com> writes:

Ali> I think yes, it will be good. The alternative is restructuring
Ali> this paragraph in the SRF docs:

>> The memory context that is current when the SRF is called is a
>> transient context that will be cleared between calls. This means
>> that you do not need to call pfree on everything you allocated
>> using palloc; it will go away anyway. However, if you want to
>> allocate any data structures to live across calls, you need to put
>> them somewhere else. The memory context referenced by
>> multi_call_memory_ctx is a suitable location for any data that
>> needs to survive until the SRF is finished running. In most cases,
>> this means that you should switch into multi_call_memory_ctx while
>> doing the first-call setup.

Ali> The important part "However, if you want to allocate any data
Ali> structures to live across calls, you need to put them somewhere
Ali> else." is buried in the docs.

Ali> But i think explicit warning is more noticeable for new
Ali> developer like me.

I was thinking something like this, added just after that para:

<warning>
<para>
While the actual arguments to the function remain unchanged between
calls, if you detoast the argument values (which is normally done
transparently by the
<function>PG_GETARG_<replaceable>xxx</replaceable></function> macro)
in the transient context then the detoasted copies will be freed on
each cycle. Accordingly, if you keep references to such values in
your <structfield>user_fctx</>, you must either copy them into the
<structfield>multi_call_memory_ctx</> after detoasting, or ensure
that you detoast the values only in that context.
</para>
</warning>

--
Andrew (irc:RhodiumToad)

On Mon, Dec 15, 2014 at 12:25 PM, Andrew Gierth
<andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
>>>>>> "Fujii" == Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>
> Fujii> Pushed.
>
> Bug found:
>
> regression=# select count(*) from generate_series(1::numeric,10) v, generate_series(1,v) w;
> count
> -------
> 99990
> (1 row)
>
> regression=# select count(*) from generate_series(1::numeric,10) v, generate_series(1,v+0) w;
> count
> -------
> 55
> (1 row)
>
> The error is in the use of PG_GETARG_NUMERIC and init_var_from_num
> when setting up the multi-call state; init_var_from_num points at the
> original num's digits rather than copying them, but start_num and
> stop_num have just been (potentially) detoasted in the per-call
> context, in which case the storage will have been freed by the next
> call.
>
> Obviously this could also be fixed by not detoasting the input until
> after switching to the multi-call context, but it looks to me like
> that would be unnecessarily complex.
>
> Suggested patch attached.

Pushed. Thanks!

Regards,

--
Fujii Masao

On Mon, Dec 15, 2014 at 2:38 PM, Andrew Gierth
<andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
>>>>>> "Ali" == Ali Akbar <the(dot)apaan(at)gmail(dot)com> writes:
>
> Ali> I think yes, it will be good. The alternative is restructuring
> Ali> this paragraph in the SRF docs:
>
> >> The memory context that is current when the SRF is called is a
> >> transient context that will be cleared between calls. This means
> >> that you do not need to call pfree on everything you allocated
> >> using palloc; it will go away anyway. However, if you want to
> >> allocate any data structures to live across calls, you need to put
> >> them somewhere else. The memory context referenced by
> >> multi_call_memory_ctx is a suitable location for any data that
> >> needs to survive until the SRF is finished running. In most cases,
> >> this means that you should switch into multi_call_memory_ctx while
> >> doing the first-call setup.
>
> Ali> The important part "However, if you want to allocate any data
> Ali> structures to live across calls, you need to put them somewhere
> Ali> else." is buried in the docs.
>
> Ali> But i think explicit warning is more noticeable for new
> Ali> developer like me.
>
> I was thinking something like this, added just after that para:
>
> <warning>
> <para>
> While the actual arguments to the function remain unchanged between
> calls, if you detoast the argument values (which is normally done
> transparently by the
> <function>PG_GETARG_<replaceable>xxx</replaceable></function> macro)
> in the transient context then the detoasted copies will be freed on
> each cycle. Accordingly, if you keep references to such values in
> your <structfield>user_fctx</>, you must either copy them into the
> <structfield>multi_call_memory_ctx</> after detoasting, or ensure
> that you detoast the values only in that context.
> </para>
> </warning>

I'm OK with this.

Regards,

--
Fujii Masao

2014-12-18 19:35 GMT+07:00 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>:

> On Mon, Dec 15, 2014 at 2:38 PM, Andrew Gierth
> <andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
> > I was thinking something like this, added just after that para:
> >
> > <warning>
> > <para>
> > While the actual arguments to the function remain unchanged between
> > calls, if you detoast the argument values (which is normally done
> > transparently by the
> > <function>PG_GETARG_<replaceable>xxx</replaceable></function>
> macro)
> > in the transient context then the detoasted copies will be freed on
> > each cycle. Accordingly, if you keep references to such values in
> > your <structfield>user_fctx</>, you must either copy them into the
> > <structfield>multi_call_memory_ctx</> after detoasting, or ensure
> > that you detoast the values only in that context.
> > </para>
> > </warning>
>
> I'm OK with this.
>

Wrapping the doc changes in a patch. Will add to next commitfest so it
won't be lost.

--
Ali Akbar

On Wed, Jan 14, 2015 at 11:04 AM, Ali Akbar <the(dot)apaan(at)gmail(dot)com> wrote:
>
> 2014-12-18 19:35 GMT+07:00 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>:
>>
>> On Mon, Dec 15, 2014 at 2:38 PM, Andrew Gierth
>> <andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
>> > I was thinking something like this, added just after that para:
>> >
>> > <warning>
>> > <para>
>> > While the actual arguments to the function remain unchanged
>> > between
>> > calls, if you detoast the argument values (which is normally done
>> > transparently by the
>> > <function>PG_GETARG_<replaceable>xxx</replaceable></function>
>> > macro)
>> > in the transient context then the detoasted copies will be freed
>> > on
>> > each cycle. Accordingly, if you keep references to such values in
>> > your <structfield>user_fctx</>, you must either copy them into the
>> > <structfield>multi_call_memory_ctx</> after detoasting, or ensure
>> > that you detoast the values only in that context.
>> > </para>
>> > </warning>
>>
>> I'm OK with this.
>
>
> Wrapping the doc changes in a patch. Will add to next commitfest so it won't
> be lost.

Pushed. Thanks!

Regards,

--
Fujii Masao