Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Add SupportRequestInlineInFrom planner support request.
- b140c8d7a3f3 19 (unreleased) landed
-
Inline non-SQL SRFs using SupportRequestSimplify
Paul A Jungwirth <pj@illuminatedcomputing.com> — 2024-06-27T22:01:23Z
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 -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-02T12:59:45Z
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) -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-26T18:58:54Z
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 -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Paul A Jungwirth <pj@illuminatedcomputing.com> — 2024-08-30T16:26:56Z
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 -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-03T16:42:01Z
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 -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-06-30T21:42:08Z
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 -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-14T21:21:23Z
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 -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-08-08T21:46:01Z
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 -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-23T00:44:41Z
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
-
Re: Inline non-SQL SRFs using SupportRequestSimplify
Pavel Stehule <pavel.stehule@gmail.com> — 2025-12-02T05:52:21Z
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 > > > -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Tom Lane <tgl@sss.pgh.pa.us> — 2025-12-02T06:05:52Z
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
-
Re: Inline non-SQL SRFs using SupportRequestSimplify
Pavel Stehule <pavel.stehule@gmail.com> — 2025-12-02T06:14:06Z
ú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 >
-
Re: Inline non-SQL SRFs using SupportRequestSimplify
zengman <zengman@halodbtech.com> — 2025-12-20T04:23:41Z
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
-
Re: Inline non-SQL SRFs using SupportRequestSimplify
Pavel Stehule <pavel.stehule@gmail.com> — 2025-12-20T06:57:25Z
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
-
Re: Inline non-SQL SRFs using SupportRequestSimplify
zengman <zengman@halodbtech.com> — 2025-12-20T10:53:17Z
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); ``` Regards Man Zeng -
Re: Inline non-SQL SRFs using SupportRequestSimplify
Pavel Stehule <pavel.stehule@gmail.com> — 2025-12-20T11:36:11Z
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 -
Re: Inline non-SQL SRFs using SupportRequestSimplify
zengman <zengman@halodbtech.com> — 2025-12-20T12:21:39Z
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
-
Re: Inline non-SQL SRFs using SupportRequestSimplify
Tom Lane <tgl@sss.pgh.pa.us> — 2025-12-20T21:37:39Z
"=?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
-
Re: Inline non-SQL SRFs using SupportRequestSimplify
zengman <zengman@halodbtech.com> — 2025-12-21T02:48:07Z
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