Re: Inline non-SQL SRFs using SupportRequestSimplify - Mailing list pgsql-hackers

Paul Jungwirth <pj@illuminatedcomputing.com> writes:
> Here are new patches using a new SupportRequestInlineSRF request type. They include patches and
> documentation.

I took a look through this.  I feel like we're still some way away
from having something committable.  I've got two main complaint
areas:

1. It doesn't seem like integrating this into
inline_set_returning_function was the right thing after all, or
maybe just the way you did it isn't right.  That function is pretty
opinionated about what it is doing, and a lot of what it is doing
doesn't seem appropriate for a support-function-driven substitution.
As an example, it rejects WITH ORDINALITY, but who's to say that a
support function couldn't handle that?  More generally, I'm not sure
if it's appropriate to make any tests on the function's properties,
rather than assuming the support function knows what it's doing.
I see you already hacked up the test on prolang, but the others in
the same if-clause seem equally dubious from here.  I'm also unsure
whether it's our business to reject volatile functions or subplans
in the function arguments.  (Maybe it is, but not sure.)  There is
also stuff towards the bottom of the function, particularly
check_sql_fn_retval and parameter substitution, that I do not think
makes sense to apply to a non-SQL-language function; but if I'm
reading this right you run all that code on the support function's
result.

It does make sense to require there to be just one RangeTblFunction in
the RTE, since it's not at all clear how we could combine the results
if there's more than one.  But I wonder if we should just pass the RTE
node to the support function, and let it make its own decision about
rte->funcordinality.  Or if that seems like a bad idea, pass the
RangeTblFunction node.  I think it's essential to do one of those
things rather than fake up a FuncExpr, because a support function for
a function returning RECORD would likely need access to the column
definition list to figure out what to do.

I notice that in the case of non-SRF function inlining, we handle
support-function calling in a totally separate function
(simplify_function) rather than try to integrate it into the
code that does SQL function inlining (inline_function).  Maybe
a similar approach should be adopted here.  We could have a
wrapper function that implements the parts worth sharing, such
as looking up the target function's pg_proc entry and doing
the permissions check.  Or perhaps put that stuff into the sole
caller, preprocess_function_rtes.

If we do keep this in inline_set_returning_function, we need to
pay more than zero attention to updating that function's header
comment.

2. The documentation needs to be a great deal more explicit
about what the function is supposed to return.  It needs to
be a SELECT Query node that has been through parse analysis
and rewriting.  I don't think pointing to a regression test
function is adequate, or even appropriate.  The test function
is a pretty bad example as-is, too.  It aggressively disregards
the API recommendation in supportnodes.h:

 * Support functions must return a NULL pointer, not fail, if they do not
 * recognize the request node type or cannot handle the given case; this
 * allows for future extensions of the set of request cases.

As a more minor nit, I think SupportRequestInlineSRF should
include "struct PlannerInfo *root", for the same reasons that
SupportRequestSimplify does.

> I split things up into three patch files because I couldn't get git to gracefully handle shifting a
> large block of code into an if statement. The first two patches have no changes except that
> indentation (and initializing one variable to NULL). They aren't meant to be committed separately.

A hack I've used in the past is to have the main patch just add

+    if (...)
+    {
...
+    }

around the to-be-reindented code, and then apply pgindent as a
separate patch step.  (We used to just leave it to the committer to
run pgindent, but I think nowadays the cfbot will whine at you if you
submit not-pgindented code.)  I think that's easier to review since
the reviewer can mechanically verify the pgindent patch.  This problem
may be moot for this patch once we detangle the support function call
from SQL-function inlining, though.

            regards, tom lane