Обсуждение: Inline non-SQL SRFs using SupportRequestSimplify

Hi Hackers,

Here is a proof-of-concept patch to inline set-returning functions (SRFs) besides those written in 
SQL. We already try to inline SQL-language functions,[1] but that means you must have a static SQL 
query. There is no way to get an inline-able query by dynamically building the sql in, say, plpgsql.

We also have a SupportRequestSimplify request type for functions that use SUPPORT to declare a 
support function, and it can replace the FuncExpr with an arbitrary nodetree.[2] I think this was 
intended for constant-substitution, but we can also use it to let functions generate dynamic SQL and 
then inline it. In this patch, if a SRF replaces itself with a Query node, then 
inline_set_returning_function will use that.

So far there are no tests or docs; I'm hoping to hear feedback on the idea before going further.

Here is my concrete use-case: I wrote a function to do a temporal semijoin,[3] and I want it to be 
inlined. There is a support function that builds the same SQL and lets Postgres parse it into a 
Query.[4] (In practice I would rewrite the main function in C too, so it could share the 
SQL-building code there, but this is just a POC.) If you build and install that extension on its 
`inlined` branch,[5] then you can do this:

```
\i bench.sql
explain select * from temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id', 
'valid_at') j(id bigint, valid_at daterange);
explain select * from temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id', 
'valid_at') j(id bigint, valid_at daterange) where j.id = 10::bigint;
```

Without this patch, you get `ERROR:  unrecognized node type: 58`. But with this patch you get these 
plans:

```
postgres=# explain select * from temporal_semijoin('employees', 'id', 'valid_at', 'positions', 
'employee_id', 'valid_at') j(id bigint, valid_at daterange);
                                           QUERY PLAN
----------------------------------------------------------------------------------------------
  ProjectSet  (cost=4918.47..6177.06 rows=22300 width=40)
    ->  Hash Join  (cost=4918.47..6062.77 rows=223 width=53)
          Hash Cond: (employees.id = j.employee_id)
          Join Filter: (employees.valid_at && j.valid_at)
          ->  Seq Scan on employees  (cost=0.00..1027.39 rows=44539 width=21)
          ->  Hash  (cost=4799.15..4799.15 rows=9545 width=40)
                ->  Subquery Scan on j  (cost=4067.61..4799.15 rows=9545 width=40)
                      ->  HashAggregate  (cost=4067.61..4703.70 rows=9545 width=40)
                            Group Key: positions.employee_id
                            Planned Partitions: 16
                            ->  Seq Scan on positions  (cost=0.00..897.99 rows=44099 width=21)
(11 rows)

postgres=# explain select * from temporal_semijoin('employees', 'id', 'valid_at', 'positions', 
'employee_id', 'valid_at') j(id bigint, valid_at daterange) where j.id = 10::bigint;
                                                       QUERY PLAN 

----------------------------------------------------------------------------------------------------------------------
  ProjectSet  (cost=0.56..9.22 rows=100 width=40)
    ->  Nested Loop  (cost=0.56..8.71 rows=1 width=53)
          ->  GroupAggregate  (cost=0.28..4.39 rows=1 width=40)
                ->  Index Only Scan using idx_positions_on_employee_id on positions 
(cost=0.28..4.36 rows=5 width=21)
                      Index Cond: (employee_id = '10'::bigint)
          ->  Index Only Scan using employees_pkey on employees  (cost=0.28..4.30 rows=1 width=21)
                Index Cond: ((id = '10'::bigint) AND (valid_at && (range_agg(positions.valid_at))))
(7 rows)

```

In particular I'm excited to see in the second plan that the predicate gets pushed into the subquery.

If it seems good to let people use SupportRequestSimplify to make their SRFs be inlineable, I'm 
happy to add tests and docs. We should really document the idea of inlined functions in general, so 
I'll do that too.

Another approach I considered is using a separate support request, e.g. SupportRequestInlineSRF, and 
just calling it from inline_set_returning_function. I didn't like having two support requests that 
did almost exactly the same thing. OTOH my current approach means you'll get an error if you do this:

```
postgres=# select temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id', 
'valid_at');
ERROR:  unrecognized node type: 66
```

I'll look into ways to fix that.

I think SupportRequestSimplify is a really cool feature. It is nearly like having macros.
I'm dreaming about other ways I can (ab)use it. Just making inline-able SRFs has many applications. 
 From my own client work, I could use this for a big permissions query or a query with complicated 
pricing logic.

The sad part though is that SUPPORT functions must be written in C. That means few people will use 
them, especially these days when so many are in the cloud. Since they take a Node and return a Node, 
maybe there is no other way. But I would love to have a different mechanism that receives the 
function's arguments (evaluated) and returns a string, which we parse as a SQL query and then 
inline. The arguments would have to be const-reducible to strings, of course. You could specify that 
function with a new INLINE keyword when you create your target function. That feature would be less 
powerful, but with broader reach.

I'd be glad to hear your thoughts!

[1] https://wiki.postgresql.org/wiki/Inlining_of_SQL_functions (I couldn't find any mention in our 
docs though, so we should add that.)
[2] https://www.postgresql.org/docs/current/xfunc-optimization.html
[3] https://github.com/pjungwir/temporal_ops/blob/master/temporal_ops--1.0.0.sql
[4] https://github.com/pjungwir/temporal_ops/blob/inlined/temporal_ops.c
[5] https://github.com/pjungwir/temporal_ops/tree/inlined

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

On 28/06/2024 01:01, Paul Jungwirth wrote:
> If it seems good to let people use SupportRequestSimplify to make their SRFs be inlineable, I'm
> happy to add tests and docs. We should really document the idea of inlined functions in general, so
> I'll do that too.
>
> Another approach I considered is using a separate support request, e.g. SupportRequestInlineSRF, and
> just calling it from inline_set_returning_function. I didn't like having two support requests that
> did almost exactly the same thing. OTOH my current approach means you'll get an error if you do this:
> 
> ```
> postgres=# select temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id',
> 'valid_at');
> ERROR:  unrecognized node type: 66
> ```
> 
> I'll look into ways to fix that.

If the support function returns a Query, we end up having a FuncExpr 
with a Query in the tree. A Query isnt an Expr, which is why you get 
that error, and it seems like a recipe for confusion in general. Perhaps 
returning a SubLink would be better.

I think we should actually add an assertion after the call to the 
SupportRequestSimplify support function, to check that it returned an 
Expr node.

+1 to the general feature of letting SRFs be simplified by the support 
function.

> I think SupportRequestSimplify is a really cool feature. It is nearly like having macros.
> I'm dreaming about other ways I can (ab)use it.

:-D

-- 
Heikki Linnakangas
Neon (https://neon.tech)

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 28/06/2024 01:01, Paul Jungwirth wrote:
>> Another approach I considered is using a separate support request, e.g. SupportRequestInlineSRF, and
>> just calling it from inline_set_returning_function. I didn't like having two support requests that
>> did almost exactly the same thing. OTOH my current approach means you'll get an error if you do this:
>>
>> ```
>> postgres=# select temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id',
>> 'valid_at');
>> ERROR:  unrecognized node type: 66
>> ```
>>
>> I'll look into ways to fix that.

I like this idea, but I like exactly nothing about this implementation.
The right thing is to have a separate SupportRequestInlineSRF request
that is called directly by inline_set_returning_function.  It might be
"almost the same thing" as SupportRequestSimplify, but "almost" only
counts in horseshoes and hand grenades.  In particular, returning a
Query node is simply broken for SupportRequestSimplify (as your
example demonstrates), whereas it's the only correct result for
SupportRequestInlineSRF.

You could imagine keeping it to one support request by adding a
boolean field to the request struct to show which behavior is wanted,
but I think the principal result of that would be to break extensions
that weren't expecting such calls.  The defined mechanism for
extending the SupportRequest protocol is to add new support request
codes, not to whack around the APIs of existing ones.

> I think we should actually add an assertion after the call to the
> SupportRequestSimplify support function, to check that it returned an
> Expr node.

Um ... IsA(node, Expr) isn't going to work, and I'm not sure that
it'd be useful to try to enumerate the set of Expr subtypes that
should be allowed there.  But possibly it'd be worth asserting that
it's not a Query, just in case anyone gets confused about the
difference between SupportRequestSimplify and SupportRequestInlineSRF.

It would be good to have an in-core test case for this request type,
but I don't really see any built-in SRFs for which expansion as a
sub-SELECT would be an improvement.

            regards, tom lane

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

Paul Jungwirth <pj@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

On Mon, Jul 14, 2025 at 2:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I got around to looking at this again.  I generally agree with your
> approach to the refactoring in clauses.c, with minor nitpicks:

Thanks for taking another look! Revisions attached.

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

Okay.

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

Makes sense. Done.

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

Agreed, done.

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

Okay, done. I was trying to defer creating a memory context past as
many checks as possible. It's not an expensive thing to do?

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

Okay, thanks for explaining! Done.

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

This is correct, but I added a comment. The idea is that $3 will get
the value of "node".

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

Parsing the function body is no more expensive than what we'd do to
execute it separately, right? And by inlining we only do it once. But
if it was too much for someone, perhaps they could keep a cache of
node trees based on the function arguments and/or hash of the SQL
string, not unlike how foreign keys cache query plans. Or they could
build the node tree directly, without parsing. But parsing the
equivalent string is easy and covers most uses. I can even imagine a
way to eventually semi-automate it, e.g. creating a general-purpose
support function that you can attach to (many) PL/pgSQL functions that
end in `EXECUTE format(...)`.

The reason for supporting more than SQL functions is to let you
construct the query dynamically, e.g. with user-supplied table/column
names, or to only include some expensive filters if needed. This would
be great for building functions that implement temporal
outer/semi/antijoin. Another use-case I personally have, which I think
is quite common, is building "parameterized views" for permissions
checks, e.g. visible_sales(user). In that case we may only need to
include certain joins if the user belongs to certain roles (e.g. a
third-party sales rep).

Rebased to 04b7ff3cd3.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com