Re: [HACKERS] Add generate_series(numeric, numeric)
On Wed, Jan 14, 2015 at 11:04 AM, Ali Akbar the.ap...@gmail.com wrote: 2014-12-18 19:35 GMT+07:00 Fujii Masao masao.fu...@gmail.com: On Mon, Dec 15, 2014 at 2:38 PM, Andrew Gierth and...@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 functionPG_GETARG_replaceablexxx/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 structfielduser_fctx/, you must either copy them into the structfieldmulti_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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
2014-12-18 19:35 GMT+07:00 Fujii Masao masao.fu...@gmail.com: On Mon, Dec 15, 2014 at 2:38 PM, Andrew Gierth and...@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 functionPG_GETARG_replaceablexxx/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 structfielduser_fctx/, you must either copy them into the structfieldmulti_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 *** a/doc/src/sgml/xfunc.sgml --- b/doc/src/sgml/xfunc.sgml *** *** 2986,2991 SRF_RETURN_DONE(funcctx) --- 2986,3005 structfieldmulti_call_memory_ctx/ while doing the first-call setup. /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 + functionPG_GETARG_replaceablexxx/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 structfielduser_fctx/, you must either copy them into the + structfieldmulti_call_memory_ctx/ after detoasting, or ensure + that you detoast the values only in that context. + /para + /warning + para A complete pseudo-code example looks like the following: programlisting -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
On Mon, Dec 15, 2014 at 12:25 PM, Andrew Gierth and...@tao11.riddles.org.uk wrote: Fujii == Fujii Masao masao.fu...@gmail.com writes: Fujii Pushed. Bug found: regression=# select count(*) from generate_series(1::numeric,10) v, generate_series(1,v) w; count --- 0 (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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
On Mon, Dec 15, 2014 at 2:38 PM, Andrew Gierth and...@tao11.riddles.org.uk wrote: Ali == Ali Akbar the.ap...@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 functionPG_GETARG_replaceablexxx/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 structfielduser_fctx/, you must either copy them into the structfieldmulti_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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
Fujii == Fujii Masao masao.fu...@gmail.com writes: Fujii Pushed. Bug found: regression=# select count(*) from generate_series(1::numeric,10) v, generate_series(1,v) w; count --- 0 (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) diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c index c73f9bc..d841b6f 100644 --- a/src/backend/utils/adt/numeric.c +++ b/src/backend/utils/adt/numeric.c @@ -1325,11 +1325,16 @@ generate_series_step_numeric(PG_FUNCTION_ARGS) /* * Use fctx to keep state from call to call. Seed current with the - * original start value. + * original start value. We must copy the start_num and stop_num + * values rather than pointing to them, since we may have detoasted + * them in the per-call context. */ - init_var_from_num(start_num, fctx-current); - init_var_from_num(stop_num, fctx-stop); + init_var(fctx-current); + init_var(fctx-stop); init_var(fctx-step); + + set_var_from_num(start_num, fctx-current); + set_var_from_num(stop_num, fctx-stop); set_var_from_var(steploc, fctx-step); funcctx-user_fctx = fctx; diff --git a/src/test/regress/expected/numeric.out b/src/test/regress/expected/numeric.out index ee6cb50..9d68145 100644 --- a/src/test/regress/expected/numeric.out +++ b/src/test/regress/expected/numeric.out @@ -1461,3 +1461,41 @@ select (i / (10::numeric ^ 131071))::numeric(1,0) 9 (4 rows) +-- Check usage with variables +select * from generate_series(1::numeric, 3::numeric) i, generate_series(i,3) j; + i | j +---+--- + 1 | 1 + 1 | 2 + 1 | 3 + 2 | 2 + 2 | 3 + 3 | 3 +(6 rows) + +select * from generate_series(1::numeric, 3::numeric) i, generate_series(1,i) j; + i | j +---+--- + 1 | 1 + 2 | 1 + 2 | 2 + 3 | 1 + 3 | 2 + 3 | 3 +(6 rows) + +select * from generate_series(1::numeric, 3::numeric) i, generate_series(1,5,i) j; + i | j +---+--- + 1 | 1 + 1 | 2 + 1 | 3 + 1 | 4 + 1 | 5 + 2 | 1 + 2 | 3 + 2 | 5 + 3 | 1 + 3 | 4 +(10 rows) + diff --git a/src/test/regress/sql/numeric.sql b/src/test/regress/sql/numeric.sql index a7e92ac..1633e4c 100644 --- a/src/test/regress/sql/numeric.sql +++ b/src/test/regress/sql/numeric.sql @@ -854,3 +854,7 @@ select (i / (10::numeric ^ 131071))::numeric(1,0) from generate_series(6 * (10::numeric ^ 131071), 9 * (10::numeric ^ 131071), 10::numeric ^ 131071) as a(i); +-- Check usage with variables +select * from generate_series(1::numeric, 3::numeric) i, generate_series(i,3) j; +select * from generate_series(1::numeric, 3::numeric) i, generate_series(1,i) j; +select * from generate_series(1::numeric, 3::numeric) i, generate_series(1,5,i) j; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
2014-12-15 10:25 GMT+07:00 Andrew Gierth and...@tao11.riddles.org.uk: Fujii == Fujii Masao masao.fu...@gmail.com writes: Fujii Pushed. Bug found: regression=# select count(*) from generate_series(1::numeric,10) v, generate_series(1,v) w; count --- 0 (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
Re: [HACKERS] Add generate_series(numeric, numeric)
Ali == Ali Akbar the.ap...@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 functionPG_GETARG_replaceablexxx/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 structfielduser_fctx/, you must either copy them into the structfieldmulti_call_memory_ctx/ after detoasting, or ensure that you detoast the values only in that context. /para /warning -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
On Fri, Nov 7, 2014 at 3:19 PM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Oct 14, 2014 at 3:22 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Oct 7, 2014 at 8:38 AM, Ali Akbar the.ap...@gmail.com wrote: 2014-10-06 22:51 GMT+07:00 Marti Raudsepp ma...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
On Tue, Oct 14, 2014 at 3:22 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Oct 7, 2014 at 8:38 AM, Ali Akbar the.ap...@gmail.com wrote: 2014-10-06 22:51 GMT+07:00 Marti Raudsepp ma...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
On Tue, Oct 7, 2014 at 8:38 AM, Ali Akbar the.ap...@gmail.com wrote:
2014-10-06 22:51 GMT+07:00 Marti Raudsepp ma...@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
From 4f8591c47bb51ea86b77be645d9c321a1e9fb962 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@vmware.com
Date: Tue, 14 Oct 2014 14:39:51 +0900
Subject: [PATCH] Implement generate_series for numeric data type
This commit adds two new system functions to generate series of numbers
for the data type numeric, with the possibility to define a custom step
value. Note that Nan is not accepted in output for all the input
variables.
---
doc/src/sgml/func.sgml| 16 +++--
src/backend/utils/adt/numeric.c | 124 ++
src/include/catalog/pg_proc.h | 4 ++
src/include/utils/builtins.h | 2 +
src/test/regress/expected/numeric.out | 52 ++
src/test/regress/sql/numeric.sql | 17 +
6 files changed, 211 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7e5bcd9..b58cfa5 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14076,8 +14076,8 @@ AND
tbody
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter)/function/literal/entry
- entrytypeint/type or typebigint/type/entry
- entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
+ entrytypeint/type, typebigint/type or typenumeric/type/entry
+ entrytypesetof int/type, typesetof bigint/type, or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of one
@@ -14086,8 +14086,8 @@ AND
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter, parameterstep/parameter)/function/literal/entry
- entrytypeint/type or typebigint/type/entry
- entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
+ entrytypeint/type, typebigint/type or typenumeric/type/entry
+ entrytypesetof int/type, typesetof bigint/type or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of parameterstep/parameter
@@ -14137,6 +14137,14 @@ SELECT * FROM generate_series(4,3);
-
(0 rows)
+SELECT generate_series(1.1, 4, 1.3);
+ generate_series
+-
+ 1.1
+ 2.4
+ 3.7
+(3 rows)
+
-- this example relies on the date-plus-integer operator
SELECT current_date + s.a AS dates FROM generate_series(0,14,7) AS s(a);
dates
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 2d6a4cb..db2f862 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -28,6 +28,7 @@
#include access/hash.h
#include catalog/pg_type.h
+#include funcapi.h
#include libpq/pqformat.h
#include miscadmin.h
#include nodes/nodeFuncs.h
@@ -261,6 +262,18 @@ typedef struct NumericVar
/* --
+ * Data for generate_series
+ * --
+ */
+typedef struct
+{
+ NumericVar current;
+ NumericVar stop;
+ NumericVar step;
+} generate_series_numeric_fctx;
+
+
+/* --
* Some preinitialized constants
* --
*/
@@ -1221,6 +1234,117 @@ numeric_floor(PG_FUNCTION_ARGS)
PG_RETURN_NUMERIC(res);
}
+
Re: [HACKERS] Add generate_series(numeric, numeric)
On Sun, Oct 5, 2014 at 7:39 PM, Ali Akbar the.ap...@gmail.com wrote:
2014-10-05 15:21 GMT+07:00 Ali Akbar the.ap...@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:
if (foo)
{
blah;
blah2;
}
Code convention is detailed in the docs:
http://www.postgresql.org/docs/devel/static/source.html
Regards,
--
Michael
Re: [HACKERS] Add generate_series(numeric, numeric)
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
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 14074,14081 AND
tbody
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter)/function/literal/entry
! entrytypeint/type or typebigint/type/entry
! entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of one
--- 14074,14081
tbody
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter)/function/literal/entry
! entrytypeint/type, typebigint/type or typenumeric/type/entry
! entrytypesetof int/type, typesetof bigint/type, or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of one
***
*** 14084,14091 AND
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter, parameterstep/parameter)/function/literal/entry
! entrytypeint/type or typebigint/type/entry
! entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of parameterstep/parameter
--- 14084,14091
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter, parameterstep/parameter)/function/literal/entry
! entrytypeint/type, typebigint/type or typenumeric/type/entry
! entrytypesetof int/type, typesetof bigint/type or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of parameterstep/parameter
*** a/src/backend/utils/adt/numeric.c
--- b/src/backend/utils/adt/numeric.c
***
*** 28,33
--- 28,34
#include access/hash.h
#include catalog/pg_type.h
+ #include funcapi.h
#include libpq/pqformat.h
#include miscadmin.h
#include nodes/nodeFuncs.h
***
*** 261,266 typedef struct NumericVar
--- 262,279
/* --
+ * Data for generate series
+ * --
+ */
+ typedef struct
+ {
+ NumericVar current;
+ NumericVar finish;
+ NumericVar step;
+ } generate_series_numeric_fctx;
+
+
+ /* --
* Some preinitialized constants
* --
*/
***
*** 1221,1226 numeric_floor(PG_FUNCTION_ARGS)
--- 1234,1346
PG_RETURN_NUMERIC(res);
}
+
+ /*
+ * generate_series_numeric() -
+ *
+ * Generate series of numeric.
+ */
+ Datum
+ generate_series_numeric(PG_FUNCTION_ARGS)
+ {
+ return generate_series_step_numeric(fcinfo);
+ }
+
+ Datum
+ generate_series_step_numeric(PG_FUNCTION_ARGS)
+ {
+ generate_series_numeric_fctx *fctx;
+ FuncCallContext *funcctx;
+ MemoryContext oldcontext;
+
+ if (SRF_IS_FIRSTCALL())
+ {
+ Numeric start_num = PG_GETARG_NUMERIC(0);
+ Numeric finish_num = PG_GETARG_NUMERIC(1);
+ NumericVar steploc = const_one;
+
+ /* Handle NaN in start finish */
+ if (NUMERIC_IS_NAN(start_num) || NUMERIC_IS_NAN(finish_num))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(start and finish cannot be NaN)));
+
+ /* see if we
Re: [HACKERS] Add generate_series(numeric, numeric)
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.ap...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
+ 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
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 14074,14081 AND
tbody
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter)/function/literal/entry
! entrytypeint/type or typebigint/type/entry
! entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of one
--- 14074,14081
tbody
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter)/function/literal/entry
! entrytypeint/type, typebigint/type or typenumeric/type/entry
! entrytypesetof int/type, typesetof bigint/type, or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of one
***
*** 14084,14091 AND
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter, parameterstep/parameter)/function/literal/entry
! entrytypeint/type or typebigint/type/entry
! entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of parameterstep/parameter
--- 14084,14091
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter, parameterstep/parameter)/function/literal/entry
! entrytypeint/type, typebigint/type or typenumeric/type/entry
! entrytypesetof int/type, typesetof bigint/type or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of parameterstep/parameter
*** a/src/backend/utils/adt/numeric.c
--- b/src/backend/utils/adt/numeric.c
***
*** 28,33
--- 28,34
#include access/hash.h
#include catalog/pg_type.h
+ #include funcapi.h
#include libpq/pqformat.h
#include miscadmin.h
#include nodes/nodeFuncs.h
***
*** 261,266 typedef struct NumericVar
--- 262,279
/* --
+ * Data for generate series
+ * --
+ */
+ typedef struct
+ {
+ NumericVar current;
+ NumericVar finish;
+ NumericVar step;
+ } generate_series_numeric_fctx;
+
+
+ /* --
* Some preinitialized constants
* --
*/
***
*** 1221,1226 numeric_floor(PG_FUNCTION_ARGS)
--- 1234,1346
PG_RETURN_NUMERIC(res);
}
+
+ /*
+ * generate_series_numeric() -
+ *
+ * Generate series of numeric.
+ */
+ Datum
+ generate_series_numeric(PG_FUNCTION_ARGS)
+ {
+ return generate_series_step_numeric(fcinfo);
+ }
+
+ Datum
+ generate_series_step_numeric(PG_FUNCTION_ARGS)
+ {
+ generate_series_numeric_fctx *fctx;
+ FuncCallContext *funcctx;
+ MemoryContext oldcontext;
+
+ if (SRF_IS_FIRSTCALL())
+ {
+ Numeric start_num = PG_GETARG_NUMERIC(0);
+ Numeric finish_num = PG_GETARG_NUMERIC(1);
+ NumericVar steploc = const_one;
+
+ /* Handle NaN in start finish */
+ if (NUMERIC_IS_NAN(start_num) || NUMERIC_IS_NAN(finish_num))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(start and finish cannot be NaN)));
+
+ /* see if we were given an explicit step size */
+ if (PG_NARGS() == 3)
+ {
+ Numeric step_num = PG_GETARG_NUMERIC(2);
+
+ if (NUMERIC_IS_NAN(step_num))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(step size cannot be NaN)));
+
+ init_var_from_num(step_num, steploc);
+
+ if (cmp_var(steploc, const_zero) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(step size cannot equal zero)));
+
Re: [HACKERS] Add generate_series(numeric, numeric)
On Mon, Oct 6, 2014 at 6:12 PM, Ali Akbar the.ap...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add generate_series(numeric, numeric)
On Tue, Oct 7, 2014 at 12:51 AM, Marti Raudsepp ma...@juffo.org wrote: On Mon, Oct 6, 2014 at 6:12 PM, Ali Akbar the.ap...@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. 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
Re: [HACKERS] Add generate_series(numeric, numeric)
2014-10-06 22:51 GMT+07:00 Marti Raudsepp ma...@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);
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
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 14074,14081 AND
tbody
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter)/function/literal/entry
! entrytypeint/type or typebigint/type/entry
! entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of one
--- 14074,14081
tbody
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter)/function/literal/entry
! entrytypeint/type, typebigint/type or typenumeric/type/entry
! entrytypesetof int/type, typesetof bigint/type, or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of one
***
*** 14084,14091 AND
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter, parameterstep/parameter)/function/literal/entry
! entrytypeint/type or typebigint/type/entry
! entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of parameterstep/parameter
--- 14084,14091
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter, parameterstep/parameter)/function/literal/entry
! entrytypeint/type, typebigint/type or typenumeric/type/entry
! entrytypesetof int/type, typesetof bigint/type or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of parameterstep/parameter
*** a/src/backend/utils/adt/numeric.c
--- b/src/backend/utils/adt/numeric.c
***
*** 28,33
--- 28,34
#include access/hash.h
#include catalog/pg_type.h
+ #include funcapi.h
#include libpq/pqformat.h
#include miscadmin.h
#include nodes/nodeFuncs.h
***
*** 261,266 typedef struct NumericVar
--- 262,279
/* --
+ * Data for generate series
+ * --
+ */
+ typedef struct
+ {
+ NumericVar current;
+ NumericVar finish;
+ NumericVar step;
+ } generate_series_numeric_fctx;
+
+
+ /* --
* Some preinitialized constants
* --
*/
***
*** 1221,1226 numeric_floor(PG_FUNCTION_ARGS)
--- 1234,1346
PG_RETURN_NUMERIC(res);
}
+
+ /*
+ * generate_series_numeric() -
+ *
+ * Generate series of numeric.
+ */
+ Datum
+ generate_series_numeric(PG_FUNCTION_ARGS)
+ {
+ return generate_series_step_numeric(fcinfo);
+ }
+
+ Datum
+ generate_series_step_numeric(PG_FUNCTION_ARGS)
+ {
+ generate_series_numeric_fctx *fctx;
+ FuncCallContext *funcctx;
+ MemoryContext oldcontext;
+
+ if (SRF_IS_FIRSTCALL())
+ {
+ Numeric start_num = PG_GETARG_NUMERIC(0);
+ Numeric finish_num = PG_GETARG_NUMERIC(1);
+ NumericVar steploc = const_one;
+
+ /* Handle NaN in start finish */
+ if (NUMERIC_IS_NAN(start_num) || NUMERIC_IS_NAN(finish_num))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(start and finish cannot be NaN)));
+
+ /* see if we were
Re: [HACKERS] Add generate_series(numeric, numeric)
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
Re: [HACKERS] Add generate_series(numeric, numeric)
2014-10-05 15:21 GMT+07:00 Ali Akbar the.ap...@gmail.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
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 14074,14081 AND
tbody
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter)/function/literal/entry
! entrytypeint/type or typebigint/type/entry
! entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of one
--- 14074,14081
tbody
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter)/function/literal/entry
! entrytypeint/type, typebigint/type or typenumeric/type/entry
! entrytypesetof int/type, typesetof bigint/type, or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of one
***
*** 14084,14091 AND
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter, parameterstep/parameter)/function/literal/entry
! entrytypeint/type or typebigint/type/entry
! entrytypesetof int/type or typesetof bigint/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of parameterstep/parameter
--- 14084,14091
row
entryliteralfunctiongenerate_series(parameterstart/parameter, parameterstop/parameter, parameterstep/parameter)/function/literal/entry
! entrytypeint/type, typebigint/type or typenumeric/type/entry
! entrytypesetof int/type, typesetof bigint/type or typesetof numeric/type (same as argument type)/entry
entry
Generate a series of values, from parameterstart/parameter to parameterstop/parameter
with a step size of parameterstep/parameter
*** a/src/backend/utils/adt/numeric.c
--- b/src/backend/utils/adt/numeric.c
***
*** 28,33
--- 28,34
#include access/hash.h
#include catalog/pg_type.h
+ #include funcapi.h
#include libpq/pqformat.h
#include miscadmin.h
#include nodes/nodeFuncs.h
***
*** 261,266 typedef struct NumericVar
--- 262,279
/* --
+ * Data for generate series
+ * --
+ */
+ typedef struct
+ {
+ NumericVar current;
+ NumericVar finish;
+ NumericVar step;
+ } generate_series_numeric_fctx;
+
+
+ /* --
* Some preinitialized constants
* --
*/
***
*** 1221,1226 numeric_floor(PG_FUNCTION_ARGS)
--- 1234,1346
PG_RETURN_NUMERIC(res);
}
+
+ /*
+ * generate_series_numeric() -
+ *
+ * Generate series of numeric.
+ */
+ Datum
+ generate_series_numeric(PG_FUNCTION_ARGS) {
+ return generate_series_step_numeric(fcinfo);
+ }
+
+ Datum
+ generate_series_step_numeric(PG_FUNCTION_ARGS)
+ {
+ generate_series_numeric_fctx *fctx;
+ FuncCallContext *funcctx;
+ MemoryContext oldcontext;
+
+ if (SRF_IS_FIRSTCALL())
+ {
+ Numeric start_num = PG_GETARG_NUMERIC(0);
+ Numeric finish_num = PG_GETARG_NUMERIC(1);
+ NumericVar
Re: [HACKERS] Add generate_series(numeric, numeric)
Also, Платон Малюгин, can you add this patch to postgresql commitfest ( http://commitfest.postgresql.org)? -- Ali Akbar
Re: [HACKERS] Add generate_series(numeric, numeric)
Hi,
Nice patch! And welcome here.
On Mon, Sep 29, 2014 at 12:42 PM, Платон Малюгин malugi...@gmail.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 -
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
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 2d6a4cb..975843d 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -28,6 +28,7 @@
#include access/hash.h
#include catalog/pg_type.h
+#include funcapi.h
#include libpq/pqformat.h
#include miscadmin.h
#include nodes/nodeFuncs.h
@@ -260,6 +261,13 @@ typedef struct NumericVar
} NumericVar;
+typedef struct
+{
+ NumericVar current;
+ NumericVar finish;
+ NumericVar step;
+} generate_series_numeric_fctx;
+
/* --
* Some preinitialized constants
* --
@@ -1221,6 +1229,111 @@ numeric_floor(PG_FUNCTION_ARGS)
PG_RETURN_NUMERIC(res);
}
+
+/*
+ * generate_series_numeric() -
+ *
+ * Generate series of numeric.
+ */
+Datum
+generate_series_numeric(PG_FUNCTION_ARGS)
+{
+ generate_series_numeric_fctx *fctx;
+ FuncCallContext *funcctx;
+ Numeric res;
+ NumericVar current_var;
+ NumericVar finish_var;
+ NumericVar step_var;
+
+ if (SRF_IS_FIRSTCALL())
+ {
+ Numeric start = PG_GETARG_NUMERIC(0);
+ Numeric finish = PG_GETARG_NUMERIC(1);
+ NumericVar steploc;
+ MemoryContext oldcontext;
+
+ /* see if we were given an explicit step size */
+ if (PG_NARGS() == 3)
+ {
+ Numeric step;
+ step = PG_GETARG_NUMERIC(2);
+
+ /* Transform step into a variable that can be manipulated */
+ init_var_from_num(step, steploc);
+ }
+ else
+ {
+ init_var(steploc);
+ set_var_from_var(steploc, const_one);
+ }
+
+ /* Step cannot be zero */
+ if (cmp_var(steploc, const_zero) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(step size cannot equal zero)));
+
+ /*
+ * switch to memory context appropriate for multiple function calls
+ */
+ funcctx = SRF_FIRSTCALL_INIT();
+
+ /* allocate memory for user context */
+ oldcontext = MemoryContextSwitchTo(funcctx-multi_call_memory_ctx);
+
+ /* allocate memory for user context */
+ fctx = (generate_series_numeric_fctx *)
+ palloc(sizeof(generate_series_numeric_fctx));
+
+ /*
+ * Use fctx to keep state from call to call. Seed current with the
+ * original start value
+ */
+ init_var_from_num(start, current_var);
+ init_var_from_num(finish, finish_var);
+ set_var_from_var(steploc, step_var);
+ free_var(steploc);
+ fctx-current = current_var;
+ fctx-finish = finish_var;
+ fctx-step = step_var;
+ funcctx-user_fctx = fctx;
+ MemoryContextSwitchTo(oldcontext);
+ }
+
+ /* stuff done on every call of the function */
+ funcctx = SRF_PERCALL_SETUP();
+
+ /*
+ * get the saved state and use current as the result for this iteration
+ */
+ fctx = funcctx-user_fctx;
+ current_var = fctx-current;
+ finish_var = fctx-finish;
+ step_var = fctx-step;
+ res = make_result(current_var);
+
+ if ((cmp_var(step_var, const_zero) 0
+ cmp_var(current_var, finish_var) = 0) ||
+ (cmp_var(step_var, const_zero) 0
+ cmp_var(current_var, finish_var) = 0))
+ {
+ NumericVar tmp;
+ init_var(tmp);
+
+ /* Increment for next
Re: [HACKERS] Add generate_series(numeric, numeric)
On Mon, Sep 29, 2014 at 4:19 PM, Michael Paquier michael.paqu...@gmail.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
Re: [HACKERS] Add generate_series(numeric, numeric)
On Tue, Sep 30, 2014 at 10:00 AM, Michael Paquier michael.paqu...@gmail.com
wrote:
On Mon, Sep 29, 2014 at 4:19 PM, Michael Paquier
michael.paqu...@gmail.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
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 2d6a4cb..4c2ff5b 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -28,6 +28,7 @@
#include access/hash.h
#include catalog/pg_type.h
+#include funcapi.h
#include libpq/pqformat.h
#include miscadmin.h
#include nodes/nodeFuncs.h
@@ -260,6 +261,13 @@ typedef struct NumericVar
} NumericVar;
+typedef struct
+{
+ NumericVar current;
+ NumericVar finish;
+ NumericVar step;
+} generate_series_numeric_fctx;
+
/* --
* Some preinitialized constants
* --
@@ -1221,6 +1229,108 @@ numeric_floor(PG_FUNCTION_ARGS)
PG_RETURN_NUMERIC(res);
}
+
+/*
+ * generate_series_numeric() -
+ *
+ * Generate series of numeric.
+ */
+Datum
+generate_series_numeric(PG_FUNCTION_ARGS)
+{
+ generate_series_numeric_fctx *fctx;
+ FuncCallContext *funcctx;
+ Numeric res;
+
+ if (SRF_IS_FIRSTCALL())
+ {
+ Numeric start_num = PG_GETARG_NUMERIC(0);
+ Numeric finish_num = PG_GETARG_NUMERIC(1);
+ NumericVar steploc;
+ MemoryContext oldcontext;
+
+ /* see if we were given an explicit step size */
+ if (PG_NARGS() == 3)
+ {
+ Numeric step_num;
+ step_num = PG_GETARG_NUMERIC(2);
+
+ /* Transform step into a variable that can be manipulated */
+ init_var_from_num(step_num, steploc);
+ }
+ else
+ {
+ init_var(steploc);
+ set_var_from_var(const_one, steploc);
+ }
+
+ /* Step cannot be zero */
+ if (cmp_var(steploc, const_zero) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(step size cannot equal zero)));
+
+ /*
+ * switch to memory context appropriate for multiple function calls
+ */
+ funcctx = SRF_FIRSTCALL_INIT();
+
+ /* allocate memory for user context */
+ oldcontext = MemoryContextSwitchTo(funcctx-multi_call_memory_ctx);
+
+ /* allocate memory for user context */
+ fctx = (generate_series_numeric_fctx *)
+ palloc(sizeof(generate_series_numeric_fctx));
+
+ /*
+ * Use fctx to keep state from call to call. Seed current with the
+ * original start value
+ */
+ init_var_from_num(start_num, (fctx-current));
+ init_var_from_num(finish_num, (fctx-finish));
+ init_var((fctx-step));
+ set_var_from_var(steploc, (fctx-step));
+ funcctx-user_fctx = fctx;
+ free_var(steploc);
+ MemoryContextSwitchTo(oldcontext);
+ }
+
+ /* stuff done on every call of the function */
+ funcctx = SRF_PERCALL_SETUP();
+
+ /*
+ * get the saved state and use current as the result for this iteration
+ */
+ fctx = funcctx-user_fctx;
+ res = make_result((fctx-current));
+
+ if ((cmp_var((fctx-step), const_zero) 0
+ cmp_var((fctx-current), (fctx-finish)) = 0) ||
+ (cmp_var((fctx-step), const_zero) 0
+ cmp_var((fctx-current), (fctx-finish)) = 0))
+ {
+ NumericVar tmp;
+ MemoryContext oldcontext;
+
+ /*
+ * Increment for next iteration and perform operation in the context
+ * of this function. Switch back to memory context of function before
+ * performing any operations.
+ */
+ oldcontext = MemoryContextSwitchTo(funcctx-multi_call_memory_ctx);
+ init_var(tmp);
+ add_var((fctx-current), (fctx-step), tmp);
+ set_var_from_var(tmp, (fctx-current));
+ free_var(tmp);
+ MemoryContextSwitchTo(oldcontext);
+ SRF_RETURN_NEXT(funcctx, NumericGetDatum(res));
+ }
+ else
+ {
+ SRF_RETURN_DONE(funcctx);
+ }
+}
+
/*
* Implements the numeric version of the width_bucket() function
* defined by SQL2003. See also width_bucket_float8().
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 3ce9849..ccdf3e8 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3923,6 +3923,10 @@ DATA(insert OID = 1068 ( generate_series PGNSP PGUID 12 1 1000 0 0 f f f f t t
DESCR(non-persistent series generator);
DATA(insert OID = 1069 ( generate_series PGNSP PGUID 12 1
