Re: REVIEW: WIP: plpgsql - foreach in

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: REVIEW: WIP: plpgsql - foreach in
Date: 2011-01-29 13:02:58
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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

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.



In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2011-01-29 13:05:27 Re: REVIEW: WIP: plpgsql - foreach in
Previous Message Thom Brown 2011-01-29 12:27:32 Re: Snapshots no longer build