Paul Jungwirth <p...@illuminatedcomputing.com> writes:
> I tried a few refactoring approaches but the nicest seemed to be to keep the
> shared parts in
> inline_set_returning_function, but have it call out to either
> inline_sql_set_returning_function or
> inline_set_returning_function_with_support. The first patch just refactors
> but doesn't yet add
> inline_set_returning_function_with_support, then the second patch adds the
> new functionality.
I got around to looking at this again. I generally agree with your
approach to the refactoring in clauses.c, with minor nitpicks:
* I don't like postponing the early exit for its-not-a-SELECT;
as coded, this wastes a pretty decent number of cycles transforming
a querytree that won't be used (not to mention that I'm not sure
that our usage of check_sql_fn_retval won't fail on a non-SELECT).
So I think we should keep this bit where it is:
- /*
- * The single command must be a plain SELECT.
- */
- if (!IsA(querytree, Query) ||
- querytree->commandType != CMD_SELECT)
- goto fail;
and then in the other path, simply Assert that those two conditions
hold for anything the support function might try to give back.
* I'm inclined to think that the test for "it must be declared to
return a set" should stay in inline_sql_set_returning_function.
In the case of a support-function-supported function, it's okay either
if the function returns a set or if it is guaranteed to return exactly
one row (including edge cases such as null function arguments).
The support function either knows that already or can take the
responsibility for checking it. But if we do it like this, we
foreclose the possibility of supporting the latter class of functions.
* But on the other hand, I wonder if this bit shouldn't move to
the outer function:
/*
* Refuse to inline if the arguments contain any volatile functions or
* sub-selects. Volatile functions are rejected because inlining may
* result in the arguments being evaluated multiple times, risking a
* change in behavior. Sub-selects are rejected partly for implementation
* reasons (pushing them down another level might change their behavior)
* and partly because they're likely to be expensive and so multiple
* evaluation would be bad.
*/
if (contain_volatile_functions((Node *) fexpr->args) ||
contain_subplans((Node *) fexpr->args))
return NULL;
I am not really convinced that any support function could safely
ignore those restrictions, and I do fear that a lot would omit the
enforcement and thereby produce wrong queries in such cases. Another
thing that likely needs to be in the outer wrapper is the check that
pg_proc_proconfig is empty, because that doesn't seem like a case
that support functions could skip over either.
* I don't like the memory management. I think creation/destruction
of the temp context should occur at the outer level, and in particular
that we want to perform substitute_actual_srf_parameters() while still
working in the temp context, and copy out only the final form of the
query tree. This addresses your XXX comment in v2-0002, and also
saves support functions from having to re-invent that wheel.
> I didn't love passing a SysCache HeapTuple into another function,
No, that's perfectly common; see for example
prepare_sql_fn_parse_info. In fact, one thing I don't like in v2-0002
is that you should pass the pg_proc entry to the support function as a
HeapTuple not Form_pg_proc. It's possible to get the Form_pg_proc
pointer from the HeapTuple but not vice versa, while the Form_pg_proc
does not allow access to varlena fields, which makes it useless for
many cases. Even your own example function is forced to re-fetch
the syscache entry because of this.
One other comment on v2-0002 is that this bit doesn't look right:
+ /* Get filter if present */
+ node = lthird(expr->args);
+ if (!(IsA(node, Const) && ((Const *) node)->constisnull))
+ {
+ appendStringInfo(&sql, " WHERE %s::text = $3",
quote_identifier(colname));
+ }
It's not actually doing anything with the "node" value.
Backing up to a higher level, it seems like we still have no answer
for how to build a valid support function result besides "construct an
equivalent SQL query string and feed it through parse analysis and
rewrite". That seems both restrictive and expensive. In particular
it begs the question of why the target function couldn't just have
been written as a SQL function to begin with. So I still have kind
of a low estimate of this feature's usefulness. Is there a way to
do better?
regards, tom lane
