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

Joe Conway <mail(at)joeconway(dot)com> writes:
> I've been buried in the backend parser/planner/executor now for the last
> 2 weeks or so, and I now have a patch for a working implementation of
> SRFs as RTEs (i.e. "SELECT tbl.* FROM myfunc() AS tbl"). I think I'm at
> a good point to get review and comments.

A few random comments ---

> 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.

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
confusion. (I think Alex started with that name because he intended
to allow fetches from cursors --- but you're not doing that, and if
someone were to add it later he'd probably want to use the name
RangePortal for that.)

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.

*** src/backend/executor/execQual.c 27 Apr 2002 03:45:03 -0000 1.91
--- src/backend/executor/execQual.c 5 May 2002 21:36:55 -0000
***************
*** 44,49 ****
--- 44,52 ----
#include "utils/fcache.h"


+ 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.

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.

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.

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.)

Good to see you making progress on this; it's been a wishlist item
for a long time.

regards, tom lane