Re: Set Returning Functions (SRF) - request for patch review

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