Thread: 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.
Attachment
Hi,
Nice patch! And welcome here.On Mon, Sep 29, 2014 at 12:42 PM, Платон Малюгин <malugin.p@gmail.com> wrote:
Regards,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
$ 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
--
Michael
Michael
Attachment
On Mon, Sep 29, 2014 at 4:19 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
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:MichaelBtw, 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.
- 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
=# 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
Michael
On Tue, Sep 30, 2014 at 10:00 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
-- And here is the patch...On Mon, Sep 29, 2014 at 4:19 PM, Michael Paquier <michael.paquier@gmail.com> wrote: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:MichaelBtw, 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.- 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.
Michael
Attachment
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:
--
Ali Akbar
- 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.apaan@gmail.com>:
attached the patch. Not checking if it overflows, but testing it with 9 * 10 ^ 131072 works (not resulting in an error). Other modifications:HiOops, 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
- 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
Attachment
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.apaan@gmail.com> wrote:
Regards,2014-10-05 15:21 GMT+07:00 Ali Akbar <the.apaan@gmail.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:
http://www.postgresql.org/docs/devel/static/source.html
if (foo)
{
{
blah;
blah2;
}
Code convention is detailed in the docs:}
http://www.postgresql.org/docs/devel/static/source.html
--
Michael
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 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);
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)
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
Ali Akbar
Attachment
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.apaan@gmail.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,
Regards,
Ali Akbar
Attachment
On Mon, Oct 6, 2014 at 6:12 PM, Ali Akbar <the.apaan@gmail.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
<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Tue, Oct 7, 2014 at 12:51 AM, Marti Raudsepp<span dir="ltr"><<a href="mailto:marti@juffo.org" target="_blank">marti@juffo.org</a>></span> wrote:<br /><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">OnMon, Oct 6, 2014 at 6:12 PM, Ali Akbar <<a href="mailto:the.apaan@gmail.com">the.apaan@gmail.com</a>> wrote:<br/> > User apaan is me. When i added to the commitfest, the patch is listed there<br /> > by me (apaan).<br/></span>That's fine I think, it's just for tracking who made the changes in<br /> the CommitFest app. What actuallymatters is what you write in the<br /> "Author" field, which could contain all 3 names separated by commas.<br /></blockquote></div>It'sfine not to add mine to the list of authors, I did a hack review. Feel free to add it to the listof reviewers though.<br />-- <br />Michael<br /></div></div>
2014-10-06 22:51 GMT+07:00 Marti Raudsepp <marti@juffo.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);
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
Ali Akbar
Attachment
On Tue, Oct 7, 2014 at 8:38 AM, Ali Akbar <the.apaan@gmail.com> wrote:
-- 2014-10-06 22:51 GMT+07:00 Marti Raudsepp <marti@juffo.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
Attachment
On Tue, Oct 14, 2014 at 3:22 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > > > On Tue, Oct 7, 2014 at 8:38 AM, Ali Akbar <the.apaan@gmail.com> wrote: >> >> 2014-10-06 22:51 GMT+07:00 Marti Raudsepp <marti@juffo.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.fujii@gmail.com> wrote: > On Tue, Oct 14, 2014 at 3:22 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> >> >> On Tue, Oct 7, 2014 at 8:38 AM, Ali Akbar <the.apaan@gmail.com> wrote: >>> >>> 2014-10-06 22:51 GMT+07:00 Marti Raudsepp <marti@juffo.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.fujii@gmail.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)
Attachment
2014-12-15 10:25 GMT+07:00 Andrew Gierth <andrew@tao11.riddles.org.uk>:
I think yes, it will be good. The alternative is restructuring this paragraph in the SRF docs:>>>>> "Fujii" == Fujii Masao <masao.fujii@gmail.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?)
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 callpfreeon everything you allocated usingpalloc; 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.
But i think explicit warning is more noticeable for new developer like me.
Regards,
--
Ali Akbar
>>>>> "Ali" == Ali Akbar <the.apaan@gmail.com> writes: Ali> I think yes, it will be good. The alternative is restructuringAli> 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 surviveuntil 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 dataAli> structures to live across calls, you need to put themsomewhereAli> else." is buried in the docs. Ali> But i think explicit warning is more noticeable for newAli> 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 detoastthe argument values (which is normally done transparently by the <function>PG_GETARG_<replaceable>xxx</replaceable></function>macro) in the transient context then the detoasted copieswill 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@tao11.riddles.org.uk> wrote: >>>>>> "Fujii" == Fujii Masao <masao.fujii@gmail.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@tao11.riddles.org.uk> wrote: >>>>>> "Ali" == Ali Akbar <the.apaan@gmail.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.fujii@gmail.com>:
I'm OK with this.On Mon, Dec 15, 2014 at 2:38 PM, Andrew Gierth
<andrew@tao11.riddles.org.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>
Wrapping the doc changes in a patch. Will add to next commitfest so it won't be lost.
--
Ali Akbar
Attachment
On Wed, Jan 14, 2015 at 11:04 AM, Ali Akbar <the.apaan@gmail.com> wrote: > > 2014-12-18 19:35 GMT+07:00 Fujii Masao <masao.fujii@gmail.com>: >> >> On Mon, Dec 15, 2014 at 2:38 PM, Andrew Gierth >> <andrew@tao11.riddles.org.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