Date: Thu, 6 Sep 2018 18:51:09 +0700
From: Aleksandr Parfenov <a.parfenov@postgrespro.ru>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Andrew Gierth
 <andrew@tao11.riddles.org.uk>, pgsql-hackers@lists.postgresql.org
Subject: Re: Optimze usage of immutable functions as relation
Message-ID: <20180906185109.661aaef0@asp437-ThinkPad-L380>
In-Reply-To: <32012.1531258460@sss.pgh.pa.us>
References: <402356c32eeb93d4fed01f66d6c7fe2d@postgrespro.ru>
	<87zi1g6jbj.fsf@news-spur.riddles.org.uk>
	<20180508125616.7ebb4d12@asp437-24-g082ur>
	<87po2679in.fsf@news-spur.riddles.org.uk>
	<20180516134746.0296a05f@asp437-g082ur>
	<9f127876-2599-0485-c5ef-a5d8d7402c85@iki.fi>
	<32012.1531258460@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/FcrfJJa/Aa9mOta+T+WMm5a"
Precedence: bulk

--MP_/FcrfJJa/Aa9mOta+T+WMm5a
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

On Tue, 10 Jul 2018 17:34:20 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

>Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> But stepping back a bit, it's a bit weird that we're handling this 
>> differently from VALUES and other subqueries. The planner knows how
>> to do this trick for simple subqueries:  
>
>> postgres=# explain select * from tenk1, (select abs(100)) as a (a)
>> where unique1 < a;
>>                          QUERY PLAN
>> -----------------------------------------------------------
>>   Seq Scan on tenk1  (cost=0.00..483.00 rows=100 width=248)
>>     Filter: (unique1 < 100)
>> (2 rows)  
>
>> Note that it not only evaluated the function into a constant, but
>> also got rid of the join. For a function RTE, however, it can't do
>> that:  
>
>> postgres=# explain select * from tenk1, abs(100) as a (a) where
>> unique1 < a; QUERY PLAN
>> -------------------------------------------------------------------
>>   Nested Loop  (cost=0.00..583.01 rows=3333 width=248)
>>     Join Filter: (tenk1.unique1 < a.a)  
>>     ->  Function Scan on a  (cost=0.00..0.01 rows=1 width=4)
>>     ->  Seq Scan on tenk1  (cost=0.00..458.00 rows=10000 width=244)  
>> (4 rows)  
>
>> Could we handle this in pull_up_subqueries(), similar to the 
>> RTE_SUBQUERY and RTE_VALUES cases?  
>
>Perhaps.  You could only do it for non-set-returning functions, which
>isn't the typical use of function RTEs, which is probably why we've not
>thought hard about it before.  I'm not sure what would need to happen
>for lateral functions.  Also to be considered, if it's not foldable to
>a constant, is whether we're risking evaluating it more times than
>before.
>
>			regards, tom lane

I reworked the patch and implemented processing of FuncScan in
pull_up_subqueries() in a way similar to VALUES processing. In order to
prevent folding of non-foldable functions it checks provolatile of the
function and are arguments const or not and return type to prevent
folding of SRF.

-- 
Aleksandr Parfenov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--MP_/FcrfJJa/Aa9mOta+T+WMm5a
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename=funcscan_plan_optimizer_v3.patch

diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index c3f46a26c3..25539bbfae 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -23,7 +23,9 @@
  */
 #include "postgres.h"
 
+#include "access/htup_details.h"
 #include "catalog/pg_type.h"
+#include "catalog/pg_proc.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/clauses.h"
@@ -35,6 +37,8 @@
 #include "parser/parse_relation.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteManip.h"
+#include "utils/syscache.h"
+#include "utils/lsyscache.h"
 
 
 typedef struct pullup_replace_vars_context
@@ -86,6 +90,8 @@ static bool is_simple_subquery(Query *subquery, RangeTblEntry *rte,
 				   bool deletion_ok);
 static Node *pull_up_simple_values(PlannerInfo *root, Node *jtnode,
 					  RangeTblEntry *rte);
+static Node *pull_up_simple_function(PlannerInfo *root, Node *jtnode,
+					  RangeTblEntry *rte);
 static bool is_simple_values(PlannerInfo *root, RangeTblEntry *rte,
 				 bool deletion_ok);
 static bool is_simple_union_all(Query *subquery);
@@ -595,6 +601,54 @@ inline_set_returning_functions(PlannerInfo *root)
 	}
 }
 
+static bool
+is_simple_stable_function(RangeTblEntry *rte)
+{
+	Form_pg_type type_form;
+	RangeTblFunction *tblFunction = linitial_node(RangeTblFunction, rte->functions);
+	FuncExpr   *expr = (FuncExpr *) tblFunction->funcexpr;
+	HeapTuple tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(expr->funcresulttype));
+	bool		result = false;
+
+	if (!HeapTupleIsValid(tuple))
+		elog(ERROR, "cache lookup failed for type %u", expr->funcresulttype);
+
+	type_form = (Form_pg_type) GETSTRUCT(tuple);
+
+	if (type_form->typtype == TYPTYPE_BASE &&
+		!type_is_array(expr->funcresulttype))
+	{
+		Form_pg_proc func_form;
+		ListCell   *arg;
+		bool		has_nonconst_input = false;
+		bool		has_null_input = false;
+
+		ReleaseSysCache(tuple);
+		tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(expr->funcid));
+		if (!HeapTupleIsValid(tuple))
+			elog(ERROR, "cache lookup failed for function %u", expr->funcid);
+		func_form = (Form_pg_proc) GETSTRUCT(tuple);
+
+		foreach(arg, expr->args)
+		{
+			if (IsA(lfirst(arg), Const))
+				has_null_input |= ((Const *) lfirst(arg))->constisnull;
+			else
+				has_nonconst_input = true;
+		}
+
+		result = func_form->prorettype != RECORDOID &&
+				 func_form->prokind == PROKIND_FUNCTION &&
+				 !func_form->proretset &&
+				 func_form->provolatile == PROVOLATILE_IMMUTABLE &&
+				 !has_null_input &&
+				 !has_nonconst_input;
+	}
+
+	ReleaseSysCache(tuple);
+	return result;
+}
+
 /*
  * pull_up_subqueries
  *		Look for subqueries in the rangetable that can be pulled up into
@@ -725,6 +779,11 @@ pull_up_subqueries_recurse(PlannerInfo *root, Node *jtnode,
 			is_simple_values(root, rte, deletion_ok))
 			return pull_up_simple_values(root, jtnode, rte);
 
+		if (rte->rtekind == RTE_FUNCTION &&
+				list_length(rte->functions) == 1 &&
+				is_simple_stable_function(rte))
+			return pull_up_simple_function(root, jtnode, rte);
+
 		/* Otherwise, do nothing at this node. */
 	}
 	else if (IsA(jtnode, FromExpr))
@@ -1707,6 +1766,107 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
 	return NULL;
 }
 
+static Node *
+pull_up_simple_function(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
+{
+	Query	   *parse = root->parse;
+	int			varno = ((RangeTblRef *) jtnode)->rtindex;
+	List	   *functions_list;
+	List	   *tlist;
+	AttrNumber	attrno;
+	pullup_replace_vars_context rvcontext;
+	ListCell   *lc;
+
+	Assert(rte->rtekind == RTE_FUNCTION);
+	Assert(list_length(rte->functions) == 1);
+
+	/*
+	 * Need a modifiable copy of the functions list to hack on, just in case it's
+	 * multiply referenced.
+	 */
+	functions_list = copyObject(rte->functions);
+
+	/*
+	 * The FUNCTION RTE can't contain any Vars of level zero, let alone any that
+	 * are join aliases, so no need to flatten join alias Vars.
+	 */
+	Assert(!contain_vars_of_level((Node *) functions, 0));
+
+	/*
+	 * Set up required context data for pullup_replace_vars.  In particular,
+	 * we have to make the VALUES list look like a subquery targetlist.
+	 */
+	tlist = NIL;
+	attrno = 1;
+	foreach(lc, functions_list)
+	{
+		RangeTblFunction *rtf = (RangeTblFunction *)lfirst(lc);
+		tlist = lappend(tlist,
+						makeTargetEntry((Expr *) rtf->funcexpr,
+										attrno,
+										NULL,
+										false));
+		attrno++;
+	}
+	rvcontext.root = root;
+	rvcontext.targetlist = tlist;
+	rvcontext.target_rte = rte;
+	rvcontext.relids = NULL;
+	rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
+	rvcontext.varno = varno;
+	rvcontext.need_phvs = false;
+	rvcontext.wrap_non_vars = false;
+	/* initialize cache array with indexes 0 .. length(tlist) */
+	rvcontext.rv_cache = palloc0((list_length(tlist) + 1) *
+								 sizeof(Node *));
+
+	/*
+	 * Replace all of the top query's references to the RTE's outputs with
+	 * copies of the adjusted VALUES expressions, being careful not to replace
+	 * any of the jointree structure. (This'd be a lot cleaner if we could use
+	 * query_tree_mutator.)  Much of this should be no-ops in the dummy Query
+	 * that surrounds a VALUES RTE, but it's not enough code to be worth
+	 * removing.
+	 */
+	parse->targetList = (List *)
+		pullup_replace_vars((Node *) parse->targetList, &rvcontext);
+	parse->returningList = (List *)
+		pullup_replace_vars((Node *) parse->returningList, &rvcontext);
+	if (parse->onConflict)
+	{
+		parse->onConflict->onConflictSet = (List *)
+			pullup_replace_vars((Node *) parse->onConflict->onConflictSet,
+								&rvcontext);
+		parse->onConflict->onConflictWhere =
+			pullup_replace_vars(parse->onConflict->onConflictWhere,
+								&rvcontext);
+
+		/*
+		 * We assume ON CONFLICT's arbiterElems, arbiterWhere, exclRelTlist
+		 * can't contain any references to a subquery
+		 */
+	}
+	replace_vars_in_jointree((Node *) parse->jointree, &rvcontext, NULL);
+	Assert(parse->setOperations == NULL);
+	parse->havingQual = pullup_replace_vars(parse->havingQual, &rvcontext);
+
+	/*
+	 * There should be no appendrels to fix, nor any join alias Vars, nor any
+	 * outer joins and hence no PlaceHolderVars.
+	 */
+	Assert(root->append_rel_list == NIL);
+	Assert(list_length(parse->rtable) == 1);
+	Assert(root->join_info_list == NIL);
+	Assert(root->placeholder_list == NIL);
+
+	/*
+	 * Return NULL to signal deletion of the VALUES RTE from the parent
+	 * jointree (and set hasDeletedRTEs to ensure cleanup later).
+	 */
+	root->hasDeletedRTEs = true;
+	return NULL;
+}
+
 /*
  * is_simple_values
  *	  Check a VALUES RTE in the range table to see if it's simple enough

--MP_/FcrfJJa/Aa9mOta+T+WMm5a--