Re: REVIEW: WIP: plpgsql - foreach in - Mailing list pgsql-hackers

2011/1/24 Stephen Frost <sfrost@snowman.net>:
> Pavel,
>
> * Pavel Stehule (pavel.stehule@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-----
>
>