Re: Set Returning Functions (SRF) - request for patch review and comment - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: Set Returning Functions (SRF) - request for patch review and comment |
Date | |
Msg-id | 24349.1020788529@sss.pgh.pa.us Whole thread Raw |
In response to | Set Returning Functions (SRF) - request for patch review and comment (Joe Conway <mail@joeconway.com>) |
List | pgsql-hackers |
Joe Conway <mail@joeconway.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. Considerselect ... 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
pgsql-hackers by date: