Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
* Itagaki Takahiro (itagaki.takah...@gmail.com) wrote: On Mon, Jan 24, 2011 at 13:05, Stephen Frost sfr...@snowman.net wrote: FOR var in ARRAY array_expression ... I like that a lot more than inventing a new top-level keyword, AFAIR, the syntax is not good at an array literal. FOR var IN ARRAY ARRAY[1,2,5] LOOP ... I don't really see why that's not good? So you have 'ARRAY' twice.. So what? That's better than having a new top-level FOREACH that doesn't have any reason to exist except to be different from FOR and to not do the same thing.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
* Pavel Stehule (pavel.steh...@gmail.com) wrote: FOR keyword - please, look on thread about my proposal FOR-IN-ARRAY I did, and I still don't agree w/ using FOREACH. I work with FOUND variable, because I like a consistent behave with FOR statement. When FOUND is true after cycle, you are sure, so there was a minimally one iteration. Then the documentation around FOUND needs to be updated for FOREACH, and there's probably other places that need to be changed too. There also appears to be some extra whitespace changes that aren't necessary and a number of places where you don't follow the indentation conventions (eg: variable definitions in exec_stmt_foreach_a()). I am really not sure about correct indentation of variables :(, if you know a correct number of spaces, please, fix it. It's not a matter of a 'correct number of space'- make it the same as what it is in the rest of the code... The gist of it is to make the variable names all line up (with maximum use of tabs at 4-spaces per tab, of course): int my_var; char *my_string; double my_double; etc, etc. I'll try to redesign main cycle. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
2011/1/29 Stephen Frost sfr...@snowman.net: * Itagaki Takahiro (itagaki.takah...@gmail.com) wrote: On Mon, Jan 24, 2011 at 13:05, Stephen Frost sfr...@snowman.net wrote: FOR var in ARRAY array_expression ... I like that a lot more than inventing a new top-level keyword, AFAIR, the syntax is not good at an array literal. FOR var IN ARRAY ARRAY[1,2,5] LOOP ... I don't really see why that's not good? So you have 'ARRAY' twice.. So what? That's better than having a new top-level FOREACH that doesn't have any reason to exist except to be different from FOR and to not do the same thing.. I don't see a problem too, but we didn't find a compromise with this syntax, so I left it. It is true, so current implementation of FOR stmt is really baroque and next argument is a compatibility with PL/SQL. My idea is so FOR stmt will be a compatible with PL/SQL original, and FOREACH can be a platform for PostgreSQL specific code. Regards Pavel Thanks, Stephen -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) iEYEARECAAYFAk1D/u8ACgkQrzgMPqB3kij2IwCfZ3W+mGc7LedBdnt9lCa0vYjk m6QAn0xh7r6oTs+T47k+EuwZRpU2T0X8 =ruBa -END PGP SIGNATURE- -- 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] REVIEW: WIP: plpgsql - foreach in
I'll try to redesign main cycle. Thanks, please, can you look on code that I sent last time? Pavel Stephen -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) iEUEARECAAYFAk1EAJwACgkQrzgMPqB3kig5bACdH0fm8Klh7Dq1GlICV/Z8yEd4 KQoAlRZEeTrBkKK6TwjZrKmFDDeRfKE= =JPG4 -END PGP SIGNATURE- -- 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] REVIEW: WIP: plpgsql - foreach in
* Pavel Stehule (pavel.steh...@gmail.com) wrote: I don't see a problem too, but we didn't find a compromise with this syntax, so I left it. It is true, so current implementation of FOR stmt is really baroque and next argument is a compatibility with PL/SQL. My idea is so FOR stmt will be a compatible with PL/SQL original, and FOREACH can be a platform for PostgreSQL specific code. I see that as an absolutely horrible idea. If you want that, it should be PGFOR or something, but I don't buy off on the idea that we should invent new top-level PG-specific keywords for PL/PgSQL because they're PG-specific. I also don't see why FOR wouldn't still be as compatible w/ PL/SQL as it was before (except in the possible case where someone actually has 'ARRAY' there already, but I'm pretty sure we can convince ourselves that such a construct is very unlikely to appear in the wild). I certainly don't think we should *not* do something under FOR because we're worried that people might use it and then get unhappy when they port that code to PL/SQL. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
* Pavel Stehule (pavel.steh...@gmail.com) wrote:
please, can you look on code that I sent last time?
I'm looking at it now and I still don't like the big set of conditionals
at the beginning which sets things up. I do think the loop is a bit
better, but have you considered factoring out the array slicing..? If
the built-ins don't give you what you want, write your own? Or make
them do what you need?
Point is, that array-slicing logic has a very clear and defined purpose,
input and output, and it could be another function.
Err, oh, except you have this horribly named 'ptr' variable that's
being used across the loops. Gah. Alright, I'd suggest actually making
an array iterator function then, which can single-step and pull out
slices, along with a single-step iterator, and then changing the top
level to be a while() loop on that. Notionally it's like this:
while (curr_slice_ptr =
array_slice(arr, curr_slice_ptr, slice_len, slice, isnull))
{
exec_assign_value(estate, ctrl_var, slice, valtype, isnull);
rc = exec_stmts(estate, stmt-body);
/* handle return codes */
}
array_slice() could be defined to return a single value if slice_len is
1, or you could just pull out the single element if it returned a
1-element array; either way would work, imv, so long as it's commented
appropriately.
Those are my thoughts at the moment. To be honest, I'd really like to
see this patch included in 9.1, since I can see myself using this
feature, so if you need help with some of this rework, let me know.
Thanks,
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
2011/1/29 Stephen Frost sfr...@snowman.net: * Pavel Stehule (pavel.steh...@gmail.com) wrote: I don't see a problem too, but we didn't find a compromise with this syntax, so I left it. It is true, so current implementation of FOR stmt is really baroque and next argument is a compatibility with PL/SQL. My idea is so FOR stmt will be a compatible with PL/SQL original, and FOREACH can be a platform for PostgreSQL specific code. I see that as an absolutely horrible idea. If you want that, it should be PGFOR or something, but I don't buy off on the idea that we should invent new top-level PG-specific keywords for PL/PgSQL because they're PG-specific. I also don't see why FOR wouldn't still be as compatible w/ PL/SQL as it was before (except in the possible case where someone actually has 'ARRAY' there already, but I'm pretty sure we can convince ourselves that such a construct is very unlikely to appear in the wild). you can rename it as some different than original ADA concept, if you like. It is similar like FORALL cycle from PL/SQL. Native ADA's FOR cycle doesn't know iteration over array. You have a similar opinion like me about design this statement. But there are others with strong negative opinion. For someone ARRAY ARRAY should be a problem. So FOREACH is third way - more, it increase a possibility for enhancing plpgsql in future. I certainly don't think we should *not* do something under FOR because we're worried that people might use it and then get unhappy when they port that code to PL/SQL. PL/pgSQL is some like Frankenstein :) Fast, functional but sometime strange - more stranger than origin. I don't think so it necessary to do live harder for people who have to work with both databases. the main issue was a maintainability of more complex FOR statement. Pavel Thanks, Stephen -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) iEYEARECAAYFAk1EBDwACgkQrzgMPqB3kijDLQCcCpb15jTvU3rIdh5j9ipaqq+X G+wAn2WrxDkgArf5xHxt4bi8EpE0HVFP =Fx/8 -END PGP SIGNATURE- -- 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] REVIEW: WIP: plpgsql - foreach in
* Pavel Stehule (pavel.steh...@gmail.com) wrote: You have a similar opinion like me about design this statement. But there are others with strong negative opinion. For someone ARRAY ARRAY should be a problem. So FOREACH is third way - more, it increase a possibility for enhancing plpgsql in future. I look forward to hearing from the silent majority on this then. the main issue was a maintainability of more complex FOR statement. That would be a reason to not have this functionality at all, not a reason to add confusion with a new top-level command. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
Stephen Frost sfr...@snowman.net writes: * Pavel Stehule (pavel.steh...@gmail.com) wrote: You have a similar opinion like me about design this statement. But there are others with strong negative opinion. For someone ARRAY ARRAY should be a problem. So FOREACH is third way - more, it increase a possibility for enhancing plpgsql in future. I look forward to hearing from the silent majority on this then. Well, I haven't been exactly silent, but I was one of the people telling Pavel not to use FOR in the first place. The trouble is that we've already overloaded FOR to within an inch of its life. Adding yet another potential syntax to follow FOR ... IN ... is just a bad idea, especially since Pavel has evidently got ambitions to add yet more nonstandard hac^H^H^Hfeatures here. I have to agree that FOREACH is pretty ugly too, but I do *not* want to use a syntax that can so easily be confused with the existing types of for-loops. We'd pay a significant price in the ability to issue syntax error messages that were actually relevant to what the user thought he was doing, for example. See also http://archives.postgresql.org/pgsql-hackers/2010-12/msg01579.php which tries to draw a clear distinction between what FOR does and what FOREACH does. regards, tom lane -- 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] REVIEW: WIP: plpgsql - foreach in
* Tom Lane (t...@sss.pgh.pa.us) wrote: See also http://archives.postgresql.org/pgsql-hackers/2010-12/msg01579.php which tries to draw a clear distinction between what FOR does and what FOREACH does. Thanks for that, somehow I had missed that post previously. I think I can get behind the idea of FOREACH being used for 'vertical' (multi-value in a single value) loops while FOR is used for 'horizontal' (multi-row). This patch certainly needs to be improved to document this, in the grammar, in the code via comments, and in the actual documentation. It also needs to touch any place that talks about the other kinds of loops to be sure that FOREACH is included and that it's behavior is documented accordingly. Thanks again, Stephen signature.asc Description: Digital signature
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
2011/1/29 Stephen Frost sfr...@snowman.net: * Tom Lane (t...@sss.pgh.pa.us) wrote: See also http://archives.postgresql.org/pgsql-hackers/2010-12/msg01579.php which tries to draw a clear distinction between what FOR does and what FOREACH does. Thanks for that, somehow I had missed that post previously. I think I can get behind the idea of FOREACH being used for 'vertical' (multi-value in a single value) loops while FOR is used for 'horizontal' (multi-row). This patch certainly needs to be improved to document this, in the grammar, in the code via comments, and in the actual documentation. It also needs to touch any place that talks about the other kinds of loops to be sure that FOREACH is included and that it's behavior is documented accordingly. Stephen, please, update documentation freely. Current documentation is really minimal. Regards Pavel Thanks again, Stephen -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) iEYEARECAAYFAk1EZJEACgkQrzgMPqB3kijrawCfbvtV/2QoJ6nLvKZENcslQgz+ do8An2Q7MvgubhLqrfbVCMiG29+RG3cp =RpHJ -END PGP SIGNATURE- -- 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] REVIEW: WIP: plpgsql - foreach in
On Mon, Jan 24, 2011 at 13:05, Stephen Frost sfr...@snowman.net wrote: FOR var in ARRAY array_expression ... I like that a lot more than inventing a new top-level keyword, AFAIR, the syntax is not good at an array literal. FOR var IN ARRAY ARRAY[1,2,5] LOOP ... And it was the only drawback compared with FOREACH var IN expr. -- Itagaki Takahiro -- 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] REVIEW: WIP: plpgsql - foreach in
On Mon, Jan 24, 2011 at 20:10, Pavel Stehule pavel.steh...@gmail.com wrote: we have to iterate over array's items because it allow seq. access to array's data. I need a global index for function array_get_isnull. I can't to use a buildin functions like array_slize_size or array_get_slice, because there is high overhead of array_seek function. I redesigned the initial part of main cycle, but code is little bit longer :(, but I hope, it is more readable. The attached patch only includes changes in plpgsql. I tested it combined with the previous one, that includes other directories. * src/pl/plpgsql/src/gram.y | for_variable K_SLICE ICONST The parameter for SLICE must be an integer literal. Is it a design? I got a syntax error when I wrote a function like below. However, FOREACH returns different types on SLICE = 0 or SLICE = 1. (element type for 0, or array type for =1) So, we cannot write a true generic function that accepts all SLICE values even if the syntax supports an expr for the parameter. CREATE FUNCTION foreach_test(int[], int) RETURNS SETOF int[] AS $$ DECLARE a integer[]; BEGIN FOREACH a SLICE $2 IN $1 LOOP -- syntax error RETURN NEXT a; END LOOP; END; $$ LANGUAGE plpgsql; * /doc/src/sgml/plpgsql.sgml + FOREACH replaceabletarget/replaceable optional SCALE s/SCALE/SLICE/ ? * Why do you use foreach_a for some identifiers? We use foreach only for arrays, so _a suffix doesn't seem needed. * accumArrayResult() for SLICE has a bit overhead. If we want to optimize it, we could use memcpy() because slices are placed in continuous memory. But I'm not sure the worth; I guess FOREACH will be used with SLICE = 0 in many cases. -- Itagaki Takahiro -- 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] REVIEW: WIP: plpgsql - foreach in
2011/1/26 Itagaki Takahiro itagaki.takah...@gmail.com: On Mon, Jan 24, 2011 at 20:10, Pavel Stehule pavel.steh...@gmail.com wrote: we have to iterate over array's items because it allow seq. access to array's data. I need a global index for function array_get_isnull. I can't to use a buildin functions like array_slize_size or array_get_slice, because there is high overhead of array_seek function. I redesigned the initial part of main cycle, but code is little bit longer :(, but I hope, it is more readable. The attached patch only includes changes in plpgsql. I tested it combined with the previous one, that includes other directories. * src/pl/plpgsql/src/gram.y | for_variable K_SLICE ICONST The parameter for SLICE must be an integer literal. Is it a design? I got a syntax error when I wrote a function like below. However, FOREACH returns different types on SLICE = 0 or SLICE = 1. (element type for 0, or array type for =1) So, we cannot write a true generic function that accepts all SLICE values even if the syntax supports an expr for the parameter. Yes, it is design. You wrote a reason. A parametrized SLICE needs only a few lines more, but a) I really don't know a use case, b) there is significant difference between slice 0 and slice 1 CREATE FUNCTION foreach_test(int[], int) RETURNS SETOF int[] AS $$ DECLARE a integer[]; BEGIN FOREACH a SLICE $2 IN $1 LOOP -- syntax error RETURN NEXT a; END LOOP; END; $$ LANGUAGE plpgsql; * /doc/src/sgml/plpgsql.sgml + FOREACH replaceabletarget/replaceable optional SCALE s/SCALE/SLICE/ ? * Why do you use foreach_a for some identifiers? We use foreach only for arrays, so _a suffix doesn't seem needed. yes, it isn't needed. But FOREACH is a new construct, that can be used for more different iterations - maybe iterations over hstore, element's set, ... so _a means - this is implementation for arrays. * accumArrayResult() for SLICE has a bit overhead. If we want to optimize it, we could use memcpy() because slices are placed in continuous memory. But I'm not sure the worth; I guess FOREACH will be used with SLICE = 0 in many cases. I agree with you, so SLICE 0 will not be used often. accumArrayResult isn't expensive function - for non varlena types. The cutting of some array needs a redundant code to CopyArrayEls and construct_md_array. All core functions are not good for our purpose, because doesn't expect a seq array reading :(. Next - simply copy can be done only for arrays without null bitmap, else you have to do some bitmap rotations :(. So, I don't would do this optimization in this moment. It has sense for multidimensional numeric arrays, but can be solved in next version. It's last commitfest now, and I don't do this patch more complex then it is now. Regards Pavel Stehule -- Itagaki Takahiro -- 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] REVIEW: WIP: plpgsql - foreach in
2011/1/24 Stephen Frost sfr...@snowman.net:
Pavel,
* Pavel Stehule (pavel.steh...@gmail.com) wrote:
I merge your changes and little enhanced comments.
Thanks. Reviewing this further-
Why are you using 'FOREACH' here instead of just making it another
variation of 'FOR'? What is 'FOUND' set to following this? I realize
that might make the code easier, but it really doesn't seem to make much
sense to me from a usability point of view.
FOR keyword - please, look on thread about my proposal FOR-IN-ARRAY
I work with FOUND variable, because I like a consistent behave with
FOR statement. When FOUND is true after cycle, you are sure, so there
was a minimally one iteration.
There also appears to be some extra whitespace changes that aren't
necessary and a number of places where you don't follow the indentation
conventions (eg: variable definitions in exec_stmt_foreach_a()).
I am really not sure about correct indentation of variables :(, if you
know a correct number of spaces, please, fix it.
I'm still not really thrilled with how the checking for scalar vs.
array, etc, is handled. Additionally, what is this? :
else if (stmt-row != NULL)
{
ctrl_var = estate-datums[stmt-row-dno];
}
else
{
ctrl_var = estate-datums[stmt-rec-dno];
}
PLpgSQL distinct between vars, row and record values. These structures
can be different and information about variable's offset in frame can
be on different position in structure. This IF means:
1) get offset of target variable
2) get addr, where is target variable saved in current frame
one note: a scalar or array can be saved on var type, only scalar can
be used on row or record type. This is often used pattern - you can
see it more time in executor.
Other comments- I don't like using 'i' and 'j', you really should use
better variable names, especially in large loops which contain other
loops. I'd also suggest changing the outer loop to be equivilant to the
number of iterations that will be done instead of the number of items
and then to *not* update 'i' inside the inner-loop. That structure is
really just confusing, imv (I certainly didn't entirely follow what was
happening there the first time I read it). Isn't there a function you
could use to pull out the array slice you need on each iteration through
the array?
I don't know a better short index identifiers than I used. But I am
not against to change.
I'll try to redesign main cycle.
Regards
Pavel Stehule
Thanks,
Stephen
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
iEYEARECAAYFAk086MEACgkQrzgMPqB3kigCzQCffx0iVSMjU2UbOgAOaY/MvtOp
iKsAnA5tdhKxTssdXJ+Rda4qkhNVm26g
=Yn5O
-END PGP SIGNATURE-
--
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] REVIEW: WIP: plpgsql - foreach in
Hello
Other comments- I don't like using 'i' and 'j', you really should use
better variable names, especially in large loops which contain other
loops. I'd also suggest changing the outer loop to be equivilant to the
number of iterations that will be done instead of the number of items
and then to *not* update 'i' inside the inner-loop. That structure is
really just confusing, imv (I certainly didn't entirely follow what was
happening there the first time I read it). Isn't there a function you
could use to pull out the array slice you need on each iteration through
the array?
I looked on code again. There are a few results:
I'll change identifiers 'i' and 'j' with any, that you send. It's
usual identifiers for nested loops and in this case they has really
well known semantic - it's subscript of array.
But others changes are more difficult
we have to iterate over array's items because it allow seq. access to
array's data. I need a global index for function array_get_isnull. I
can't to use a buildin functions like array_slize_size or
array_get_slice, because there is high overhead of array_seek
function. I redesigned the initial part of main cycle, but code is
little bit longer :(, but I hope, it is more readable.
Regards
Pavel
I don't know a better short index identifiers than I used. But I am
not against to change.
I'll try to redesign main cycle.
Regards
Pavel Stehule
Thanks,
Stephen
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
iEYEARECAAYFAk086MEACgkQrzgMPqB3kigCzQCffx0iVSMjU2UbOgAOaY/MvtOp
iKsAnA5tdhKxTssdXJ+Rda4qkhNVm26g
=Yn5O
-END PGP SIGNATURE-
*** ./gram.y.orig 2011-01-16 14:18:59.0 +0100
--- ./gram.y 2011-01-21 21:35:21.0 +0100
***
*** 21,26
--- 21,27
#include parser/parse_type.h
#include parser/scanner.h
#include parser/scansup.h
+ #include utils/array.h
/* Location tracking support --- simpler than bison's default */
***
*** 134,139
--- 135,141
PLpgSQL_datum *scalar;
PLpgSQL_rec *rec;
PLpgSQL_row *row;
+ int slice;
} forvariable;
struct
{
***
*** 178,184
%type ival assign_var
%type var cursor_variable
%type datum decl_cursor_arg
! %type forvariable for_variable
%type stmt for_control
%type str any_identifier opt_block_label opt_label
--- 180,186
%type ival assign_var
%type var cursor_variable
%type datum decl_cursor_arg
! %type forvariable for_variable foreach_control
%type stmt for_control
%type str any_identifier opt_block_label opt_label
***
*** 190,196
%type stmt stmt_return stmt_raise stmt_execsql
%type stmt stmt_dynexecute stmt_for stmt_perform stmt_getdiag
%type stmt stmt_open stmt_fetch stmt_move stmt_close stmt_null
! %type stmt stmt_case
%type list proc_exceptions
%type exception_block exception_sect
--- 192,198
%type stmt stmt_return stmt_raise stmt_execsql
%type stmt stmt_dynexecute stmt_for stmt_perform stmt_getdiag
%type stmt stmt_open stmt_fetch stmt_move stmt_close stmt_null
! %type stmt stmt_case stmt_foreach_a
%type list proc_exceptions
%type exception_block exception_sect
***
*** 264,269
--- 266,272
%token keyword K_FETCH
%token keyword K_FIRST
%token keyword K_FOR
+ %token keyword K_FOREACH
%token keyword K_FORWARD
%token keyword K_FROM
%token keyword K_GET
***
*** 298,303
--- 301,307
%token keyword K_ROWTYPE
%token keyword K_ROW_COUNT
%token keyword K_SCROLL
+ %token keyword K_SLICE
%token keyword K_SQLSTATE
%token keyword K_STRICT
%token keyword K_THEN
***
*** 739,744
--- 743,750
{ $$ = $1; }
| stmt_for
{ $$ = $1; }
+ | stmt_foreach_a
+ { $$ = $1; }
| stmt_exit
{ $$ = $1; }
| stmt_return
***
*** 1386,1391
--- 1392,1455
}
;
+ stmt_foreach_a : opt_block_label K_FOREACH foreach_control K_IN expr_until_loop loop_body
+ {
+ PLpgSQL_stmt_foreach_a *new = palloc0(sizeof(PLpgSQL_stmt_foreach_a));
+ new-cmd_type = PLPGSQL_STMT_FOREACH_A;
+ new-lineno = plpgsql_location_to_lineno(@2);
+ new-label = $1;
+ new-expr = $5;
+ new-slice = $3.slice;
+ new-body = $6.stmts;
+
+ if ($3.rec)
+ {
+ new-rec = $3.rec;
+ check_assignable((PLpgSQL_datum *) new-rec, @3);
+ }
+ else if ($3.row)
+ {
+ new-row = $3.row;
+ check_assignable((PLpgSQL_datum *) new-row, @3);
+ }
+ else if ($3.scalar)
+ {
+ Assert($3.scalar-dtype == PLPGSQL_DTYPE_VAR);
+ new-var = (PLpgSQL_var *) $3.scalar;
+ check_assignable($3.scalar, @3);
+
+ }
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg(loop variable of loop over array
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
2011/1/24 Pavel Stehule pavel.steh...@gmail.com: Hello Other comments- I don't like using 'i' and 'j', you really should use better variable names, especially in large loops which contain other loops. I'd also suggest changing the outer loop to be equivilant to the number of iterations that will be done instead of the number of items and then to *not* update 'i' inside the inner-loop. That structure is really just confusing, imv (I certainly didn't entirely follow what was happening there the first time I read it). Isn't there a function you could use to pull out the array slice you need on each iteration through the array? I looked on code again. There are a few results: I'll change identifiers 'i' and 'j' with any, that you send. It's usual identifiers for nested loops and in this case they has really well known semantic - it's subscript of array. But others changes are more difficult we have to iterate over array's items because it allow seq. access to array's data. I need a global index for function array_get_isnull. I can't to use a buildin functions like array_slize_size or array_get_slice, because there is high overhead of array_seek function. I redesigned the initial part of main cycle, but code is little bit longer :(, but I hope, it is more readable. btw, having array_get_isnul accessible from contrib code is nice (I used to copy/paste it for my own needs) Regards Pavel I don't know a better short index identifiers than I used. But I am not against to change. I'll try to redesign main cycle. Regards Pavel Stehule Thanks, Stephen -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) iEYEARECAAYFAk086MEACgkQrzgMPqB3kigCzQCffx0iVSMjU2UbOgAOaY/MvtOp iKsAnA5tdhKxTssdXJ+Rda4qkhNVm26g =Yn5O -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support -- 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] REVIEW: WIP: plpgsql - foreach in
Pavel,
* Pavel Stehule (pavel.steh...@gmail.com) wrote:
I merge your changes and little enhanced comments.
Thanks. Reviewing this further-
Why are you using 'FOREACH' here instead of just making it another
variation of 'FOR'? What is 'FOUND' set to following this? I realize
that might make the code easier, but it really doesn't seem to make much
sense to me from a usability point of view.
There also appears to be some extra whitespace changes that aren't
necessary and a number of places where you don't follow the indentation
conventions (eg: variable definitions in exec_stmt_foreach_a()).
I'm still not really thrilled with how the checking for scalar vs.
array, etc, is handled. Additionally, what is this? :
else if (stmt-row != NULL)
{
ctrl_var = estate-datums[stmt-row-dno];
}
else
{
ctrl_var = estate-datums[stmt-rec-dno];
}
Other comments- I don't like using 'i' and 'j', you really should use
better variable names, especially in large loops which contain other
loops. I'd also suggest changing the outer loop to be equivilant to the
number of iterations that will be done instead of the number of items
and then to *not* update 'i' inside the inner-loop. That structure is
really just confusing, imv (I certainly didn't entirely follow what was
happening there the first time I read it). Isn't there a function you
could use to pull out the array slice you need on each iteration through
the array?
Thanks,
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
On Sun, Jan 23, 2011 at 9:49 PM, Stephen Frost sfr...@snowman.net wrote: Pavel, * Pavel Stehule (pavel.steh...@gmail.com) wrote: I merge your changes and little enhanced comments. Thanks. Reviewing this further- Why are you using 'FOREACH' here instead of just making it another variation of 'FOR'? Uh oh. You just reopened the can of worms from hell. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] REVIEW: WIP: plpgsql - foreach in
* Robert Haas (robertmh...@gmail.com) wrote: On Sun, Jan 23, 2011 at 9:49 PM, Stephen Frost sfr...@snowman.net wrote: Why are you using 'FOREACH' here instead of just making it another variation of 'FOR'? Uh oh. You just reopened the can of worms from hell. hahahaha. Apparently I missed that discussion; also wasn't linked off the patch. :/ Guess I'll go poke through the archives... Struck me as obviously wrong to invent something completely new for this, but.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
* Robert Haas (robertmh...@gmail.com) wrote: Uh oh. You just reopened the can of worms from hell. Alright.. I'm missing what happened to this suggestion of using: FOR var in ARRAY array_expression ... I like that a lot more than inventing a new top-level keyword, for the same reasons that Tom mentioned: using a different initial keyword makes it awkward to make generic statements about all types of FOR loop (as I noticed while looking through the documentation changes that should be made for this); and also some of the other comments about how FOREACH doesn't give you any clue that this is some array-specific-FOR-loop-thingy. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
Hello
I merge your changes and little enhanced comments.
Regards
Pavel Stehule
2011/1/20 Stephen Frost sfr...@snowman.net:
Greetings,
* Pavel Stehule (pavel.steh...@gmail.com) wrote:
attached patch contains a implementation of iteration over a array:
I've gone through this patch and, in general, it looks pretty reasonable
to me. There's a number of places where I think additional comments
would be good and maybe some variable name improvments. Also, my
changes should be reviewed to make sure they make sense.
Attached is a patch against master which includes my changes, and a
patch against Pavel's patch, so he can more easily see my changes and
include them if he'd like.
I'm going to mark this returned to author with feedback.
commit 30295015739930e68c33b29da4f7ef535bc293ea
Author: Stephen Frost sfr...@snowman.net
Date: Wed Jan 19 17:58:24 2011 -0500
Clean up foreach-in-array PL/PgSQL code/comments
Minor clean-up of the PL/PgSQL foreach-in-array patch, includes
some white-space cleanup, grammar fixes, additional errhint where
it makes sense, etc.
Also added a number of 'XXX' comments asking for clarification
and additional comments on what's happening in the code.
commit f1a02fe3a8fa84217dae32d5ba74e9764c77431c
Author: Stephen Frost sfr...@snowman.net
Date: Wed Jan 19 15:11:53 2011 -0500
PL/PgSQL - Add interate-over-array support
This patch adds support for iterating over an array in PL/PgSQL.
Patch Author: Pavel Stehule
Thanks,
Stephen
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
iEYEARECAAYFAk03bf8ACgkQrzgMPqB3kihxuwCfZYKFpEraRCIltlUeYtD9AyX0
tvoAnjuxddXhZB6w2/V9oVSD1+K7Idu9
=w38Z
-END PGP SIGNATURE-
*** ./doc/src/sgml/plpgsql.sgml.orig 2011-01-16 14:18:58.0 +0100
--- ./doc/src/sgml/plpgsql.sgml 2011-01-17 11:31:54.086217514 +0100
***
*** 2238,2243
--- 2238,2268
/para
/sect2
+sect2 id=plpgsql-array-iterating
+ titleLooping Through Array/title
+ synopsis
+ optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
+ FOREACH replaceabletarget/replaceable optional SCALE replaceablenumber/replaceable /optional IN replaceableexpression/replaceable LOOP
+ replaceablestatements/replaceable
+ END LOOP optional replaceablelabel/replaceable /optional;
+ /synopsis
+
+ programlisting
+ CREATE FUNCTION sum(VARIADIC int[]) RETURNS int8 AS $$
+ DECLARE
+ s int8; x int;
+ BEGIN
+ FOREACH x IN $1
+ LOOP
+ s := s + x;
+ END LOOP;
+ RETURN s;
+ END;
+ $$ LANGUAGE plpgsql;
+ /programlisting
+
+/sect2
+
sect2 id=plpgsql-error-trapping
titleTrapping Errors/title
*** ./src/backend/utils/adt/arrayfuncs.c.orig 2011-01-16 14:18:58.0 +0100
--- ./src/backend/utils/adt/arrayfuncs.c 2011-01-21 22:59:00.315522289 +0100
***
*** 68,74
Datum *values, bool *nulls, int nitems,
int typlen, bool typbyval, char typalign,
bool freedata);
- static bool array_get_isnull(const bits8 *nullbitmap, int offset);
static void array_set_isnull(bits8 *nullbitmap, int offset, bool isNull);
static Datum ArrayCast(char *value, bool byval, int len);
static int ArrayCastAndSet(Datum src,
--- 68,73
***
*** 3836,3842
* nullbitmap: pointer to array's null bitmap (NULL if none)
* offset: 0-based linear element number of array element
*/
! static bool
array_get_isnull(const bits8 *nullbitmap, int offset)
{
if (nullbitmap == NULL)
--- 3835,3841
* nullbitmap: pointer to array's null bitmap (NULL if none)
* offset: 0-based linear element number of array element
*/
! bool
array_get_isnull(const bits8 *nullbitmap, int offset)
{
if (nullbitmap == NULL)
*** ./src/include/utils/array.h.orig 2011-01-16 14:18:59.0 +0100
--- ./src/include/utils/array.h 2011-01-21 23:03:07.294898398 +0100
***
*** 215,220
--- 215,221
extern ArrayType *array_set(ArrayType *array, int nSubscripts, int *indx,
Datum dataValue, bool isNull,
int arraytyplen, int elmlen, bool elmbyval, char elmalign);
+ extern bool array_get_isnull(const bits8 *nullbitmap, int offset);
extern ArrayType *array_get_slice(ArrayType *array, int nSubscripts,
int *upperIndx, int *lowerIndx,
int arraytyplen, int elmlen, bool elmbyval, char elmalign);
*** ./src/pl/plpgsql/src/gram.y.orig 2011-01-16 14:18:59.0 +0100
--- ./src/pl/plpgsql/src/gram.y 2011-01-21 21:35:21.959579809 +0100
***
*** 21,26
--- 21,27
#include parser/parse_type.h
#include parser/scanner.h
#include parser/scansup.h
+ #include utils/array.h
/* Location tracking support --- simpler than bison's default */
***
*** 134,139
--- 135,141
PLpgSQL_datum *scalar;
PLpgSQL_rec *rec;
PLpgSQL_row *row;
+ int slice;
} forvariable;
struct
{
***
*** 178,184
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
2011/1/20 Stephen Frost sfr...@snowman.net: * Robert Haas (robertmh...@gmail.com) wrote: On Wed, Jan 19, 2011 at 6:04 PM, Stephen Frost sfr...@snowman.net wrote: I'm going to mark this returned to author with feedback. That implies you don't think it should be considered further for this CommitFest. Perhaps you mean Waiting on Author? I did, actually, and that's what I actually marked it as in the CF. Sorry for any confusion. When I went to mark it in CF, I realized my mistake. ok :), I'll look on it tomorrow. regards Pavel Thanks, Stephen -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) iEYEARECAAYFAk03ltkACgkQrzgMPqB3kihSmQCePy6+fpC7RJdki5guPRCLp5IZ EJMAoIqgjb+IsG853/gC9T9xgFg5M5aM =VLWh -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] REVIEW: WIP: plpgsql - foreach in
Greetings,
* Pavel Stehule (pavel.steh...@gmail.com) wrote:
attached patch contains a implementation of iteration over a array:
I've gone through this patch and, in general, it looks pretty reasonable
to me. There's a number of places where I think additional comments
would be good and maybe some variable name improvments. Also, my
changes should be reviewed to make sure they make sense.
Attached is a patch against master which includes my changes, and a
patch against Pavel's patch, so he can more easily see my changes and
include them if he'd like.
I'm going to mark this returned to author with feedback.
commit 30295015739930e68c33b29da4f7ef535bc293ea
Author: Stephen Frost sfr...@snowman.net
Date: Wed Jan 19 17:58:24 2011 -0500
Clean up foreach-in-array PL/PgSQL code/comments
Minor clean-up of the PL/PgSQL foreach-in-array patch, includes
some white-space cleanup, grammar fixes, additional errhint where
it makes sense, etc.
Also added a number of 'XXX' comments asking for clarification
and additional comments on what's happening in the code.
commit f1a02fe3a8fa84217dae32d5ba74e9764c77431c
Author: Stephen Frost sfr...@snowman.net
Date: Wed Jan 19 15:11:53 2011 -0500
PL/PgSQL - Add interate-over-array support
This patch adds support for iterating over an array in PL/PgSQL.
Patch Author: Pavel Stehule
Thanks,
Stephen
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
***
*** 2238,2243 END LOOP optional replaceablelabel/replaceable /optional;
--- 2238,2268
/para
/sect2
+sect2 id=plpgsql-array-iterating
+ titleLooping Through Array/title
+ synopsis
+ optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
+ FOREACH replaceabletarget/replaceable optional SCALE replaceablenumber/replaceable /optional IN replaceableexpression/replaceable LOOP
+ replaceablestatements/replaceable
+ END LOOP optional replaceablelabel/replaceable /optional;
+ /synopsis
+
+ programlisting
+ CREATE FUNCTION sum(VARIADIC int[]) RETURNS int8 AS $$
+ DECLARE
+ s int8; x int;
+ BEGIN
+ FOREACH x IN $1
+ LOOP
+ s := s + x;
+ END LOOP;
+ RETURN s;
+ END;
+ $$ LANGUAGE plpgsql;
+ /programlisting
+
+/sect2
+
sect2 id=plpgsql-error-trapping
titleTrapping Errors/title
*** a/src/pl/plpgsql/src/gram.y
--- b/src/pl/plpgsql/src/gram.y
***
*** 21,26
--- 21,27
#include parser/parse_type.h
#include parser/scanner.h
#include parser/scansup.h
+ #include utils/array.h
/* Location tracking support --- simpler than bison's default */
***
*** 134,139 static List *read_raise_options(void);
--- 135,141
PLpgSQL_datum *scalar;
PLpgSQL_rec *rec;
PLpgSQL_row *row;
+ int slice;
} forvariable;
struct
{
***
*** 178,184 static List *read_raise_options(void);
%type ival assign_var
%type var cursor_variable
%type datum decl_cursor_arg
! %type forvariable for_variable
%type stmt for_control
%type str any_identifier opt_block_label opt_label
--- 180,186
%type ival assign_var
%type var cursor_variable
%type datum decl_cursor_arg
! %type forvariable for_variable foreach_control
%type stmt for_control
%type str any_identifier opt_block_label opt_label
***
*** 190,196 static List *read_raise_options(void);
%type stmt stmt_return stmt_raise stmt_execsql
%type stmt stmt_dynexecute stmt_for stmt_perform stmt_getdiag
%type stmt stmt_open stmt_fetch stmt_move stmt_close stmt_null
! %type stmt stmt_case
%type list proc_exceptions
%type exception_block exception_sect
--- 192,198
%type stmt stmt_return stmt_raise stmt_execsql
%type stmt stmt_dynexecute stmt_for stmt_perform stmt_getdiag
%type stmt stmt_open stmt_fetch stmt_move stmt_close stmt_null
! %type stmt stmt_case stmt_foreach_a
%type list proc_exceptions
%type exception_block exception_sect
***
*** 264,269 static List *read_raise_options(void);
--- 266,272
%token keyword K_FETCH
%token keyword K_FIRST
%token keyword K_FOR
+ %token keyword K_FOREACH
%token keyword K_FORWARD
%token keyword K_FROM
%token keyword K_GET
***
*** 298,303 static List *read_raise_options(void);
--- 301,307
%token keyword K_ROWTYPE
%token keyword K_ROW_COUNT
%token keyword K_SCROLL
+ %token keyword K_SLICE
%token keyword K_SQLSTATE
%token keyword K_STRICT
%token keyword K_THEN
***
*** 739,744 proc_stmt : pl_block ';'
--- 743,750
{ $$ = $1; }
| stmt_for
{ $$ = $1; }
+ | stmt_foreach_a
+ { $$ = $1; }
| stmt_exit
{ $$ = $1; }
| stmt_return
***
*** 1386,1391 for_variable : T_DATUM
--- 1392,1455
}
;
+ stmt_foreach_a : opt_block_label K_FOREACH foreach_control K_IN
Re: [HACKERS] REVIEW: WIP: plpgsql - foreach in
On Wed, Jan 19, 2011 at 6:04 PM, Stephen Frost sfr...@snowman.net wrote: I'm going to mark this returned to author with feedback. That implies you don't think it should be considered further for this CommitFest. Perhaps you mean Waiting on Author? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] REVIEW: WIP: plpgsql - foreach in
* Robert Haas (robertmh...@gmail.com) wrote: On Wed, Jan 19, 2011 at 6:04 PM, Stephen Frost sfr...@snowman.net wrote: I'm going to mark this returned to author with feedback. That implies you don't think it should be considered further for this CommitFest. Perhaps you mean Waiting on Author? I did, actually, and that's what I actually marked it as in the CF. Sorry for any confusion. When I went to mark it in CF, I realized my mistake. Thanks, Stephen signature.asc Description: Digital signature
