Query samples in docs and better errors

src/backend/distributed/planner/recursive_planning.c

@@ -164,7 +164,7 @@ static bool CteReferenceListWalker(Node *node, CteReferenceWalkerContext *contex
 static bool ContainsReferencesToOuterQuery(Query *query);
 static bool ContainsReferencesToOuterQueryWalker(Node *node,
 												 VarLevelsUpWalkerContext *context);
-static void WrapFunctionsInQuery(Query *query);
+static void WrapFunctionsInSubqueries(Query *query);
 static void TransformFunctionRTE(RangeTblEntry *rangeTblEntry);
 static bool ShouldTransformRTE(RangeTblEntry *rangeTableEntry);
 
@@ -268,7 +268,7 @@ RecursivelyPlanSubqueriesAndCTEs(Query *query, RecursivePlanningContext *context
 	}
 
 	/* make sure function calls in joins are executed in the coordinator */
-	WrapFunctionsInQuery(query);
+	WrapFunctionsInSubqueries(query);
 
 	/* descend into subqueries */
 	query_tree_walker(query, RecursivelyPlanSubqueryWalker, context, 0);
@@ -1313,14 +1313,14 @@ ContainsReferencesToOuterQueryWalker(Node *node, VarLevelsUpWalkerContext *conte
 
 
 /*
- * WrapFunctionsInQuery iterates over all the immediate Range Table Entries
+ * WrapFunctionsInSubqueries iterates over all the immediate Range Table Entries
  * of a query and wraps the functions inside (SELECT * FROM fnc() f) subqueries.
  *
  * We currently wrap only those functions that return a single value. If a
  * function returns records or a table we leave it as it is
  * */
 static void
-WrapFunctionsInQuery(Query *query)
+WrapFunctionsInSubqueries(Query *query)
 {
 	List *rangeTableList = query->rtable;
 	ListCell *rangeTableCell = NULL;
@@ -1358,7 +1358,7 @@ TransformFunctionRTE(RangeTblEntry *rangeTblEntry)
 	Var *targetColumn = NULL;
 	TargetEntry *targetEntry = NULL;
 	RangeTblFunction *rangeTblFunction = NULL;
-	int targetColumnIndex = 0;
+	AttrNumber targetColumnIndex = 0;
 	TupleDesc tupleDesc = NULL;
 
 	rangeTblFunction = linitial(rangeTblEntry->functions);
@@ -1381,10 +1381,20 @@ TransformFunctionRTE(RangeTblEntry *rangeTblEntry)
 	tupleDesc = (TupleDesc) get_expr_result_tupdesc(rangeTblFunction->funcexpr,
 													true);
 
-	/* if tupleDesc is not null, we iterate over all the attributes and
+	/*
-	 * create targetEntries*/
+	 * If tupleDesc is not null, we iterate over all the attributes and
+	 * create targetEntries
+	 * */
 	if (tupleDesc)
 	{
+		/*
+		 * A sample function join that end up here:
+		 *
+		 * CREATE FUNCTION f(..) RETURNS TABLE(c1 int, c2 text) AS .. ;
+		 * SELECT .. FROM table JOIN f(..) ON ( .. ) ;
+		 *
+		 * We will iterate over Tuple Description attributes. i.e (c1 int, c2 text)
+		 */
 		for (targetColumnIndex = 0; targetColumnIndex < tupleDesc->natts;
 			 targetColumnIndex++)
 		{
@@ -1397,7 +1407,7 @@ TransformFunctionRTE(RangeTblEntry *rangeTblEntry)
 			 * The indexing of attributes and TupleDesc and varattno differ
 			 *
 			 * varattno=0 corresponds to whole row
-			 * varattno=1 corrensponds to first column that is stored in tupDesc->attrs[0] */
+			 * varattno=1 corresponds to first column that is stored in tupDesc->attrs[0] */
 			targetColumn = makeVar(1, targetColumnIndex + 1, columnType, -1, InvalidOid,
 								   0);
 			targetEntry = makeTargetEntry((Expr *) targetColumn, targetColumnIndex + 1,
@@ -1407,13 +1417,13 @@ TransformFunctionRTE(RangeTblEntry *rangeTblEntry)
 		}
 	}
 	/*
-	 * If tupleDesc is NULL we have several cases:
+	 * If tupleDesc is NULL we have 2 different cases:
 	 *
 	 * 1. The function returns a record but the attributes can not be
-	 * determined before running the query. In this case the column names and
+	 * determined just by looking at the function definition. In this case the
-	 * types must be defined explicitly in the query
+	 * column names and types must be defined explicitly in the query
 	 *
-	 * 2. The function returns a non-composite type
+	 * 2. The function returns a non-composite type (e.g. int, text, jsonb ..)
 	 * */
 	else
 	{
@@ -1427,24 +1437,56 @@ TransformFunctionRTE(RangeTblEntry *rangeTblEntry)
 			char *columnName = strVal(lfirst(functionColumnName));
 			Oid columnType = InvalidOid;
 
-			/* use explicitly defined types in the query if they are available */
+			/*
+			 * If the function returns a set of records, the query needs
+			 * to explicitly name column names and types
+			 *
+			 * Use explicitly defined types in the query if they are
+			 * available
+			 * */
 			if (list_length(rangeTblFunction->funccoltypes) > 0)
 			{
+				/*
+				 * A sample function join that end up here:
+				 *
+				 * CREATE FUNCTION get_set_of_records() RETURNS SETOF RECORD AS
+				 * $cmd$
+				 * SELECT x, x+1 FROM generate_series(0,4) f(x)
+				 * $cmd$
+				 * LANGUAGE SQL;
+				 *
+				 * SELECT *
+				 * FROM table1 JOIN get_set_of_records() AS t2(x int, y int)
+				 * ON (id = x);
+				 *
+				 * Note that the function definition does not have column
+				 * names and types. Therefore the user needs to explicitly
+				 * state them in the query
+				 * */
 				columnType = list_nth_oid(rangeTblFunction->funccoltypes,
 										  targetColumnIndex);
 			}
 			/* use the types in the function definition otherwise */
 			else
 			{
+				/*
+				 * Only functions returning simple types end up here.
+				 * A sample function:
+				 *
+				 * CREATE FUNCTION add(integer, integer) RETURNS integer AS
+				 * 'SELECT $1 + $2;'
+				 * LANGUAGE SQL;
+				 * SELECT * FROM table JOIN add(3,5) sum ON ( .. ) ;
+				 * */
 				FuncExpr *funcExpr = (FuncExpr *) rangeTblFunction->funcexpr;
 				columnType = funcExpr->funcresulttype;
 			}
 
+			/* Note that the column k is associated with varattno/resno of k+1 */
 			targetColumn = makeVar(1, targetColumnIndex + 1, columnType, -1,
 								   InvalidOid, 0);
 			targetEntry = makeTargetEntry((Expr *) targetColumn,
-										  targetColumnIndex + 1, columnName,
+										  targetColumnIndex + 1, columnName, false);
-										  false);
 			subquery->targetList = lappend(subquery->targetList, targetEntry);
 
 			targetColumnIndex++;
@@ -1461,33 +1503,22 @@ TransformFunctionRTE(RangeTblEntry *rangeTblEntry)
  * ShouldTransformRTE determines whether a given RTE should bne wrapped in a
  * subquery.
  *
- * Not all functions can be wrapped in a subquery for now. As we support more
+ * Not all functions should be wrapped in a subquery for now. As we support more
  * functions to be used in joins, the constraints here will be relaxed.
  * */
 static bool
 ShouldTransformRTE(RangeTblEntry *rangeTableEntry)
 {
-	/* wrap only function rtes */
-	if (rangeTableEntry->rtekind != RTE_FUNCTION)
-	{
-		return false;
-	}
-
 	/*
-	 * TODO: remove this check once lateral joins are supported
+	 * We should wrap only function rtes that are not LATERAL and
-	 * We do not support lateral joins on functions for now, as referencing
+	 * without WITH CARDINALITY clause
-	 * columns of an outer query is quite tricky */
+	 * */
-	if (rangeTableEntry->lateral)
+	if (rangeTableEntry->rtekind != RTE_FUNCTION ||
+		rangeTableEntry->lateral ||
+		rangeTableEntry->funcordinality)
 	{
 		return false;
 	}
-
-	/* We do not want to wrap read-intermediate-result function calls */
-	if (ContainsReadIntermediateResultFunction(linitial(rangeTableEntry->functions)))
-	{
-		return false;
-	}
-
 	return true;
 }
 

src/include/distributed/version_compat.h

@@ -139,7 +139,7 @@ canonicalize_qual_compat(Expr *qual, bool is_check)
  * A convenient wrapper around get_expr_result_type() that is added on PG11
  *
  * Note that this function ignores the second parameter and behaves
- * slightly differently.
+ * slightly differently than the PG11 version.
  *
  * 1. The original function throws errors if noError flag is not set, we ignore
  * this flag here and return NULL in that case

src/test/regress/expected/multi_function_in_join.out

@@ -6,8 +6,8 @@
 -- that we wrap those functions inside (SELECT * FROM fnc()) sub queries.
 --
 -- We do not yet support those functions that:
---  - return records
+--  - have lateral joins
---  - return tables
+--  - have WITH ORDINALITY clause
 --  - are user-defined and immutable
 CREATE SCHEMA functions_in_joins;
 SET search_path TO 'functions_in_joins';
@@ -202,14 +202,16 @@ SELECT * FROM table1 JOIN the_answer_to_life() the_answer ON (id = the_answer)
 $cmd$);
 ERROR:  Task failed to execute
 CONTEXT:  PL/pgSQL function public.raise_failed_execution(text) line 6 at RAISE
--- WITH ORDINALITY clause forcing the result type to be RECORD/RECORDs
+-- WITH ORDINALITY clause
+SELECT public.raise_failed_execution($cmd$
 SELECT *
 FROM table1
        JOIN next_k_integers(10,5) WITH ORDINALITY next_integers
          ON (id = next_integers.result)
 ORDER BY id ASC;
-ERROR:  attribute 2 of type record has wrong type
+$cmd$);
-DETAIL:  Table has type bigint, but query expects integer.
+ERROR:  Task failed to execute
+CONTEXT:  PL/pgSQL function public.raise_failed_execution(text) line 6 at RAISE
 RESET client_min_messages;
 DROP SCHEMA functions_in_joins CASCADE;
 NOTICE:  drop cascades to 11 other objects

src/test/regress/sql/multi_function_in_join.sql

@@ -6,8 +6,8 @@
 -- that we wrap those functions inside (SELECT * FROM fnc()) sub queries.
 --
 -- We do not yet support those functions that:
---  - return records
+--  - have lateral joins
---  - return tables
+--  - have WITH ORDINALITY clause
 --  - are user-defined and immutable
 
 CREATE SCHEMA functions_in_joins;
@@ -132,12 +132,14 @@ SELECT public.raise_failed_execution($cmd$
 SELECT * FROM table1 JOIN the_answer_to_life() the_answer ON (id = the_answer)
 $cmd$);
 
--- WITH ORDINALITY clause forcing the result type to be RECORD/RECORDs
+-- WITH ORDINALITY clause
+SELECT public.raise_failed_execution($cmd$
 SELECT *
 FROM table1
        JOIN next_k_integers(10,5) WITH ORDINALITY next_integers
          ON (id = next_integers.result)
 ORDER BY id ASC;
+$cmd$);
 
 RESET client_min_messages;
 DROP SCHEMA functions_in_joins CASCADE;