public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Paul Jungwirth <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Inline non-SQL SRFs using SupportRequestSimplify
Date: Mon, 14 Jul 2025 17:21:23 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Paul Jungwirth <[email protected]> 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

view thread (19+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Inline non-SQL SRFs using SupportRequestSimplify
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

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