Re: Set Returning Functions (SRF) - request for patch review - Mailing list pgsql-hackers

Tom Lane wrote:
>>4. The SRF *must* be aliased in the FROM clause. This is similar to the 
>>requirement for a subselect used in the FROM clause.
> 
> This seems unnecessary; couldn't we use the function name as the default
> alias name?  The reason we require an alias for a subselect is that
> there's no obvious automatic choice for a subselect; but there is for a
> function.

Yeah, I was on the fence about this. The only problem I could see is 
when the function returns a base type, what do I use for the column 
alias? Is it OK to use the same alias for the relation and column?


> 
> You may not want to hear this at this point ;-) but I'd be strongly
> inclined to s/portal/function/ throughout the patch.  The implementation
> doesn't seem to have anything to do with portals as defined by
> portalmem.c, so using the same name just sounds like a recipe for

I was already thinking the same thing. It will be a real PITA, but I do 
believe it is the right thing to do.


> The patch's approach to checking function execute permissions seems
> wrong, because it only looks at the topmost node of the function
> expression.  Consider
>     select ... from myfunc(1, sin(x))
> Probably better to let init_fcache do the checking instead when the
> expression is prepared for execution.

OK.


> + Datum ExecEvalFunc(Expr *funcClause, ExprContext *econtext,
> +              bool *isNull, ExprDoneCond *isDone);
> + 
> 
> (and a corresponding "extern" in some other .c file)  Naughty naughty...
> this should be in a .h file.  But actually you should probably just be
> calling ExecEvalExpr anyway, rather than hard-wiring the assumption that
> the top node of the expression tree is a Func.  Most of the other places
> that assume that could be fixed easily by using functions like
> exprType() in place of direct field access.

OK.


> 
> I've been toying with eliminating Iter nodes, which don't seem to do
> anything especially worthwhile --- it'd make a lot more sense to add
> a "returnsSet" boolean in Func nodes.  Dunno if that simplifies life
> for you.  If you take the above advice you may find you don't really
> care anymore whether there's an Iter node in the tree.

Actually it gets in my way a bit, and I think I remember some 
discussions wrt removing it. But I wasn't sure how large the impact 
would be on the current API if I messed with it, so I thought I'd leave 
it for now. Do you think it's worth it to address this now?


> 
> ExecPortalReScan does not look like it works yet (in fact, it looks like
> it will dump core).  This is important.  It also brings up the question
> of how you are handling parameters passed into the function.  I think
> there's a lot more to that than meets the eye.

Yeah, I took a first shot at writing the rescan support, but have not 
yet begun to use/test it. I'd like to get the rest of the patch to an 
acceptable level first, then concentrate on get materialization and 
rescan working.


> 
> I have been thinking that TupleStore ought to be extended to allow
> fetching of existing entries without foreclosing the option of storing
> more tuples.  This would allow you to operate "on the fly" without
> necessarily having to fetch the entire function output on first call.
> You fetch only as far as you've been requested to provide answers.
> (Which would be a good thing; consider cases with LIMIT for example.)
> 

Hmm, I'll have to look at this more closely. When I get to the 
materialization/rescan stuff, I'll see if I can address this idea.

Thanks for the review and comments!

Joe