Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

On 2016-09-13 12:07:35 -0400, Tom Lane wrote:
> diff --git a/src/backend/optimizer/plan/analindex e28a8dc..74e4245 100644
> --- a/src/backend/optimizer/plan/analyzejoins.c
> +++ b/src/backend/optimizer/plan/analyzejoins.c
> @@ -650,6 +650,11 @@ rel_is_distinct_for(PlannerInfo *root, R
> bool
> query_supports_distinctness(Query *query)
> {
> + /* we don't cope with SRFs, see comment below */
> + if (query->hasTargetSRFs)
> + return false;
> +
> + /* check for features we can prove distinctness with */
> if (query->distinctClause != NIL ||
> query->groupClause != NIL ||
> query->groupingSets != NIL ||
> @@ -695,7 +700,7 @@ query_is_distinct_for(Query *query, List
> * specified columns, since those must be evaluated before de-duplication;
> * but it doesn't presently seem worth the complication to check that.)
> */
> - if (expression_returns_set((Node *) query->targetList))
> + if (query->hasTargetSRFs)
> return false;

Maybe make this hasTargetSRFs && expression_returns_set()? The SRF could
have been optimized away. (Oh, I see you recheck below. Forget that then).

> @@ -1419,8 +1419,8 @@ is_simple_subquery(Query *subquery, Rang
> return false;
>
> /*
> - * Can't pull up a subquery involving grouping, aggregation, sorting,
> - * limiting, or WITH. (XXX WITH could possibly be allowed later)
> + * Can't pull up a subquery involving grouping, aggregation, SRFs,
> + * sorting, limiting, or WITH. (XXX WITH could possibly be allowed later)
> *
> * We also don't pull up a subquery that has explicit FOR UPDATE/SHARE
> * clauses, because pullup would cause the locking to occur semantically
> @@ -1430,6 +1430,7 @@ is_simple_subquery(Query *subquery, Rang
> */
> if (subquery->hasAggs ||
> subquery->hasWindowFuncs ||
> + subquery->hasTargetSRFs ||
> subquery->groupClause ||
> subquery->groupingSets ||
> subquery->havingQual ||
> @@ -1543,15 +1544,6 @@ is_simple_subquery(Query *subquery, Rang
> }
>
> /*
> - * Don't pull up a subquery that has any set-returning functions in its
> - * targetlist. Otherwise we might well wind up inserting set-returning
> - * functions into places where they mustn't go, such as quals of higher
> - * queries. This also ensures deletion of an empty jointree is valid.
> - */
> - if (expression_returns_set((Node *) subquery->targetList))
> - return false;

I don't quite understand parts of the comment you removed here. What
does "This also ensures deletion of an empty jointree is valid." mean?

Looks good, except that you didn't adopt the hunk adjusting
src/backend/executor/README, which still seems to read:
> We disallow set-returning functions in the targetlist of SELECT FOR UPDATE,
> so as to ensure that at most one tuple can be returned for any particular
> set of scan tuples. Otherwise we'd get duplicates due to the original
> query returning the same set of scan tuples multiple times. (Note: there
> is no explicit prohibition on SRFs in UPDATE, but the net effect will be
> that only the first result row of an SRF counts, because all subsequent
> rows will result in attempts to re-update an already updated target row.
> This is historical behavior and seems not worth changing.)

Regards,

Andres