pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
Inline non-SQL SRFs using SupportRequestSimplify
19+ messages / 6 participants
[nested] [flat]

Commits from this thread:
  2025-11-23 b140c8d7a3  Add SupportRequestInlineInFrom planner support request.
* Inline non-SQL SRFs using SupportRequestSimplify
@ 2024-06-27 22:01  Paul Jungwirth <pj@illuminatedcomputing.com>
  0 siblings, 1 reply; 19+ messages in thread

From: Paul Jungwirth @ 2024-06-27 22:01 UTC (permalink / raw)
  To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

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

Attachments:

v1-0001-Inline-non-SQL-SRFs-using-SupportRequestSimplify.patch (text/x-patch, 1.3K) download From dcd58bbe27e715688d49b3d0244524c817c5c6c3 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com> Date: Thu, 27 Jun 2024 10:17:35 -0700 Subject: [PATCH v1] Inline non-SQL SRFs using SupportRequestSimplify If the support request returns a Query node, then inline_set_returning_function will use it in place of the FuncExpr. --- src/backend/optimizer/util/clauses.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index b4e085e9d4b..d412033dba6 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5085,11 +5085,17 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) if (rte->funcordinality) return NULL; - /* Fail if RTE isn't a single, simple FuncExpr */ if (list_length(rte->functions) != 1) return NULL; rtfunc = (RangeTblFunction *) linitial(rte->functions); + /* + * If the SRF was inlined using SupportRequestInline, then we expect a + * Query, otherwise a FuncExpr. Either way, only one item. + */ + if (IsA(rtfunc->funcexpr, Query)) + return (Query *)rtfunc->funcexpr; + if (!IsA(rtfunc->funcexpr, FuncExpr)) return NULL; fexpr = (FuncExpr *) rtfunc->funcexpr; -- 2.42.0
^ permalink raw reply [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2024-07-02 12:59  Heikki Linnakangas <hlinnaka@iki.fi>
  parent: Paul Jungwirth <pj@illuminatedcomputing.com>
  0 siblings, 1 reply; 19+ messages in thread

From: Heikki Linnakangas @ 2024-07-02 12:59 UTC (permalink / raw)
  To: Paul Jungwirth <pj@illuminatedcomputing.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

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)






^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2024-07-26 18:58  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Heikki Linnakangas <hlinnaka@iki.fi>
  0 siblings, 1 reply; 19+ messages in thread

From: Tom Lane @ 2024-07-26 18:58 UTC (permalink / raw)
  To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Paul Jungwirth <pj@illuminatedcomputing.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

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





^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2024-08-30 16:26  Paul Jungwirth <pj@illuminatedcomputing.com>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 19+ messages in thread

From: Paul Jungwirth @ 2024-08-30 16:26 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

On 7/26/24 11:58, Tom Lane wrote:
 > 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.

Here are new patches using a new SupportRequestInlineSRF request type. They include patches and 
documentation.

The patches handle this:

    SELECT * FROM srf();

but not this:

    SELECT srf();

In the latter case, Postgres always calls the function in "materialized mode" and gets the whole 
result up front, so inline_set_returning_function is never called, even for SQL functions.

For tests I added a `foo_from_bar(colname, tablename, filter)` PL/pgSQL function that does `SELECT 
$colname FROM $tablename [WHERE $colname = $filter]`, then the support function generates the same 
SQL and turns it into a Query node. This matches how I want to use the feature for my 
temporal_semijoin etc functions. If you give a non-NULL filter, you get a Query with a Var node, so 
we are testing something that isn't purely Const.

The SupportRequestSimplify type has some comments about supporting operators, but I don't think you 
can have a set-returning operator, so I didn't repeat those comments for this new type.

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.

Rebased to a83a944e9f.

Yours,

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

Attachments: download all (3 files, .tar.gz)

v2-0001-Add-indented-section.patch (text/x-patch, 3.2K) download From 591de3a06ec97cf1bff10d603946f0bac176b9d9 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com> Date: Thu, 29 Aug 2024 18:17:55 -0700 Subject: [PATCH v2 1/3] Add indented section --- src/backend/optimizer/util/clauses.c | 74 +++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index b4e085e9d4b..ef8282ded49 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5069,7 +5069,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) TupleDesc rettupdesc; List *raw_parsetree_list; List *querytree_list; - Query *querytree; + Query *querytree = NULL; Assert(rte->rtekind == RTE_FUNCTION); @@ -5235,6 +5235,78 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) goto fail; querytree = linitial(querytree_list); } + if (!querytree) + { + /* Fetch the function body */ + tmp = SysCacheGetAttrNotNull(PROCOID, func_tuple, Anum_pg_proc_prosrc); + src = TextDatumGetCString(tmp); + + /* + * Setup error traceback support for ereport(). This is so that we can + * finger the function that bad information came from. + */ + callback_arg.proname = NameStr(funcform->proname); + callback_arg.prosrc = src; + + sqlerrcontext.callback = sql_inline_error_callback; + sqlerrcontext.arg = (void *) &callback_arg; + sqlerrcontext.previous = error_context_stack; + error_context_stack = &sqlerrcontext; + + /* If we have prosqlbody, pay attention to that not prosrc */ + tmp = SysCacheGetAttr(PROCOID, + func_tuple, + Anum_pg_proc_prosqlbody, + &isNull); + if (!isNull) + { + Node *n; + + n = stringToNode(TextDatumGetCString(tmp)); + if (IsA(n, List)) + querytree_list = linitial_node(List, castNode(List, n)); + else + querytree_list = list_make1(n); + if (list_length(querytree_list) != 1) + goto fail; + querytree = linitial(querytree_list); + + /* Acquire necessary locks, then apply rewriter. */ + AcquireRewriteLocks(querytree, true, false); + querytree_list = pg_rewrite_query(querytree); + if (list_length(querytree_list) != 1) + goto fail; + querytree = linitial(querytree_list); + } + else + { + /* + * Set up to handle parameters while parsing the function body. We + * can use the FuncExpr just created as the input for + * prepare_sql_fn_parse_info. + */ + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) fexpr, + fexpr->inputcollid); + + /* + * Parse, analyze, and rewrite (unlike inline_function(), we can't + * skip rewriting here). We can fail as soon as we find more than one + * query, though. + */ + raw_parsetree_list = pg_parse_query(src); + if (list_length(raw_parsetree_list) != 1) + goto fail; + + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + src, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + goto fail; + querytree = linitial(querytree_list); + } + } /* * Also resolve the actual function result tupdesc, if composite. If we -- 2.42.0
v2-0002-Remove-unindented-section.patch (text/x-patch, 2.9K) download From 7a9ee15d17bee6a6743bf18e7428e6594e17be35 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com> Date: Thu, 29 Aug 2024 18:18:20 -0700 Subject: [PATCH v2 2/3] Remove unindented section --- src/backend/optimizer/util/clauses.c | 69 ---------------------------- 1 file changed, 69 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index ef8282ded49..d2d8f7eb0c0 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5166,75 +5166,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) ALLOCSET_DEFAULT_SIZES); oldcxt = MemoryContextSwitchTo(mycxt); - /* Fetch the function body */ - tmp = SysCacheGetAttrNotNull(PROCOID, func_tuple, Anum_pg_proc_prosrc); - src = TextDatumGetCString(tmp); - - /* - * Setup error traceback support for ereport(). This is so that we can - * finger the function that bad information came from. - */ - callback_arg.proname = NameStr(funcform->proname); - callback_arg.prosrc = src; - - sqlerrcontext.callback = sql_inline_error_callback; - sqlerrcontext.arg = (void *) &callback_arg; - sqlerrcontext.previous = error_context_stack; - error_context_stack = &sqlerrcontext; - - /* If we have prosqlbody, pay attention to that not prosrc */ - tmp = SysCacheGetAttr(PROCOID, - func_tuple, - Anum_pg_proc_prosqlbody, - &isNull); - if (!isNull) - { - Node *n; - - n = stringToNode(TextDatumGetCString(tmp)); - if (IsA(n, List)) - querytree_list = linitial_node(List, castNode(List, n)); - else - querytree_list = list_make1(n); - if (list_length(querytree_list) != 1) - goto fail; - querytree = linitial(querytree_list); - - /* Acquire necessary locks, then apply rewriter. */ - AcquireRewriteLocks(querytree, true, false); - querytree_list = pg_rewrite_query(querytree); - if (list_length(querytree_list) != 1) - goto fail; - querytree = linitial(querytree_list); - } - else - { - /* - * Set up to handle parameters while parsing the function body. We - * can use the FuncExpr just created as the input for - * prepare_sql_fn_parse_info. - */ - pinfo = prepare_sql_fn_parse_info(func_tuple, - (Node *) fexpr, - fexpr->inputcollid); - - /* - * Parse, analyze, and rewrite (unlike inline_function(), we can't - * skip rewriting here). We can fail as soon as we find more than one - * query, though. - */ - raw_parsetree_list = pg_parse_query(src); - if (list_length(raw_parsetree_list) != 1) - goto fail; - - querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), - src, - (ParserSetupHook) sql_fn_parser_setup, - pinfo, NULL); - if (list_length(querytree_list) != 1) - goto fail; - querytree = linitial(querytree_list); - } if (!querytree) { /* Fetch the function body */ -- 2.42.0
v2-0003-Inline-non-SQL-SRFs-using-SupportRequestInlineSRF.patch (text/x-patch, 22.9K) download From 4ed50cad16901c5b7b3447bad7105de11a81773a Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com> Date: Thu, 29 Aug 2024 18:20:47 -0700 Subject: [PATCH v2 3/3] Inline non-SQL SRFs using SupportRequestInlineSRF By inlining functions written in PL/pgSQL (or other languages), we let users compose dynamic queries without paying the overhead of function invocations. Formerly this was only possible for SQL functions. If the support request returns a Query node, then inline_set_returning_function will use it in place of the FuncExpr. The Query node can reference the original function's arguments as Var nodes. We only try to inline SRFs (even SQL SRFs) when they are used as RTEs, so inlining happens for `SELECT * FROM foo()` but not for `SELECT foo()`. --- doc/src/sgml/xfunc.sgml | 14 ++ src/backend/optimizer/util/clauses.c | 63 +++++- src/include/nodes/supportnodes.h | 22 ++ src/test/regress/expected/misc_functions.out | 212 +++++++++++++++++++ src/test/regress/regress.c | 91 ++++++++ src/test/regress/sql/misc_functions.sql | 87 ++++++++ 6 files changed, 487 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 9bc23a9a938..311a87b14a5 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -4076,6 +4076,20 @@ supportfn(internal) returns internal expression and an actual execution of the target function. </para> + <para> + Similarly, a <link linkend="queries-tablefunction">set-returning function</link> + can implement <literal>SupportRequestInlineSRF</literal> to return a + <literal>Query</literal> node, which the planner will try to inline into + the outer query, just as <productname>PostgreSQL</productname> inlines + SQL functions. Normallly only SQL functions can be inlined, but this support + request allows a function in <link linkend="plpgsql">PL/pgSQL</link> + or another language to build a dynamic SQL query and have it inlined too. + It is the responsibility of the support function to return + a node that matches the parent function's implementation. One way to do this is + to build a SQL string then parse it with <literal>pg_parse_query</literal>. + See <literal>regress.c</literal> for an example. + </para> + <para> For target functions that return <type>boolean</type>, it is often useful to estimate the fraction of rows that will be selected by a <literal>WHERE</literal> clause using that diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index d2d8f7eb0c0..9ee5831ed0e 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5134,7 +5134,8 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) funcform = (Form_pg_proc) GETSTRUCT(func_tuple); /* - * Forget it if the function is not SQL-language or has other showstopper + * Forget it unless the function is SQL-language or provides a support function. + * Also check for other showstopper * properties. In particular it mustn't be declared STRICT, since we * couldn't enforce that. It also mustn't be VOLATILE, because that is * supposed to cause it to be executed with its own snapshot, rather than @@ -5143,7 +5144,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) * of the function's last SELECT, which should not happen in that case. * (Rechecking prokind, proretset, and pronargs is just paranoia.) */ - if (funcform->prolang != SQLlanguageId || + if ((funcform->prolang != SQLlanguageId && !funcform->prosupport) || funcform->prokind != PROKIND_FUNCTION || funcform->proisstrict || funcform->provolatile == PROVOLATILE_VOLATILE || @@ -5166,6 +5167,64 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) ALLOCSET_DEFAULT_SIZES); oldcxt = MemoryContextSwitchTo(mycxt); + /* + * If the function has a SupportRequestInlineSRF support function, + * see if it can produce a Query node we can inline. + * This can be mutually-exclusive with SQL functions. + * Those are inlineable already, so it doesn't make sense to + * attach such a support request to them. + */ + if (funcform->prolang != SQLlanguageId && funcform->prosupport) { + SupportRequestInlineSRF req; + FuncExpr dummy_fexpr; + Node *newnode; + + /* + * We don't need the whole sqlerrcontext, + * but make sure we restore it correctly + * if we goto fail. + */ + sqlerrcontext.previous = error_context_stack; + + /* + * Build a SupportRequestInlineSRF node to pass to the support + * function, pointing to a dummy FuncExpr node containing the + * simplified arg list. We use this approach to present a uniform + * interface to the support function regardless of how the target + * function is actually being invoked. + */ + + dummy_fexpr.xpr.type = T_FuncExpr; + dummy_fexpr.funcid = func_oid; + dummy_fexpr.funcresulttype = funcform->prorettype; + dummy_fexpr.funcretset = funcform->proretset; + dummy_fexpr.funcvariadic = OidIsValid(funcform->provariadic); + dummy_fexpr.funcformat = COERCE_EXPLICIT_CALL; + dummy_fexpr.funccollid = InvalidOid; + dummy_fexpr.inputcollid = InvalidOid; + dummy_fexpr.args = fexpr->args; + dummy_fexpr.location = -1; + + req.type = T_SupportRequestInlineSRF; + req.fcall = &dummy_fexpr; + + newnode = (Node *) + DatumGetPointer(OidFunctionCall1(funcform->prosupport, + PointerGetDatum(&req))); + + if (!newnode) + goto fail; + + if (!IsA(newnode, Query)) + elog(ERROR, + "Got unexpected node type %d from SupportRequestInlineSRF for function %s", + newnode->type, NameStr(funcform->proname)); + + querytree = (Query *) newnode; + querytree_list = list_make1(querytree); + } + + if (!querytree) { /* Fetch the function body */ diff --git a/src/include/nodes/supportnodes.h b/src/include/nodes/supportnodes.h index 5f7bcde891c..bc0b5cccaf3 100644 --- a/src/include/nodes/supportnodes.h +++ b/src/include/nodes/supportnodes.h @@ -69,6 +69,28 @@ typedef struct SupportRequestSimplify FuncExpr *fcall; /* Function call to be simplified */ } SupportRequestSimplify; +/* + * The InlineSRF request allows the support function to perform plan-time + * simplification of a call to its target set-returning function. For + * example a plpgsql function could build a dynamic SQL query and execute it. + * Normally only SQL functions can be inlined, but with this support function + * the dynamic query can be inlined as well. + * + * "fcall" will be a FuncExpr invoking the support function's target + * function. + * + * The result should be a semantically-equivalent transformed node tree, + * or NULL if no simplification could be performed. Do *not* return or + * modify *fcall, as it isn't really a separately allocated Node. But + * it's okay to use fcall->args, or parts of it, in the result tree. + */ +typedef struct SupportRequestInlineSRF +{ + NodeTag type; + + FuncExpr *fcall; /* Function call to be simplified */ +} SupportRequestInlineSRF; + /* * The Selectivity request allows the support function to provide a * selectivity estimate for a function appearing at top level of a WHERE diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index 35fb72f302b..c5bac6d43ea 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -595,6 +595,11 @@ CREATE FUNCTION test_support_func(internal) RETURNS internal AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) SELECT * FROM tenk1 a JOIN tenk1 b ON a.unique1 = b.unique1 @@ -706,6 +711,213 @@ false, true, false, true); -- the support function. SELECT * FROM generate_series(TIMESTAMPTZ '2024-02-01', TIMESTAMPTZ '2024-03-01', INTERVAL '0 day') g(s); ERROR: step size cannot equal zero +-- +-- Test inlining PL/pgSQL functions +-- +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); ok diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index 14aad5a0c6e..9f97ad6ad45 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -29,6 +29,7 @@ #include "commands/sequence.h" #include "commands/trigger.h" #include "executor/executor.h" +#include "executor/functions.h" #include "executor/spi.h" #include "funcapi.h" #include "mb/pg_wchar.h" @@ -39,11 +40,13 @@ #include "parser/parse_coerce.h" #include "port/atomics.h" #include "storage/spin.h" +#include "tcop/tcopprot.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/geo_decls.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/syscache.h" #include "utils/typcache.h" #define EXPECT_TRUE(expr) \ @@ -989,6 +992,94 @@ test_support_func(PG_FUNCTION_ARGS) PG_RETURN_POINTER(ret); } +PG_FUNCTION_INFO_V1(test_inline_srf_support_func); +Datum +test_inline_srf_support_func(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + + if (IsA(rawreq, SupportRequestInlineSRF)) + { + /* + * Assume that the target is foo_from_bar; that's safe as long + * as we don't attach this to any other set-returning function. + */ + SupportRequestInlineSRF *req = (SupportRequestInlineSRF *) rawreq; + StringInfoData sql; + FuncExpr *expr = req->fcall; + Node *node; + Const *c; + char *colname; + char *tablename; + HeapTuple func_tuple; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + List *querytree_list; + Query *querytree; + + if (list_length(expr->args) != 3) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with %d args but expected 3", list_length(expr->args)))); + + /* Get colname */ + node = linitial(expr->args); + if (!IsA(node, Const)) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + c = (Const *) node; + if (c->consttype != TEXTOID) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + colname = TextDatumGetCString(c->constvalue); + + /* Get tablename */ + node = lsecond(expr->args); + if (!IsA(node, Const)) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + c = (Const *) node; + if (c->consttype != TEXTOID) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + tablename = TextDatumGetCString(c->constvalue); + + initStringInfo(&sql); + appendStringInfo(&sql, "SELECT %s::text FROM %s", quote_identifier(colname), quote_identifier(tablename)); + + /* Get filter if present */ + node = lthird(expr->args); + if (!(IsA(node, Const) && ((Const *) node)->constisnull)) + { + appendStringInfo(&sql, " WHERE %s::text = $3", quote_identifier(colname)); + } + + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(expr->funcid)); + if (!HeapTupleIsValid(func_tuple)) + elog(ERROR, "cache lookup failed for function %u", expr->funcid); + + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) expr, + expr->inputcollid); + + ReleaseSysCache(func_tuple); + + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + elog(ERROR, "test_inline_srf_support_func parsed to more than one node"); + + querytree_list = pg_analyze_and_rewrite_withcb( + linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + elog(ERROR, "test_inline_srf_support_func rewrote to more than one node"); + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + ereport(ERROR, (errmsg("test_inline_srf_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(querytree); + } + + PG_RETURN_POINTER(NULL); +} + PG_FUNCTION_INFO_V1(test_opclass_options_func); Datum test_opclass_options_func(PG_FUNCTION_ARGS) diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index e570783453c..bf692edd0b7 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -247,6 +247,12 @@ CREATE FUNCTION test_support_func(internal) AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; + ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) @@ -265,6 +271,8 @@ SELECT * FROM tenk1 a JOIN my_gen_series(1,1000) g ON a.unique1 = g; EXPLAIN (COSTS OFF) SELECT * FROM tenk1 a JOIN my_gen_series(1,10) g ON a.unique1 = g; + + -- -- Test the SupportRequestRows support function for generate_series_timestamp() -- @@ -310,6 +318,85 @@ false, true, false, true); -- the support function. SELECT * FROM generate_series(TIMESTAMPTZ '2024-02-01', TIMESTAMPTZ '2024-03-01', INTERVAL '0 day') g(s); +-- +-- Test inlining PL/pgSQL functions +-- +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; + -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); SELECT count(*) > 0 AS ok FROM pg_control_init(); -- 2.42.0
^ permalink raw reply [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2024-09-03 16:42  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Paul Jungwirth <pj@illuminatedcomputing.com>
  0 siblings, 1 reply; 19+ messages in thread

From: Tom Lane @ 2024-09-03 16:42 UTC (permalink / raw)
  To: Paul Jungwirth <pj@illuminatedcomputing.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

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





^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-06-30 21:42  Paul Jungwirth <pj@illuminatedcomputing.com>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 19+ messages in thread

From: Paul Jungwirth @ 2025-06-30 21:42 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

On 9/3/24 09:42, Tom Lane wrote:
> 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.
 > ...
> 2. The documentation needs to be a great deal more explicit
> about what the function is supposed to return.

Thanks for the review . . . and your patience waiting for an update!

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.

The refactor lets us share lots of pre-condition checks, as well as parameter substitution into the 
Query result. In some cases the refactor changes the order of things, but all of those changes 
looked safe to me. I didn't love passing a SysCache HeapTuple into another function, but it does 
make the cleanup a little easier, since now we can always release it in the same place.

The first patch gave me a wacky diff, but I couldn't get git to make something less fragmented. The 
idea is simple though: move part of inline_set_returning_function into 
inline_sql_set_returning_function, and call that instead.

Rebased to 0836683a89.

Yours,

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

Attachments: download all (2 files, .tar.gz)

v2-0001-Move-some-things-outside-of-inline_set_returning_.patch (text/x-patch, 10.0K) download From 2a0d2bb6cc6fe25cab1065411ed4db17cc394c53 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com> Date: Tue, 24 Jun 2025 19:10:15 -0700 Subject: [PATCH v2 1/2] Move some things outside of inline_set_returning_function. Added a new inline_sql_set_returning_function in preparation for inline_set_returning_function_with_support. Then inline_set_returning_function can call both, handling their shared needs itself. Author: Paul A. Jungwirth <pj@illuminatedcomputing.com> --- src/backend/optimizer/util/clauses.c | 212 +++++++++++++++------------ 1 file changed, 122 insertions(+), 90 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 26a3e050086..a0dc5dfcd61 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5049,29 +5049,21 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, /* - * inline_set_returning_function - * Attempt to "inline" a set-returning function in the FROM clause. - * - * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a - * set-returning SQL function that can safely be inlined, expand the function - * and return the substitute Query structure. Otherwise, return NULL. + * inline_sql_set_returning_function * - * We assume that the RTE's expression has already been put through - * eval_const_expressions(), which among other things will take care of - * default arguments and named-argument notation. + * This implements inline_set_returning_function for sql-language functions. + * It parses the body (or uses the pre-parsed body if available). + * It allocates its own temporary MemoryContext for the parsing, then copies + * the result into the caller's context. * - * This has a good deal of similarity to inline_function(), but that's - * for the non-set-returning case, and there are enough differences to - * justify separate functions. + * Returns NULL if the function couldn't be inlined. */ -Query * -inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) +static Query * +inline_sql_set_returning_function(PlannerInfo *root, RangeTblEntry *rte, + RangeTblFunction *rtfunc, + FuncExpr *fexpr, Oid func_oid, HeapTuple func_tuple, + Form_pg_proc funcform) { - RangeTblFunction *rtfunc; - FuncExpr *fexpr; - Oid func_oid; - HeapTuple func_tuple; - Form_pg_proc funcform; char *src; Datum tmp; bool isNull; @@ -5088,37 +5080,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) Assert(rte->rtekind == RTE_FUNCTION); - /* - * It doesn't make a lot of sense for a SQL SRF to refer to itself in its - * own FROM clause, since that must cause infinite recursion at runtime. - * It will cause this code to recurse too, so check for stack overflow. - * (There's no need to do more.) - */ - check_stack_depth(); - - /* Fail if the RTE has ORDINALITY - we don't implement that here. */ - if (rte->funcordinality) - return NULL; - - /* Fail if RTE isn't a single, simple FuncExpr */ - if (list_length(rte->functions) != 1) - return NULL; - rtfunc = (RangeTblFunction *) linitial(rte->functions); - - if (!IsA(rtfunc->funcexpr, FuncExpr)) - return NULL; - fexpr = (FuncExpr *) rtfunc->funcexpr; - - func_oid = fexpr->funcid; - - /* - * The function must be declared to return a set, else inlining would - * change the results if the contained SELECT didn't return exactly one - * row. - */ - if (!fexpr->funcretset) - return NULL; - /* * Refuse to inline if the arguments contain any volatile functions or * sub-selects. Volatile functions are rejected because inlining may @@ -5132,22 +5093,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) contain_subplans((Node *) fexpr->args)) return NULL; - /* Check permission to call function (fail later, if not) */ - if (object_aclcheck(ProcedureRelationId, func_oid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK) - return NULL; - - /* Check whether a plugin wants to hook function entry/exit */ - if (FmgrHookIsNeeded(func_oid)) - return NULL; - - /* - * OK, let's take a look at the function's pg_proc entry. - */ - func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(func_oid)); - if (!HeapTupleIsValid(func_tuple)) - elog(ERROR, "cache lookup failed for function %u", func_oid); - funcform = (Form_pg_proc) GETSTRUCT(func_tuple); - /* * Forget it if the function is not SQL-language or has other showstopper * properties. In particular it mustn't be declared STRICT, since we @@ -5168,7 +5113,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) list_length(fexpr->args) != funcform->pronargs || !heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL)) { - ReleaseSysCache(func_tuple); return NULL; } @@ -5177,7 +5121,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) * that parsing might create. */ mycxt = AllocSetContextCreate(CurrentMemoryContext, - "inline_set_returning_function", + "inline_sql_set_returning_function", ALLOCSET_DEFAULT_SIZES); oldcxt = MemoryContextSwitchTo(mycxt); @@ -5267,13 +5211,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) else functypclass = get_expr_result_type((Node *) fexpr, NULL, &rettupdesc); - /* - * The single command must be a plain SELECT. - */ - if (!IsA(querytree, Query) || - querytree->commandType != CMD_SELECT) - goto fail; - /* * Make sure the function (still) returns what it's declared to. This * will raise an error if wrong, but that's okay since the function would @@ -5302,13 +5239,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) */ querytree = linitial_node(Query, querytree_list); - /* - * Looks good --- substitute parameters into the query. - */ - querytree = substitute_actual_srf_parameters(querytree, - funcform->pronargs, - fexpr->args); - /* * Copy the modified query out of the temporary memory context, and clean * up. @@ -5319,7 +5249,112 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) MemoryContextDelete(mycxt); error_context_stack = sqlerrcontext.previous; - ReleaseSysCache(func_tuple); + + return querytree; + + /* Here if func is not inlinable: release temp memory and return NULL */ +fail: + MemoryContextSwitchTo(oldcxt); + MemoryContextDelete(mycxt); + error_context_stack = sqlerrcontext.previous; + + return NULL; +} + +/* + * inline_set_returning_function + * Attempt to "inline" an SQL set-returning function in the FROM clause. + * + * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a + * set-returning SQL function that can safely be inlined, expand the function + * and return the substitute Query structure. Otherwise, return NULL. + * + * We assume that the RTE's expression has already been put through + * eval_const_expressions(), which among other things will take care of + * default arguments and named-argument notation. + * + * This has a good deal of similarity to inline_function(), but that's + * for the non-set-returning case, and there are enough differences to + * justify separate functions. + */ +Query * +inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) +{ + RangeTblFunction *rtfunc; + FuncExpr *fexpr; + Oid func_oid; + HeapTuple func_tuple; + Form_pg_proc funcform; + Query *funcquery; + + Assert(rte->rtekind == RTE_FUNCTION); + + /* + * It doesn't make a lot of sense for a SRF to refer to itself in its own + * FROM clause, since that must cause infinite recursion at runtime. It + * will cause this code to recurse too, so check for stack overflow. + * (There's no need to do more.) + */ + check_stack_depth(); + + /* Fail if the RTE has ORDINALITY - we don't implement that here. */ + if (rte->funcordinality) + return NULL; + + /* Fail if RTE isn't a single, simple FuncExpr */ + if (list_length(rte->functions) != 1) + return NULL; + rtfunc = (RangeTblFunction *) linitial(rte->functions); + + if (!IsA(rtfunc->funcexpr, FuncExpr)) + return NULL; + fexpr = (FuncExpr *) rtfunc->funcexpr; + + func_oid = fexpr->funcid; + + /* + * The function must be declared to return a set, else inlining would + * change the results if the contained SELECT didn't return exactly one + * row. + */ + if (!fexpr->funcretset) + return NULL; + + /* Check permission to call function (fail later, if not) */ + if (object_aclcheck(ProcedureRelationId, func_oid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK) + return NULL; + + /* Check whether a plugin wants to hook function entry/exit */ + if (FmgrHookIsNeeded(func_oid)) + return NULL; + + /* + * OK, let's take a look at the function's pg_proc entry. + */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(func_oid)); + if (!HeapTupleIsValid(func_tuple)) + elog(ERROR, "cache lookup failed for function %u", func_oid); + funcform = (Form_pg_proc) GETSTRUCT(func_tuple); + + funcquery = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, + func_oid, func_tuple, funcform); + + if (!funcquery) + goto fail; + + /* + * The single command must be a plain SELECT. + */ + if (!IsA(funcquery, Query) || + funcquery->commandType != CMD_SELECT) + goto fail; + + /* + * Looks good --- substitute parameters into the query. + */ + funcquery = substitute_actual_srf_parameters(funcquery, + funcform->pronargs, + fexpr->args); /* * We don't have to fix collations here because the upper query is already @@ -5336,18 +5371,15 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) * We must also notice if the inserted query adds a dependency on the * calling role due to RLS quals. */ - if (querytree->hasRowSecurity) + if (funcquery->hasRowSecurity) root->glob->dependsOnRole = true; - return querytree; + ReleaseSysCache(func_tuple); + + return funcquery; - /* Here if func is not inlinable: release temp memory and return NULL */ fail: - MemoryContextSwitchTo(oldcxt); - MemoryContextDelete(mycxt); - error_context_stack = sqlerrcontext.previous; ReleaseSysCache(func_tuple); - return NULL; } -- 2.39.5
v2-0002-Add-SupportRequestInlineSRF.patch (text/x-patch, 27.2K) download From c48655640c56055ee5d10fd6feb804a88d7eee29 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com> Date: Tue, 24 Jun 2025 21:30:02 -0700 Subject: [PATCH v2 2/2] Add SupportRequestInlineSRF If a set-returning function has an attached support function that can handle SupportRequestInlineSRF, then we replace the FuncExpr with a Query node built by the support function. Then the planner can rewrite the Query as if it were from a SQL-language function, merging it with the outer query. Author: Paul A. Jungwirth <pj@illuminatedcomputing.com> --- doc/src/sgml/xfunc.sgml | 101 +++++++++ src/backend/optimizer/util/clauses.c | 65 +++++- src/include/nodes/supportnodes.h | 33 +++ src/test/regress/expected/misc_functions.out | 212 +++++++++++++++++++ src/test/regress/regress.c | 122 +++++++++++ src/test/regress/sql/misc_functions.sql | 86 ++++++++ 6 files changed, 614 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 2d81afce8cb..ef41ce49e65 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -4166,6 +4166,107 @@ supportfn(internal) returns internal expression and an actual execution of the target function. </para> + <para> + Similarly, a <link linkend="queries-tablefunctions">set-returning function</link> + can implement <literal>SupportRequestInlineSRF</literal> to return a + <literal>Query</literal> node, which the planner will try to inline into + the outer query, just as <productname>PostgreSQL</productname> inlines + SQL functions. Normallly only SQL functions can be inlined, but this support + request allows a function in <link linkend="plpgsql">PL/pgSQL</link> + or another language to build a dynamic SQL query and have it inlined too. + The <literal>Query</literal> node must be a <literal>SELECT</literal> query + that has gone through parse analysis and rewriting. + You may include <literal>Param</literal> nodes referencing the original function's + parameters, and <productname>PostgreSQL</productname> will map those appropriately + to the arguments passed by the caller. + It is the responsibility of the support function to return + a node that matches the parent function's implementation. + We make no guarantee that <productname>PostgreSQL</productname> will + never call the target function in cases that the support function could + simplify. Functions called in <literal>SELECT</literal> are not simplified. + Or if the <literal>RangeTblEntry</literal> has more than one + <literal>RangeTblFunction</literal> (such as when using + <literal>ROWS FROM</literal>), the function will not be simplified. + Ensure rigorous equivalence between the simplified expression and an actual + execution of the target function. + </para> + + <para> + One way to implement a <literal>SupportRequestInlineSRF</literal> support function + is to build a SQL string then parse it with <literal>pg_parse_query</literal>. + The outline of such a function might look like this: +<programlisting> +PG_FUNCTION_INFO_V1(my_support_function); +Datum +my_support_function(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + SupportRequestInlineSRF *req + RangeTblFunction *rtfunc; + FuncExpr *expr; + Query *querytree; + StringInfoData sql; + HeapTuple func_tuple; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + + /* Return if it's not a type we handle. */ + if (!IsA(rawreq, SupportRequestInlineSRF)) + PG_RETURN_POINTER(NULL); + + /* Get things we need off the support request node. */ + req = (SupportRequestInlineSRF *) rawreq; + rtfunc = req->rtfunc; + expr = (FuncExpr *) rtfunc->funcexpr; + + /* Generate the SQL string. */ + initStringInfo(&amp;sql); + appendStringInfo(&amp;sql, "SELECT ..."); + + /* Build a SQLFunctionParseInfo. */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(expr->funcid)); + if (!HeapTupleIsValid(func_tuple)) + { + ereport(WARNING, (errmsg("cache lookup failed for function %u", expr->funcid))); + PG_RETURN_POINTER(NULL); + } + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) expr, + expr->inputcollid); + ReleaseSysCache(func_tuple); + + /* Parse the SQL. */ + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + { + ereport(WARNING, (errmsg("my_support_func parsed to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + /* Analyze the parse tree as if it were a SQL-language body. */ + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + { + ereport(WARNING, (errmsg("my_support_func rewrote to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + { + ereport(WARNING, (errmsg("my_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(NULL); + } + + PG_RETURN_POINTER(querytree); +} +</programlisting> + </para> + <para> For target functions that return <type>boolean</type>, it is often useful to estimate the fraction of rows that will be selected by a <literal>WHERE</literal> clause using that diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index a0dc5dfcd61..c9304c2b8fb 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5048,6 +5048,50 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, } +/* + * inline_set_returning_function_with_support + * + * This implements inline_set_returning_function for functions with + * a support function that can handle SupportRequestInlineSRF. + * We let the support function make its own decisions about what it can + * handle. For instance we don't forbid a VOLATILE function or volatile + * arguments. + */ +static Query * +inline_set_returning_function_with_support(PlannerInfo *root, RangeTblEntry *rte, + RangeTblFunction *rtfunc, + FuncExpr *fexpr, Form_pg_proc funcform) +{ + SupportRequestInlineSRF req; + Node *newnode; + + /* It must have a support function. */ + Assert(funcform->prosupport); + + req.root = root; + req.type = T_SupportRequestInlineSRF; + req.rtfunc = rtfunc; + req.proc = funcform; + + /* + * XXX: wrap this in its own memory context, as + * inline_sql_set_returning_function does below? + */ + newnode = (Node *) + DatumGetPointer(OidFunctionCall1(funcform->prosupport, + PointerGetDatum(&req))); + + if (!newnode) + return NULL; + + if (!IsA(newnode, Query)) + elog(ERROR, + "Got unexpected node type %d from %s for function %s", + newnode->type, "SupportRequestInlineSRF", NameStr(funcform->proname)); + + return (Query *) newnode; +} + /* * inline_sql_set_returning_function * @@ -5263,10 +5307,10 @@ fail: /* * inline_set_returning_function - * Attempt to "inline" an SQL set-returning function in the FROM clause. + * Attempt to "inline" a set-returning function in the FROM clause. * * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a - * set-returning SQL function that can safely be inlined, expand the function + * set-returning function that can safely be inlined, expand the function * and return the substitute Query structure. Otherwise, return NULL. * * We assume that the RTE's expression has already been put through @@ -5285,7 +5329,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) Oid func_oid; HeapTuple func_tuple; Form_pg_proc funcform; - Query *funcquery; + Query *funcquery = NULL; Assert(rte->rtekind == RTE_FUNCTION); @@ -5336,8 +5380,19 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) elog(ERROR, "cache lookup failed for function %u", func_oid); funcform = (Form_pg_proc) GETSTRUCT(func_tuple); - funcquery = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, - func_oid, func_tuple, funcform); + /* + * If the function has an attached support function that can handle + * SupportRequestInlineSRF, then attempt to inline with that. Return the + * result if we get one, otherwise proceed. + */ + if (funcform->prosupport) + funcquery = inline_set_returning_function_with_support(root, rte, rtfunc, fexpr, + funcform); + + /* Try to inline automatically */ + if (!funcquery) + funcquery = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, + func_oid, func_tuple, funcform); if (!funcquery) goto fail; diff --git a/src/include/nodes/supportnodes.h b/src/include/nodes/supportnodes.h index 9c047cc401b..2016e8def65 100644 --- a/src/include/nodes/supportnodes.h +++ b/src/include/nodes/supportnodes.h @@ -33,6 +33,7 @@ #ifndef SUPPORTNODES_H #define SUPPORTNODES_H +#include "catalog/pg_proc.h" #include "nodes/plannodes.h" struct PlannerInfo; /* avoid including pathnodes.h here */ @@ -69,6 +70,38 @@ typedef struct SupportRequestSimplify FuncExpr *fcall; /* Function call to be simplified */ } SupportRequestSimplify; +/* + * The InlineSRF request allows the support function to perform plan-time + * simplification of a call to its target set-returning function. For + * example a PL/pgSQL function could build a dynamic SQL query and execute it. + * Normally only SQL functions can be inlined, but with this support function + * the dynamic query can be inlined as well. + * + * The planner's PlannerInfo "root" is typically not needed, but can be + * consulted if it's necessary to obtain info about Vars present in + * the given node tree. Beware that root could be NULL in some usages. + * + * "rtfunc" will be a RangeTblFunction node for the function being replaced. + * The support function is only called if rtfunc->functions contains a + * single FuncExpr node. (ROWS FROM is one way to get more than one.) + * + * "proc" will be the Form_pg_proc record for the function being replaced. + * + * The result should be a semantically-equivalent transformed node tree, + * or NULL if no simplification could be performed. It should be allocated + * in the CurrentMemoryContext. Do *not* return or modify the FuncExpr node + * tree, as it isn't really a separately allocated Node. But it's okay to + * use its args, or parts of it, in the result tree. + */ +typedef struct SupportRequestInlineSRF +{ + NodeTag type; + + struct PlannerInfo *root; /* Planner's infrastructure */ + RangeTblFunction *rtfunc; /* Function call to be simplified */ + Form_pg_proc proc; /* Function definition */ +} SupportRequestInlineSRF; + /* * The Selectivity request allows the support function to provide a * selectivity estimate for a function appearing at top level of a WHERE diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index c3b2b9d8603..13388ebc715 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -601,6 +601,11 @@ CREATE FUNCTION test_support_func(internal) RETURNS internal AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) SELECT * FROM tenk1 a JOIN tenk1 b ON a.unique1 = b.unique1 @@ -777,6 +782,213 @@ false, true, false, true); Function Scan on generate_series g (cost=N..N rows=1000 width=N) (1 row) +-- +-- Test inlining PL/pgSQL functions +-- +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); ok diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index 3dbba069024..626ec3a1a71 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -28,6 +28,7 @@ #include "commands/sequence.h" #include "commands/trigger.h" #include "executor/executor.h" +#include "executor/functions.h" #include "executor/spi.h" #include "funcapi.h" #include "mb/pg_wchar.h" @@ -39,11 +40,13 @@ #include "port/atomics.h" #include "postmaster/postmaster.h" /* for MAX_BACKENDS */ #include "storage/spin.h" +#include "tcop/tcopprot.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/geo_decls.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/syscache.h" #include "utils/typcache.h" #define EXPECT_TRUE(expr) \ @@ -803,6 +806,125 @@ test_support_func(PG_FUNCTION_ARGS) PG_RETURN_POINTER(ret); } +PG_FUNCTION_INFO_V1(test_inline_srf_support_func); +Datum +test_inline_srf_support_func(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + Query *querytree = NULL; + + if (IsA(rawreq, SupportRequestInlineSRF)) + { + /* + * Assume that the target is foo_from_bar; that's safe as long as we + * don't attach this to any other set-returning function. + */ + SupportRequestInlineSRF *req = (SupportRequestInlineSRF *) rawreq; + StringInfoData sql; + RangeTblFunction *rtfunc = req->rtfunc; + FuncExpr *expr = (FuncExpr *) rtfunc->funcexpr; + Node *node; + Const *c; + char *colname; + char *tablename; + HeapTuple func_tuple; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + List *querytree_list; + + if (list_length(expr->args) != 3) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with %d args but expected 3", list_length(expr->args)))); + PG_RETURN_POINTER(NULL); + } + + /* Get colname */ + node = linitial(expr->args); + if (!IsA(node, Const)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + PG_RETURN_POINTER(NULL); + } + + c = (Const *) node; + if (c->consttype != TEXTOID) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + PG_RETURN_POINTER(NULL); + } + colname = TextDatumGetCString(c->constvalue); + + /* Get tablename */ + node = lsecond(expr->args); + if (!IsA(node, Const)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + PG_RETURN_POINTER(NULL); + } + + c = (Const *) node; + if (c->consttype != TEXTOID) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + PG_RETURN_POINTER(NULL); + } + tablename = TextDatumGetCString(c->constvalue); + + initStringInfo(&sql); + appendStringInfo(&sql, "SELECT %s::text FROM %s", quote_identifier(colname), quote_identifier(tablename)); + + /* Get filter if present */ + node = lthird(expr->args); + if (!(IsA(node, Const) && ((Const *) node)->constisnull)) + { + appendStringInfo(&sql, " WHERE %s::text = $3", quote_identifier(colname)); + } + + /* Build a SQLFunctionParseInfo. */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(expr->funcid)); + if (!HeapTupleIsValid(func_tuple)) + { + ereport(WARNING, (errmsg("cache lookup failed for function %u", expr->funcid))); + PG_RETURN_POINTER(NULL); + } + + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) expr, + expr->inputcollid); + + ReleaseSysCache(func_tuple); + + /* Parse the SQL. */ + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func parsed to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + /* Analyze the parse tree as if it were a SQL-language body. */ + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func rewrote to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(NULL); + } + } + + PG_RETURN_POINTER(querytree); +} + PG_FUNCTION_INFO_V1(test_opclass_options_func); Datum test_opclass_options_func(PG_FUNCTION_ARGS) diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 23792c4132a..56c6e7a8a88 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -248,6 +248,12 @@ CREATE FUNCTION test_support_func(internal) AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; + ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) @@ -349,6 +355,86 @@ SELECT explain_mask_costs($$ SELECT * FROM generate_series(25.0, 2.0, 0.0) g(s);$$, false, true, false, true); +-- +-- Test inlining PL/pgSQL functions +-- + +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; + -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); SELECT count(*) > 0 AS ok FROM pg_control_init(); -- 2.39.5
^ permalink raw reply [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-07-14 21:21  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Paul Jungwirth <pj@illuminatedcomputing.com>
  0 siblings, 1 reply; 19+ messages in thread

From: Tom Lane @ 2025-07-14 21:21 UTC (permalink / raw)
  To: Paul Jungwirth <pj@illuminatedcomputing.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

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





^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-08-08 21:46  Paul A Jungwirth <pj@illuminatedcomputing.com>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 19+ messages in thread

From: Paul A Jungwirth @ 2025-08-08 21:46 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

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

Attachments: download all (2 files, .tar.gz)

v3-0001-Move-some-things-outside-of-inline_set_returning_.patch (application/octet-stream, 12.9K) download From c74e3fea09f44df39c62299777527e28616e0345 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com> Date: Tue, 24 Jun 2025 19:10:15 -0700 Subject: [PATCH v3 1/2] Move some things outside of inline_set_returning_function. Added a new inline_sql_set_returning_function in preparation for inline_set_returning_function_with_support. Then inline_set_returning_function can call both, handling their shared needs itself. Author: Paul A. Jungwirth <pj@illuminatedcomputing.com> --- src/backend/optimizer/util/clauses.c | 273 +++++++++++++++------------ 1 file changed, 157 insertions(+), 116 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 6f0b338d2cd..77f48ff4069 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5146,36 +5146,21 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, /* - * inline_set_returning_function - * Attempt to "inline" a set-returning function in the FROM clause. - * - * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a - * set-returning SQL function that can safely be inlined, expand the function - * and return the substitute Query structure. Otherwise, return NULL. + * inline_sql_set_returning_function * - * We assume that the RTE's expression has already been put through - * eval_const_expressions(), which among other things will take care of - * default arguments and named-argument notation. + * This implements inline_set_returning_function for sql-language functions. + * It parses the body (or uses the pre-parsed body if available). * - * This has a good deal of similarity to inline_function(), but that's - * for the non-set-returning case, and there are enough differences to - * justify separate functions. + * Returns NULL if the function couldn't be inlined. */ -Query * -inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) +static Query * +inline_sql_set_returning_function(PlannerInfo *root, RangeTblEntry *rte, + RangeTblFunction *rtfunc, + FuncExpr *fexpr, Oid func_oid, HeapTuple func_tuple, + Form_pg_proc funcform, char *src) { - RangeTblFunction *rtfunc; - FuncExpr *fexpr; - Oid func_oid; - HeapTuple func_tuple; - Form_pg_proc funcform; - char *src; - Datum tmp; + Datum sqlbody; bool isNull; - MemoryContext oldcxt; - MemoryContext mycxt; - inline_error_callback_arg callback_arg; - ErrorContextCallback sqlerrcontext; SQLFunctionParseInfoPtr pinfo; TypeFuncClass functypclass; TupleDesc rettupdesc; @@ -5185,29 +5170,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) Assert(rte->rtekind == RTE_FUNCTION); - /* - * It doesn't make a lot of sense for a SQL SRF to refer to itself in its - * own FROM clause, since that must cause infinite recursion at runtime. - * It will cause this code to recurse too, so check for stack overflow. - * (There's no need to do more.) - */ - check_stack_depth(); - - /* Fail if the RTE has ORDINALITY - we don't implement that here. */ - if (rte->funcordinality) - return NULL; - - /* Fail if RTE isn't a single, simple FuncExpr */ - if (list_length(rte->functions) != 1) - return NULL; - rtfunc = (RangeTblFunction *) linitial(rte->functions); - - if (!IsA(rtfunc->funcexpr, FuncExpr)) - return NULL; - fexpr = (FuncExpr *) rtfunc->funcexpr; - - func_oid = fexpr->funcid; - /* * The function must be declared to return a set, else inlining would * change the results if the contained SELECT didn't return exactly one @@ -5216,35 +5178,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) if (!fexpr->funcretset) return NULL; - /* - * 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; - - /* Check permission to call function (fail later, if not) */ - if (object_aclcheck(ProcedureRelationId, func_oid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK) - return NULL; - - /* Check whether a plugin wants to hook function entry/exit */ - if (FmgrHookIsNeeded(func_oid)) - return NULL; - - /* - * OK, let's take a look at the function's pg_proc entry. - */ - func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(func_oid)); - if (!HeapTupleIsValid(func_tuple)) - elog(ERROR, "cache lookup failed for function %u", func_oid); - funcform = (Form_pg_proc) GETSTRUCT(func_tuple); - /* * Forget it if the function is not SQL-language or has other showstopper * properties. In particular it mustn't be declared STRICT, since we @@ -5262,61 +5195,34 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) funcform->prorettype == VOIDOID || funcform->prosecdef || !funcform->proretset || - list_length(fexpr->args) != funcform->pronargs || - !heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL)) + list_length(fexpr->args) != funcform->pronargs) { - ReleaseSysCache(func_tuple); return NULL; } - /* - * Make a temporary memory context, so that we don't leak all the stuff - * that parsing might create. - */ - mycxt = AllocSetContextCreate(CurrentMemoryContext, - "inline_set_returning_function", - ALLOCSET_DEFAULT_SIZES); - oldcxt = MemoryContextSwitchTo(mycxt); - - /* Fetch the function body */ - tmp = SysCacheGetAttrNotNull(PROCOID, func_tuple, Anum_pg_proc_prosrc); - src = TextDatumGetCString(tmp); - - /* - * Setup error traceback support for ereport(). This is so that we can - * finger the function that bad information came from. - */ - callback_arg.proname = NameStr(funcform->proname); - callback_arg.prosrc = src; - - sqlerrcontext.callback = sql_inline_error_callback; - sqlerrcontext.arg = &callback_arg; - sqlerrcontext.previous = error_context_stack; - error_context_stack = &sqlerrcontext; - /* If we have prosqlbody, pay attention to that not prosrc */ - tmp = SysCacheGetAttr(PROCOID, - func_tuple, - Anum_pg_proc_prosqlbody, - &isNull); + sqlbody = SysCacheGetAttr(PROCOID, + func_tuple, + Anum_pg_proc_prosqlbody, + &isNull); if (!isNull) { Node *n; - n = stringToNode(TextDatumGetCString(tmp)); + n = stringToNode(TextDatumGetCString(sqlbody)); if (IsA(n, List)) querytree_list = linitial_node(List, castNode(List, n)); else querytree_list = list_make1(n); if (list_length(querytree_list) != 1) - goto fail; + return NULL; querytree = linitial(querytree_list); /* Acquire necessary locks, then apply rewriter. */ AcquireRewriteLocks(querytree, true, false); querytree_list = pg_rewrite_query(querytree); if (list_length(querytree_list) != 1) - goto fail; + return NULL; querytree = linitial(querytree_list); } else @@ -5337,14 +5243,14 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) */ raw_parsetree_list = pg_parse_query(src); if (list_length(raw_parsetree_list) != 1) - goto fail; + return NULL; querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), src, (ParserSetupHook) sql_fn_parser_setup, pinfo, NULL); if (list_length(querytree_list) != 1) - goto fail; + return NULL; querytree = linitial(querytree_list); } @@ -5369,7 +5275,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) */ if (!IsA(querytree, Query) || querytree->commandType != CMD_SELECT) - goto fail; + return NULL; /* * Make sure the function (still) returns what it's declared to. This @@ -5391,7 +5297,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) (functypclass == TYPEFUNC_COMPOSITE || functypclass == TYPEFUNC_COMPOSITE_DOMAIN || functypclass == TYPEFUNC_RECORD)) - goto fail; /* reject not-whole-tuple-result cases */ + return NULL; /* reject not-whole-tuple-result cases */ /* * check_sql_fn_retval might've inserted a projection step, but that's @@ -5399,6 +5305,141 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) */ querytree = linitial_node(Query, querytree_list); + return querytree; +} + +/* + * inline_set_returning_function + * Attempt to "inline" an SQL set-returning function in the FROM clause. + * + * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a + * set-returning SQL function that can safely be inlined, expand the function + * and return the substitute Query structure. Otherwise, return NULL. + * + * We assume that the RTE's expression has already been put through + * eval_const_expressions(), which among other things will take care of + * default arguments and named-argument notation. + * + * This has a good deal of similarity to inline_function(), but that's + * for the non-set-returning case, and there are enough differences to + * justify separate functions. + * + * It allocates its own temporary MemoryContext for the parsing, then copies + * the result into the caller's context. + */ +Query * +inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) +{ + RangeTblFunction *rtfunc; + FuncExpr *fexpr; + Oid func_oid; + HeapTuple func_tuple; + Form_pg_proc funcform; + Datum tmp; + char *src; + inline_error_callback_arg callback_arg; + ErrorContextCallback sqlerrcontext; + MemoryContext oldcxt; + MemoryContext mycxt; + Query *querytree; + + Assert(rte->rtekind == RTE_FUNCTION); + + /* + * It doesn't make a lot of sense for a SRF to refer to itself in its own + * FROM clause, since that must cause infinite recursion at runtime. It + * will cause this code to recurse too, so check for stack overflow. + * (There's no need to do more.) + */ + check_stack_depth(); + + /* Fail if the RTE has ORDINALITY - we don't implement that here. */ + if (rte->funcordinality) + return NULL; + + /* Fail if RTE isn't a single, simple FuncExpr */ + if (list_length(rte->functions) != 1) + return NULL; + rtfunc = (RangeTblFunction *) linitial(rte->functions); + + if (!IsA(rtfunc->funcexpr, FuncExpr)) + return NULL; + fexpr = (FuncExpr *) rtfunc->funcexpr; + + func_oid = fexpr->funcid; + + /* + * 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; + + /* Check permission to call function (fail later, if not) */ + if (object_aclcheck(ProcedureRelationId, func_oid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK) + return NULL; + + /* Check whether a plugin wants to hook function entry/exit */ + if (FmgrHookIsNeeded(func_oid)) + return NULL; + + /* + * OK, let's take a look at the function's pg_proc entry. + */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(func_oid)); + if (!HeapTupleIsValid(func_tuple)) + elog(ERROR, "cache lookup failed for function %u", func_oid); + funcform = (Form_pg_proc) GETSTRUCT(func_tuple); + + /* + * Make a temporary memory context, so that we don't leak all the stuff + * that parsing might create. + */ + mycxt = AllocSetContextCreate(CurrentMemoryContext, + "inline_set_returning_function", + ALLOCSET_DEFAULT_SIZES); + oldcxt = MemoryContextSwitchTo(mycxt); + + /* Fetch the function body */ + tmp = SysCacheGetAttrNotNull(PROCOID, func_tuple, Anum_pg_proc_prosrc); + src = TextDatumGetCString(tmp); + + /* + * Setup error traceback support for ereport(). This is so that we can + * finger the function that bad information came from. + */ + callback_arg.proname = NameStr(funcform->proname); + callback_arg.prosrc = src; + + sqlerrcontext.callback = sql_inline_error_callback; + sqlerrcontext.arg = &callback_arg; + sqlerrcontext.previous = error_context_stack; + error_context_stack = &sqlerrcontext; + + /* + * If the function SETs configuration parameters, inlining would cause us + * to skip those changes. + */ + if (!heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL)) + goto fail; + + querytree = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, + func_oid, func_tuple, funcform, + src); + + if (!querytree) + goto fail; + + /* Only SELECTs are permitted */ + Assert(IsA(querytree, Query)); + Assert(querytree->commandType == CMD_SELECT); + /* * Looks good --- substitute parameters into the query. */ -- 2.45.0
v3-0002-Add-SupportRequestInlineSRF.patch (application/octet-stream, 27.5K) download From d6409f6d61019e54ae5c1a49ef57b7bded80de7b Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com> Date: Tue, 24 Jun 2025 21:30:02 -0700 Subject: [PATCH v3 2/2] Add SupportRequestInlineSRF If a set-returning function has an attached support function that can handle SupportRequestInlineSRF, then we replace the FuncExpr with a Query node built by the support function. Then the planner can rewrite the Query as if it were from a SQL-language function, merging it with the outer query. Author: Paul A. Jungwirth <pj@illuminatedcomputing.com> --- doc/src/sgml/xfunc.sgml | 101 +++++++++ src/backend/optimizer/util/clauses.c | 63 +++++- src/include/nodes/supportnodes.h | 34 +++ src/test/regress/expected/misc_functions.out | 212 +++++++++++++++++++ src/test/regress/regress.c | 114 ++++++++++ src/test/regress/sql/misc_functions.sql | 86 ++++++++ src/tools/pgindent/typedefs.list | 1 + 7 files changed, 605 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 30219f432d9..ecd7ced8be6 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -4166,6 +4166,107 @@ supportfn(internal) returns internal expression and an actual execution of the target function. </para> + <para> + Similarly, a <link linkend="queries-tablefunctions">set-returning function</link> + can implement <literal>SupportRequestInlineSRF</literal> to return a + <literal>Query</literal> node, which the planner will try to inline into + the outer query, just as <productname>PostgreSQL</productname> inlines + SQL functions. Normallly only SQL functions can be inlined, but this support + request allows a function in <link linkend="plpgsql">PL/pgSQL</link> + or another language to build a dynamic SQL query and have it inlined too. + The <literal>Query</literal> node must be a <literal>SELECT</literal> query + that has gone through parse analysis and rewriting. + You may include <literal>Param</literal> nodes referencing the original function's + parameters, and <productname>PostgreSQL</productname> will map those appropriately + to the arguments passed by the caller. + It is the responsibility of the support function to return + a node that matches the parent function's implementation. + We make no guarantee that <productname>PostgreSQL</productname> will + never call the target function in cases that the support function could + simplify. Functions called in <literal>SELECT</literal> are not simplified. + Or if the <literal>RangeTblEntry</literal> has more than one + <literal>RangeTblFunction</literal> (such as when using + <literal>ROWS FROM</literal>), the function will not be simplified. + Ensure rigorous equivalence between the simplified expression and an actual + execution of the target function. + </para> + + <para> + One way to implement a <literal>SupportRequestInlineSRF</literal> support function + is to build a SQL string then parse it with <literal>pg_parse_query</literal>. + The outline of such a function might look like this: +<programlisting> +PG_FUNCTION_INFO_V1(my_support_function); +Datum +my_support_function(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + SupportRequestInlineSRF *req + RangeTblFunction *rtfunc; + FuncExpr *expr; + Query *querytree; + StringInfoData sql; + HeapTuple func_tuple; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + + /* Return if it's not a type we handle. */ + if (!IsA(rawreq, SupportRequestInlineSRF)) + PG_RETURN_POINTER(NULL); + + /* Get things we need off the support request node. */ + req = (SupportRequestInlineSRF *) rawreq; + rtfunc = req->rtfunc; + expr = (FuncExpr *) rtfunc->funcexpr; + + /* Generate the SQL string. */ + initStringInfo(&amp;sql); + appendStringInfo(&amp;sql, "SELECT ..."); + + /* Build a SQLFunctionParseInfo. */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(expr->funcid)); + if (!HeapTupleIsValid(func_tuple)) + { + ereport(WARNING, (errmsg("cache lookup failed for function %u", expr->funcid))); + PG_RETURN_POINTER(NULL); + } + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) expr, + expr->inputcollid); + ReleaseSysCache(func_tuple); + + /* Parse the SQL. */ + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + { + ereport(WARNING, (errmsg("my_support_func parsed to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + /* Analyze the parse tree as if it were a SQL-language body. */ + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + { + ereport(WARNING, (errmsg("my_support_func rewrote to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + { + ereport(WARNING, (errmsg("my_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(NULL); + } + + PG_RETURN_POINTER(querytree); +} +</programlisting> + </para> + <para> For target functions that return <type>boolean</type>, it is often useful to estimate the fraction of rows that will be selected by a <literal>WHERE</literal> clause using that diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 77f48ff4069..312237a5e13 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5145,6 +5145,47 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, } +/* + * inline_set_returning_function_with_support + * + * This implements inline_set_returning_function for functions with + * a support function that can handle SupportRequestInlineSRF. + * We check fewer things than inline_sql_set_returning_function, + * so that support functions can make their own decisions about what + * to handle. For instance we don't forbid a VOLATILE function. + */ +static Query * +inline_set_returning_function_with_support(PlannerInfo *root, RangeTblEntry *rte, + RangeTblFunction *rtfunc, + FuncExpr *fexpr, HeapTuple func_tuple, + Form_pg_proc funcform) +{ + SupportRequestInlineSRF req; + Node *newnode; + + /* It must have a support function. */ + Assert(funcform->prosupport); + + req.root = root; + req.type = T_SupportRequestInlineSRF; + req.rtfunc = rtfunc; + req.proc = func_tuple; + + newnode = (Node *) + DatumGetPointer(OidFunctionCall1(funcform->prosupport, + PointerGetDatum(&req))); + + if (!newnode) + return NULL; + + if (!IsA(newnode, Query)) + elog(ERROR, + "Got unexpected node type %d from %s for function %s", + newnode->type, "SupportRequestInlineSRF", NameStr(funcform->proname)); + + return (Query *) newnode; +} + /* * inline_sql_set_returning_function * @@ -5310,10 +5351,10 @@ inline_sql_set_returning_function(PlannerInfo *root, RangeTblEntry *rte, /* * inline_set_returning_function - * Attempt to "inline" an SQL set-returning function in the FROM clause. + * Attempt to "inline" a set-returning function in the FROM clause. * * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a - * set-returning SQL function that can safely be inlined, expand the function + * set-returning function that can safely be inlined, expand the function * and return the substitute Query structure. Otherwise, return NULL. * * We assume that the RTE's expression has already been put through @@ -5341,7 +5382,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) ErrorContextCallback sqlerrcontext; MemoryContext oldcxt; MemoryContext mycxt; - Query *querytree; + Query *querytree = NULL; Assert(rte->rtekind == RTE_FUNCTION); @@ -5429,9 +5470,19 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) if (!heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL)) goto fail; - querytree = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, - func_oid, func_tuple, funcform, - src); + /* + * If the function has an attached support function that can handle + * SupportRequestInlineSRF, then attempt to inline with that. Return the + * result if we get one, otherwise proceed. + */ + if (funcform->prosupport) + querytree = inline_set_returning_function_with_support(root, rte, rtfunc, fexpr, + func_tuple, funcform); + + /* Try to inline automatically */ + if (!querytree) + querytree = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, + func_oid, func_tuple, funcform, src); if (!querytree) goto fail; diff --git a/src/include/nodes/supportnodes.h b/src/include/nodes/supportnodes.h index 9c047cc401b..94f8a52de2b 100644 --- a/src/include/nodes/supportnodes.h +++ b/src/include/nodes/supportnodes.h @@ -33,6 +33,7 @@ #ifndef SUPPORTNODES_H #define SUPPORTNODES_H +#include "catalog/pg_proc.h" #include "nodes/plannodes.h" struct PlannerInfo; /* avoid including pathnodes.h here */ @@ -69,6 +70,39 @@ typedef struct SupportRequestSimplify FuncExpr *fcall; /* Function call to be simplified */ } SupportRequestSimplify; +/* + * The InlineSRF request allows the support function to perform plan-time + * simplification of a call to its target set-returning function. For + * example a PL/pgSQL function could build a dynamic SQL query and execute it. + * Normally only SQL functions can be inlined, but with this support function + * the dynamic query can be inlined as well. + * + * The planner's PlannerInfo "root" is typically not needed, but can be + * consulted if it's necessary to obtain info about Vars present in + * the given node tree. Beware that root could be NULL in some usages. + * + * "rtfunc" will be a RangeTblFunction node for the function being replaced. + * The support function is only called if rtfunc->functions contains a + * single FuncExpr node. (ROWS FROM is one way to get more than one.) + * + * "proc" will be the HeapTuple for the pg_proc record of the function being + * replaced. + * + * The result should be a semantically-equivalent transformed node tree, + * or NULL if no simplification could be performed. It should be allocated + * in the CurrentMemoryContext. Do *not* return or modify the FuncExpr node + * tree, as it isn't really a separately allocated Node. But it's okay to + * use its args, or parts of it, in the result tree. + */ +typedef struct SupportRequestInlineSRF +{ + NodeTag type; + + struct PlannerInfo *root; /* Planner's infrastructure */ + RangeTblFunction *rtfunc; /* Function call to be simplified */ + HeapTuple proc; /* Function definition from pg_proc */ +} SupportRequestInlineSRF; + /* * The Selectivity request allows the support function to provide a * selectivity estimate for a function appearing at top level of a WHERE diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index c3b2b9d8603..13388ebc715 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -601,6 +601,11 @@ CREATE FUNCTION test_support_func(internal) RETURNS internal AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) SELECT * FROM tenk1 a JOIN tenk1 b ON a.unique1 = b.unique1 @@ -777,6 +782,213 @@ false, true, false, true); Function Scan on generate_series g (cost=N..N rows=1000 width=N) (1 row) +-- +-- Test inlining PL/pgSQL functions +-- +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); ok diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index 3dbba069024..eadb40369d4 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -28,6 +28,7 @@ #include "commands/sequence.h" #include "commands/trigger.h" #include "executor/executor.h" +#include "executor/functions.h" #include "executor/spi.h" #include "funcapi.h" #include "mb/pg_wchar.h" @@ -39,11 +40,13 @@ #include "port/atomics.h" #include "postmaster/postmaster.h" /* for MAX_BACKENDS */ #include "storage/spin.h" +#include "tcop/tcopprot.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/geo_decls.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/syscache.h" #include "utils/typcache.h" #define EXPECT_TRUE(expr) \ @@ -803,6 +806,117 @@ test_support_func(PG_FUNCTION_ARGS) PG_RETURN_POINTER(ret); } +PG_FUNCTION_INFO_V1(test_inline_srf_support_func); +Datum +test_inline_srf_support_func(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + Query *querytree = NULL; + + if (IsA(rawreq, SupportRequestInlineSRF)) + { + /* + * Assume that the target is foo_from_bar; that's safe as long as we + * don't attach this to any other set-returning function. + */ + SupportRequestInlineSRF *req = (SupportRequestInlineSRF *) rawreq; + StringInfoData sql; + RangeTblFunction *rtfunc = req->rtfunc; + FuncExpr *expr = (FuncExpr *) rtfunc->funcexpr; + Node *node; + Const *c; + char *colname; + char *tablename; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + List *querytree_list; + + if (list_length(expr->args) != 3) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with %d args but expected 3", list_length(expr->args)))); + PG_RETURN_POINTER(NULL); + } + + /* Get colname */ + node = linitial(expr->args); + if (!IsA(node, Const)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + PG_RETURN_POINTER(NULL); + } + + c = (Const *) node; + if (c->consttype != TEXTOID) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + PG_RETURN_POINTER(NULL); + } + colname = TextDatumGetCString(c->constvalue); + + /* Get tablename */ + node = lsecond(expr->args); + if (!IsA(node, Const)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + PG_RETURN_POINTER(NULL); + } + + c = (Const *) node; + if (c->consttype != TEXTOID) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + PG_RETURN_POINTER(NULL); + } + tablename = TextDatumGetCString(c->constvalue); + + initStringInfo(&sql); + appendStringInfo(&sql, "SELECT %s::text FROM %s", quote_identifier(colname), quote_identifier(tablename)); + + /* Get filter if present */ + node = lthird(expr->args); + if (!(IsA(node, Const) && ((Const *) node)->constisnull)) + { + /* Only filter if $3 is Const */ + appendStringInfo(&sql, " WHERE %s::text = $3", quote_identifier(colname)); + } + + /* Build a SQLFunctionParseInfo. */ + + pinfo = prepare_sql_fn_parse_info(req->proc, + (Node *) expr, + expr->inputcollid); + + /* Parse the SQL. */ + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func parsed to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + /* Analyze the parse tree as if it were a SQL-language body. */ + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func rewrote to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(NULL); + } + } + + PG_RETURN_POINTER(querytree); +} + PG_FUNCTION_INFO_V1(test_opclass_options_func); Datum test_opclass_options_func(PG_FUNCTION_ARGS) diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 23792c4132a..56c6e7a8a88 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -248,6 +248,12 @@ CREATE FUNCTION test_support_func(internal) AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; + ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) @@ -349,6 +355,86 @@ SELECT explain_mask_costs($$ SELECT * FROM generate_series(25.0, 2.0, 0.0) g(s);$$, false, true, false, true); +-- +-- Test inlining PL/pgSQL functions +-- + +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; + -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); SELECT count(*) > 0 AS ok FROM pg_control_init(); diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index e6f2e93b2d6..8b681e9e44e 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2900,6 +2900,7 @@ SubscriptionRelState SummarizerReadLocalXLogPrivate SupportRequestCost SupportRequestIndexCondition +SupportRequestInlineSRF SupportRequestModifyInPlace SupportRequestOptimizeWindowClause SupportRequestRows -- 2.45.0
^ permalink raw reply [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-11-23 00:44  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Paul A Jungwirth <pj@illuminatedcomputing.com>
  0 siblings, 1 reply; 19+ messages in thread

From: Tom Lane @ 2025-11-23 00:44 UTC (permalink / raw)
  To: Paul A Jungwirth <pj@illuminatedcomputing.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Paul A Jungwirth <pj@illuminatedcomputing.com> writes:
> 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).

I went through this again, and committed it with a bunch of
mostly-cosmetic changes.  In particular, it seemed like talking
about inlining "set-returning functions" is no longer really on-point,
since this mechanism is perfectly capable of inlining non-SRFs.
(The reason we haven't done that for SQL functions is mainly that
we didn't feel like doing the analysis necessary to prove that a
SELECT will return exactly one row, which would be necessary to
maintain semantic equivalence for a non-SRF after inlining.
The easy cases of that, such as "SELECT expression", are already
sufficiently handled by regular inlining.)

So after some thought I renamed inline_set_returning_function to
inline_function_in_from, and made a bunch of other changes in names
and comments to line up with that.

Thanks for working on this!  I know it's been a long slog.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-02 05:52  Pavel Stehule <pavel.stehule@gmail.com>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 2 replies; 19+ messages in thread

From: Pavel Stehule @ 2025-12-02 05:52 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Paul A Jungwirth <pj@illuminatedcomputing.com>; Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Hi

ne 23. 11. 2025 v 1:45 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Paul A Jungwirth <pj@illuminatedcomputing.com> writes:
> > 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).
>
> I went through this again, and committed it with a bunch of
> mostly-cosmetic changes.  In particular, it seemed like talking
> about inlining "set-returning functions" is no longer really on-point,
> since this mechanism is perfectly capable of inlining non-SRFs.
> (The reason we haven't done that for SQL functions is mainly that
> we didn't feel like doing the analysis necessary to prove that a
> SELECT will return exactly one row, which would be necessary to
> maintain semantic equivalence for a non-SRF after inlining.
> The easy cases of that, such as "SELECT expression", are already
> sufficiently handled by regular inlining.)
>
> So after some thought I renamed inline_set_returning_function to
> inline_function_in_from, and made a bunch of other changes in names
> and comments to line up with that.
>
> Thanks for working on this!  I know it's been a long slog.
>

I checked this patch, and I think so using the body of the
function foo_from_bar is very confusing.
If I understand this patch and functionality, then the function is never
executed (when the support function has support
for SupportRequestInlineInFrom).

+CREATE FUNCTION test_inline_in_from_support_func(internal)
+    RETURNS internal
+    AS :'regresslib', 'test_inline_in_from_support_func'
+    LANGUAGE C STRICT;
+CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT)
+RETURNS SETOF TEXT
+LANGUAGE plpgsql
+AS $function$
+DECLARE
+  sql TEXT;
+BEGIN
+  sql := format('SELECT %I::text FROM %I', colname, tablename);
+  IF filter IS NOT NULL THEN
+    sql := CONCAT(sql, format(' WHERE %I::text = $1', colname));
+  END IF;
+  RETURN QUERY EXECUTE sql USING filter;
+END;
+$function$ STABLE;

More correct form should be

BEGIN
  RAISE EXCEPTION 'halt - should not be executed directly';
END;

Regards

Pavel


>
>                         regards, tom lane
>
>
>

^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-02 06:05  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Pavel Stehule <pavel.stehule@gmail.com>
  1 sibling, 1 reply; 19+ messages in thread

From: Tom Lane @ 2025-12-02 06:05 UTC (permalink / raw)
  To: Pavel Stehule <pavel.stehule@gmail.com>; +Cc: Paul A Jungwirth <pj@illuminatedcomputing.com>; Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Pavel Stehule <pavel.stehule@gmail.com> writes:
> ne 23. 11. 2025 v 1:45 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> So after some thought I renamed inline_set_returning_function to
>> inline_function_in_from, and made a bunch of other changes in names
>> and comments to line up with that.

> I checked this patch, and I think so using the body of the
> function foo_from_bar is very confusing.

Perhaps that example could use more documentation effort ...

> More correct form should be
>   RAISE EXCEPTION 'halt - should not be executed directly';

... but I don't agree with this at all.  We specifically do
not guarantee that replacement via SupportRequestSimplify
will be honored, and I think the same must be true for
SupportRequestInlineInFrom.  Otherwise we risk breaking
who-knows-how-many third-party tools, as well as cases
where the optimizer declines to inline (eg, because there
are volatile arguments).  Besides, what's the value?

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-02 06:14  Pavel Stehule <pavel.stehule@gmail.com>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 0 replies; 19+ messages in thread

From: Pavel Stehule @ 2025-12-02 06:14 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Paul A Jungwirth <pj@illuminatedcomputing.com>; Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

út 2. 12. 2025 v 7:05 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > ne 23. 11. 2025 v 1:45 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
> >> So after some thought I renamed inline_set_returning_function to
> >> inline_function_in_from, and made a bunch of other changes in names
> >> and comments to line up with that.
>
> > I checked this patch, and I think so using the body of the
> > function foo_from_bar is very confusing.
>
> Perhaps that example could use more documentation effort ...
>
> > More correct form should be
> >   RAISE EXCEPTION 'halt - should not be executed directly';
>
> ... but I don't agree with this at all.  We specifically do
> not guarantee that replacement via SupportRequestSimplify
> will be honored, and I think the same must be true for
> SupportRequestInlineInFrom.  Otherwise we risk breaking
> who-knows-how-many third-party tools, as well as cases
> where the optimizer declines to inline (eg, because there
> are volatile arguments).  Besides, what's the value?
>

My note is just for this test.

I understand that the planner function can handle only some specific
variants - and all others will be done by real execution.

But this is really confusing - I read the blog
https://www.thatguyfromdelhi.com/2025/11/teaching-query-planner-to-see-inside-c.html
and I was very interested in what is magic in forward, and the reality is
very simple.

I think this feature can be very practical - but can be very messy for
people who don't understand well to complete the picture. And it is a
second level of overloading (what is byself dangerous for someone).

Regards

Pavel






>
>                         regards, tom lane
>

^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-20 04:23  zengman <zengman@halodbtech.com>
  parent: Pavel Stehule <pavel.stehule@gmail.com>
  1 sibling, 2 replies; 19+ messages in thread

From: zengman @ 2025-12-20 04:23 UTC (permalink / raw)
  To: Pavel Stehule <pavel.stehule@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>

Hi everyone,

I've noticed this interesting feature and would like to modify this test case to clarify that we fall back to the original logic when inlining fails. 
This is a small change that doesn't touch core code. What do you all think?

Regards
Man Zeng

Attachments:

0001-misc_functions-Verify-Function-Scan-fallback-on-inli.patch (application/octet-stream, 3.7K) download From 3b55e316c45a1f309864b384d147bd85c19a9579 Mon Sep 17 00:00:00 2001 From: Man Zeng <zengman@halodbtech.com> Date: Sat, 20 Dec 2025 12:09:07 +0800 Subject: [PATCH] misc_functions: Verify Function Scan fallback on inline failure Execution plan falls back to Function Scan (run original PL/pgSQL logic) when inlining fails Signed-off-by: Man Zeng <zengman@halodbtech.com> --- src/test/regress/expected/misc_functions.out | 21 +++++++++++++++++++- src/test/regress/sql/misc_functions.sql | 8 +++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index d7d965d884a..212de3e2783 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -815,13 +815,17 @@ CREATE FUNCTION test_inline_in_from_support_func(internal) RETURNS internal AS :'regresslib', 'test_inline_in_from_support_func' LANGUAGE C STRICT; -CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +CREATE FUNCTION foo_from_bar(colname TEXT DEFAULT NULL, tablename TEXT DEFAULT NULL, filter TEXT DEFAULT NULL) RETURNS SETOF TEXT LANGUAGE plpgsql AS $function$ DECLARE sql TEXT; BEGIN + IF tablename IS NULL AND colname IS NULL THEN + tablename = 'text_tbl'; + colname = 'f1'; + END IF; sql := format('SELECT %I::text FROM %I', colname, tablename); IF filter IS NOT NULL THEN sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); @@ -844,6 +848,14 @@ SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); doh! (1 row) +SELECT * FROM foo_from_bar(); +WARNING: test_inline_in_from_support_func called with non-TEXT parameters + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); QUERY PLAN ---------------------- @@ -857,6 +869,13 @@ EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); Filter: (f1 = 'doh!'::text) (2 rows) +EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar(); +WARNING: test_inline_in_from_support_func called with non-TEXT parameters + QUERY PLAN +------------------------------- + Function Scan on foo_from_bar +(1 row) + DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 0fc20fbb6b4..a4078ee3dfc 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -369,13 +369,17 @@ CREATE FUNCTION test_inline_in_from_support_func(internal) AS :'regresslib', 'test_inline_in_from_support_func' LANGUAGE C STRICT; -CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +CREATE FUNCTION foo_from_bar(colname TEXT DEFAULT NULL, tablename TEXT DEFAULT NULL, filter TEXT DEFAULT NULL) RETURNS SETOF TEXT LANGUAGE plpgsql AS $function$ DECLARE sql TEXT; BEGIN + IF tablename IS NULL AND colname IS NULL THEN + tablename = 'text_tbl'; + colname = 'f1'; + END IF; sql := format('SELECT %I::text FROM %I', colname, tablename); IF filter IS NOT NULL THEN sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); @@ -389,8 +393,10 @@ ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +SELECT * FROM foo_from_bar(); EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar(); DROP FUNCTION foo_from_bar; -- 2.45.2
^ permalink raw reply [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-20 06:57  Pavel Stehule <pavel.stehule@gmail.com>
  parent: zengman <zengman@halodbtech.com>
  1 sibling, 1 reply; 19+ messages in thread

From: Pavel Stehule @ 2025-12-20 06:57 UTC (permalink / raw)
  To: zengman <zengman@halodbtech.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>

so 20. 12. 2025 v 5:23 odesílatel zengman <zengman@halodbtech.com> napsal:

> Hi everyone,
>
> I've noticed this interesting feature and would like to modify this test
> case to clarify that we fall back to the original logic when inlining
> fails.
> This is a small change that doesn't touch core code. What do you all think?
>

+1

I think so message of warning should be enhanced -

"test_inline_in_from_support_func called with non-TEXT parameters" is messy
little bit.

Regards

Pavel



>
> Regards
> Man Zeng

^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-20 10:53  zengman <zengman@halodbtech.com>
  parent: Pavel Stehule <pavel.stehule@gmail.com>
  0 siblings, 1 reply; 19+ messages in thread

From: zengman @ 2025-12-20 10:53 UTC (permalink / raw)
  To: Pavel Stehule <pavel.stehule@gmail.com>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>

I share this opinion. In fact, in the existing tests, if test_inline_in_from_support_func is triggered, 
the input parameters are necessarily of type TEXT―so there’s no need for an additional type check.

How about changing it to the following?
```
inline patch 1: src/test/regress/regress.c (+4/-4 lines) download inline-1.patchdiff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index b7a926c6f01..adb8b9793ba 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -849,9 +849,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) } c = (Const *) node; - if (c->consttype != TEXTOID || c->constisnull) + if (c->constisnull) { - ereport(WARNING, (errmsg("test_inline_in_from_support_func called with non-TEXT parameters"))); + ereport(WARNING, (errmsg("test_inline_in_from_support_func called with null parameter for colname"))); PG_RETURN_POINTER(NULL); } colname = TextDatumGetCString(c->constvalue); @@ -865,9 +865,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) } c = (Const *) node; - if (c->consttype != TEXTOID || c->constisnull) + if (c->constisnull) { - ereport(WARNING, (errmsg("test_inline_in_from_support_func called with non-TEXT parameters"))); + ereport(WARNING, (errmsg("test_inline_in_from_support_func called with null parameter for tablename"))); PG_RETURN_POINTER(NULL); } tablename = TextDatumGetCString(c->constvalue);
``` Regards Man Zeng ^ permalink raw reply [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-20 11:36  Pavel Stehule <pavel.stehule@gmail.com>
  parent: zengman <zengman@halodbtech.com>
  0 siblings, 1 reply; 19+ messages in thread

From: Pavel Stehule @ 2025-12-20 11:36 UTC (permalink / raw)
  To: zengman <zengman@halodbtech.com>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>

so 20. 12. 2025 v 11:53 odesílatel zengman <zengman@halodbtech.com> napsal:

> I share this opinion. In fact, in the existing tests, if
> test_inline_in_from_support_func is triggered,
> the input parameters are necessarily of type TEXT—so there’s no need for
> an additional type check.
>
> How about changing it to the following?
> ```
> diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
> index b7a926c6f01..adb8b9793ba 100644
> --- a/src/test/regress/regress.c
> +++ b/src/test/regress/regress.c
> @@ -849,9 +849,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS)
>                 }
>
>                 c = (Const *) node;
> -               if (c->consttype != TEXTOID || c->constisnull)
> +               if (c->constisnull)
>                 {
> -                       ereport(WARNING,
> (errmsg("test_inline_in_from_support_func called with non-TEXT
> parameters")));
> +                       ereport(WARNING,
> (errmsg("test_inline_in_from_support_func called with null parameter for
> colname")));
>                         PG_RETURN_POINTER(NULL);
>                 }
>                 colname = TextDatumGetCString(c->constvalue);
> @@ -865,9 +865,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS)
>                 }
>
>                 c = (Const *) node;
> -               if (c->consttype != TEXTOID || c->constisnull)
> +               if (c->constisnull)
>                 {
> -                       ereport(WARNING,
> (errmsg("test_inline_in_from_support_func called with non-TEXT
> parameters")));
> +                       ereport(WARNING,
> (errmsg("test_inline_in_from_support_func called with null parameter for
> tablename")));
>                         PG_RETURN_POINTER(NULL);
>                 }
>                 tablename = TextDatumGetCString(c->constvalue);
>

+1

Pavel


> ```
>
> Regards
> Man Zeng

^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-20 12:21  zengman <zengman@halodbtech.com>
  parent: Pavel Stehule <pavel.stehule@gmail.com>
  0 siblings, 0 replies; 19+ messages in thread

From: zengman @ 2025-12-20 12:21 UTC (permalink / raw)
  To: Pavel Stehule <pavel.stehule@gmail.com>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>

Thank you for your guidance. Attached is the updated patch.
I wonder if there are any other adjustments needed and look forward to more feedback.

-- 
Regards
Man Zeng
www.openhalo.org

Attachments:

0002-misc_functions-Verify-Function-Scan-fallback-on-inli.patch (application/octet-stream, 4.9K) download From b7f1ded3512993cb2b71270dd0124af51d2e1936 Mon Sep 17 00:00:00 2001 From: Man Zeng <zengman@halodbtech.com> Date: Sat, 20 Dec 2025 12:09:07 +0800 Subject: [PATCH] misc_functions: Verify inline fallback & fix TEXT check/warnings Verify inline failure falls back to Function Scan (original PL/pgSQL logic) Clean up redundant TEXT check and adjust warnings Signed-off-by: Man Zeng <zengman@halodbtech.com> --- src/test/regress/expected/misc_functions.out | 21 +++++++++++++++++++- src/test/regress/regress.c | 8 ++++---- src/test/regress/sql/misc_functions.sql | 8 +++++++- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index d7d965d884a..44e4450cacd 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -815,13 +815,17 @@ CREATE FUNCTION test_inline_in_from_support_func(internal) RETURNS internal AS :'regresslib', 'test_inline_in_from_support_func' LANGUAGE C STRICT; -CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +CREATE FUNCTION foo_from_bar(colname TEXT DEFAULT NULL, tablename TEXT DEFAULT NULL, filter TEXT DEFAULT NULL) RETURNS SETOF TEXT LANGUAGE plpgsql AS $function$ DECLARE sql TEXT; BEGIN + IF tablename IS NULL AND colname IS NULL THEN + tablename = 'text_tbl'; + colname = 'f1'; + END IF; sql := format('SELECT %I::text FROM %I', colname, tablename); IF filter IS NOT NULL THEN sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); @@ -844,6 +848,14 @@ SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); doh! (1 row) +SELECT * FROM foo_from_bar(); +WARNING: test_inline_in_from_support_func called with null parameter for colname + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); QUERY PLAN ---------------------- @@ -857,6 +869,13 @@ EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); Filter: (f1 = 'doh!'::text) (2 rows) +EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar(); +WARNING: test_inline_in_from_support_func called with null parameter for colname + QUERY PLAN +------------------------------- + Function Scan on foo_from_bar +(1 row) + DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index b7a926c6f01..adb8b9793ba 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -849,9 +849,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) } c = (Const *) node; - if (c->consttype != TEXTOID || c->constisnull) + if (c->constisnull) { - ereport(WARNING, (errmsg("test_inline_in_from_support_func called with non-TEXT parameters"))); + ereport(WARNING, (errmsg("test_inline_in_from_support_func called with null parameter for colname"))); PG_RETURN_POINTER(NULL); } colname = TextDatumGetCString(c->constvalue); @@ -865,9 +865,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) } c = (Const *) node; - if (c->consttype != TEXTOID || c->constisnull) + if (c->constisnull) { - ereport(WARNING, (errmsg("test_inline_in_from_support_func called with non-TEXT parameters"))); + ereport(WARNING, (errmsg("test_inline_in_from_support_func called with null parameter for tablename"))); PG_RETURN_POINTER(NULL); } tablename = TextDatumGetCString(c->constvalue); diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 0fc20fbb6b4..a4078ee3dfc 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -369,13 +369,17 @@ CREATE FUNCTION test_inline_in_from_support_func(internal) AS :'regresslib', 'test_inline_in_from_support_func' LANGUAGE C STRICT; -CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +CREATE FUNCTION foo_from_bar(colname TEXT DEFAULT NULL, tablename TEXT DEFAULT NULL, filter TEXT DEFAULT NULL) RETURNS SETOF TEXT LANGUAGE plpgsql AS $function$ DECLARE sql TEXT; BEGIN + IF tablename IS NULL AND colname IS NULL THEN + tablename = 'text_tbl'; + colname = 'f1'; + END IF; sql := format('SELECT %I::text FROM %I', colname, tablename); IF filter IS NOT NULL THEN sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); @@ -389,8 +393,10 @@ ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +SELECT * FROM foo_from_bar(); EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar(); DROP FUNCTION foo_from_bar; -- 2.45.2
^ permalink raw reply [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-20 21:37  Tom Lane <tgl@sss.pgh.pa.us>
  parent: zengman <zengman@halodbtech.com>
  1 sibling, 1 reply; 19+ messages in thread

From: Tom Lane @ 2025-12-20 21:37 UTC (permalink / raw)
  To: zengman <zengman@halodbtech.com>; +Cc: Pavel Stehule <pavel.stehule@gmail.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>

"=?ISO-8859-1?B?emVuZ21hbg==?=" <zengman@halodbtech.com> writes:
> I've noticed this interesting feature and would like to modify this test case to clarify that we fall back to the original logic when inlining fails. 
> This is a small change that doesn't touch core code. What do you all think?

I think this looks like a waste of test cycles.  AFAICS it won't add
even a single line of code coverage for inline_function_in_from().
Why do you think we need it?

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Inline non-SQL SRFs using SupportRequestSimplify
@ 2025-12-21 02:48  zengman <zengman@halodbtech.com>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 0 replies; 19+ messages in thread

From: zengman @ 2025-12-21 02:48 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Pavel Stehule <pavel.stehule@gmail.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>

Well, from my perspective, the test logic for this feature seems a bit odd: 
even if you directly modify the function body to return NULL; or return 1/0;, 
none of this execution logic takes effect at all―it serves no purpose whatsoever as long as inlining succeeds.
This could be unfriendly to developers who are new to this feature, 
so I added a test for this scenario to verify that our logic falls back to the function body when inlining fails. 
This may just be my personal opinion, though; if you think everything is fine, we can leave it as it is.

--
Regards,
Man Zeng
www.openhalo.org

^ permalink  raw  reply  [nested|flat] 19+ messages in thread


end of thread, other threads:[~2025-12-21 02:48 UTC | newest]

Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-06-27 22:01 Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <pj@illuminatedcomputing.com>
2024-07-02 12:59 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-07-26 18:58   ` Tom Lane <tgl@sss.pgh.pa.us>
2024-08-30 16:26     ` Paul Jungwirth <pj@illuminatedcomputing.com>
2024-09-03 16:42       ` Tom Lane <tgl@sss.pgh.pa.us>
2025-06-30 21:42         ` Paul Jungwirth <pj@illuminatedcomputing.com>
2025-07-14 21:21           ` Tom Lane <tgl@sss.pgh.pa.us>
2025-08-08 21:46             ` Paul A Jungwirth <pj@illuminatedcomputing.com>
2025-11-23 00:44               ` Tom Lane <tgl@sss.pgh.pa.us>
2025-12-02 05:52                 ` Pavel Stehule <pavel.stehule@gmail.com>
2025-12-02 06:05                   ` Tom Lane <tgl@sss.pgh.pa.us>
2025-12-02 06:14                     ` Pavel Stehule <pavel.stehule@gmail.com>
2025-12-20 04:23                   ` zengman <zengman@halodbtech.com>
2025-12-20 06:57                     ` Pavel Stehule <pavel.stehule@gmail.com>
2025-12-20 10:53                       ` zengman <zengman@halodbtech.com>
2025-12-20 11:36                         ` Pavel Stehule <pavel.stehule@gmail.com>
2025-12-20 12:21                           ` zengman <zengman@halodbtech.com>
2025-12-20 21:37                     ` Tom Lane <tgl@sss.pgh.pa.us>
2025-12-21 02:48                       ` zengman <zengman@halodbtech.com>

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox