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

Andres Freund <andres@anarazel.de> writes:
> Attached is a significantly updated patch series (see the mail one up
> for details about what this is, I don't want to quote it in its
> entirety).

I've reviewed the portions of 0005 that have to do with making the parser
mark queries with hasTargetSRF.  The code as you had it was wrong because
it would set the flag as a consequence of SRFs in function RTEs, which
we don't want.  It seemed to me that the best way to fix that was to rely
on the parser's p_expr_kind mechanism to tell which part of the query
we're in, whereupon we might as well make the parser act more like it does
for aggregates and window functions and give a suitable error at parse
time for misplaced SRFs.  The attached isn't perfect, in that it doesn't
know about nesting restrictions (ie that SRFs must be at top level of a
function RTE), but we could improve that later if we wanted, and anyway
it's definitely a good bit nicer than before.

This also incorporates the part of 0002 that I agree with, namely
disallowing SRFs in UPDATE, since check_srf_call_placement() naturally
would do that.

I also renamed the flag to hasTargetSRFs, which is more parallel to
hasAggs and hasWindowFuncs, and made some effort to use it in place
of expression_returns_set() searches.

I'd like to go ahead and push this, since it's a necessary prerequisite
for either approach we might adopt for the rest of the patch series,
and the improved error reporting and avoidance of expensive
expression_returns_set searches make it a win IMO even if we were not
planning to do anything more with SRFs.

            regards, tom lane

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index e997b57..dbd6094 100644
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
*************** cookDefault(ParseState *pstate,
*** 2560,2573 ****

      /*
       * transformExpr() should have already rejected subqueries, aggregates,
!      * and window functions, based on the EXPR_KIND_ for a default expression.
!      *
!      * It can't return a set either.
       */
-     if (expression_returns_set(expr))
-         ereport(ERROR,
-                 (errcode(ERRCODE_DATATYPE_MISMATCH),
-                  errmsg("default expression must not return a set")));

      /*
       * Coerce the expression to the correct type and typmod, if given. This
--- 2560,2568 ----

      /*
       * transformExpr() should have already rejected subqueries, aggregates,
!      * window functions, and SRFs, based on the EXPR_KIND_ for a default
!      * expression.
       */

      /*
       * Coerce the expression to the correct type and typmod, if given. This
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 4f39dad..71714bc 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
*************** _copyQuery(const Query *from)
*** 2731,2736 ****
--- 2731,2737 ----
      COPY_SCALAR_FIELD(resultRelation);
      COPY_SCALAR_FIELD(hasAggs);
      COPY_SCALAR_FIELD(hasWindowFuncs);
+     COPY_SCALAR_FIELD(hasTargetSRFs);
      COPY_SCALAR_FIELD(hasSubLinks);
      COPY_SCALAR_FIELD(hasDistinctOn);
      COPY_SCALAR_FIELD(hasRecursive);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 4800165..29a090f 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
*************** _equalQuery(const Query *a, const Query
*** 921,926 ****
--- 921,927 ----
      COMPARE_SCALAR_FIELD(resultRelation);
      COMPARE_SCALAR_FIELD(hasAggs);
      COMPARE_SCALAR_FIELD(hasWindowFuncs);
+     COMPARE_SCALAR_FIELD(hasTargetSRFs);
      COMPARE_SCALAR_FIELD(hasSubLinks);
      COMPARE_SCALAR_FIELD(hasDistinctOn);
      COMPARE_SCALAR_FIELD(hasRecursive);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 90fecb1..7e092d7 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
*************** _outQuery(StringInfo str, const Query *n
*** 2683,2688 ****
--- 2683,2689 ----
      WRITE_INT_FIELD(resultRelation);
      WRITE_BOOL_FIELD(hasAggs);
      WRITE_BOOL_FIELD(hasWindowFuncs);
+     WRITE_BOOL_FIELD(hasTargetSRFs);
      WRITE_BOOL_FIELD(hasSubLinks);
      WRITE_BOOL_FIELD(hasDistinctOn);
      WRITE_BOOL_FIELD(hasRecursive);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 894a48f..917e6c8 100644
*** a/src/backend/nodes/readfuncs.c
--- b/src/backend/nodes/readfuncs.c
*************** _readQuery(void)
*** 238,243 ****
--- 238,244 ----
      READ_INT_FIELD(resultRelation);
      READ_BOOL_FIELD(hasAggs);
      READ_BOOL_FIELD(hasWindowFuncs);
+     READ_BOOL_FIELD(hasTargetSRFs);
      READ_BOOL_FIELD(hasSubLinks);
      READ_BOOL_FIELD(hasDistinctOn);
      READ_BOOL_FIELD(hasRecursive);
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 04264b4..99b6bc8 100644
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
*************** check_output_expressions(Query *subquery
*** 2422,2428 ****
              continue;

          /* Functions returning sets are unsafe (point 1) */
!         if (expression_returns_set((Node *) tle->expr))
          {
              safetyInfo->unsafeColumns[tle->resno] = true;
              continue;
--- 2422,2429 ----
              continue;

          /* Functions returning sets are unsafe (point 1) */
!         if (subquery->hasTargetSRFs &&
!             expression_returns_set((Node *) tle->expr))
          {
              safetyInfo->unsafeColumns[tle->resno] = true;
              continue;
*************** remove_unused_subquery_outputs(Query *su
*** 2835,2841 ****
           * If it contains a set-returning function, we can't remove it since
           * that could change the number of rows returned by the subquery.
           */
!         if (expression_returns_set(texpr))
              continue;

          /*
--- 2836,2843 ----
           * If it contains a set-returning function, we can't remove it since
           * that could change the number of rows returned by the subquery.
           */
!         if (subquery->hasTargetSRFs &&
!             expression_returns_set(texpr))
              continue;

          /*
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index e28a8dc..74e4245 100644
*** a/src/backend/optimizer/plan/analyzejoins.c
--- b/src/backend/optimizer/plan/analyzejoins.c
*************** rel_is_distinct_for(PlannerInfo *root, R
*** 650,655 ****
--- 650,660 ----
  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 ||
*************** query_is_distinct_for(Query *query, List
*** 695,701 ****
       * 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))
          return false;

      /*
--- 700,706 ----
       * specified columns, since those must be evaluated before de-duplication;
       * but it doesn't presently seem worth the complication to check that.)
       */
!     if (query->hasTargetSRFs)
          return false;

      /*
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 174210b..f657ffc 100644
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** subquery_planner(PlannerGlobal *glob, Qu
*** 604,609 ****
--- 604,613 ----
          preprocess_expression(root, (Node *) parse->targetList,
                                EXPRKIND_TARGET);

+     /* Constant-folding might have removed all set-returning functions */
+     if (parse->hasTargetSRFs)
+         parse->hasTargetSRFs = expression_returns_set((Node *) parse->targetList);
+
      newWithCheckOptions = NIL;
      foreach(l, parse->withCheckOptions)
      {
*************** grouping_planner(PlannerInfo *root, bool
*** 1702,1717 ****
           * Figure out whether there's a hard limit on the number of rows that
           * query_planner's result subplan needs to return.  Even if we know a
           * hard limit overall, it doesn't apply if the query has any
!          * grouping/aggregation operations.  (XXX it also doesn't apply if the
!          * tlist contains any SRFs; but checking for that here seems more
!          * costly than it's worth, since root->limit_tuples is only used for
!          * cost estimates, and only in a small number of cases.)
           */
          if (parse->groupClause ||
              parse->groupingSets ||
              parse->distinctClause ||
              parse->hasAggs ||
              parse->hasWindowFuncs ||
              root->hasHavingQual)
              root->limit_tuples = -1.0;
          else
--- 1706,1719 ----
           * Figure out whether there's a hard limit on the number of rows that
           * query_planner's result subplan needs to return.  Even if we know a
           * hard limit overall, it doesn't apply if the query has any
!          * grouping/aggregation operations, or SRFs in the tlist.
           */
          if (parse->groupClause ||
              parse->groupingSets ||
              parse->distinctClause ||
              parse->hasAggs ||
              parse->hasWindowFuncs ||
+             parse->hasTargetSRFs ||
              root->hasHavingQual)
              root->limit_tuples = -1.0;
          else
*************** grouping_planner(PlannerInfo *root, bool
*** 1928,1934 ****
       * weird usage that it doesn't seem worth greatly complicating matters to
       * account for it.
       */
!     tlist_rows = tlist_returns_set_rows(tlist);
      if (tlist_rows > 1)
      {
          foreach(lc, current_rel->pathlist)
--- 1930,1940 ----
       * weird usage that it doesn't seem worth greatly complicating matters to
       * account for it.
       */
!     if (parse->hasTargetSRFs)
!         tlist_rows = tlist_returns_set_rows(tlist);
!     else
!         tlist_rows = 1;
!
      if (tlist_rows > 1)
      {
          foreach(lc, current_rel->pathlist)
*************** make_sort_input_target(PlannerInfo *root
*** 4995,5001 ****
               * Check for SRF or volatile functions.  Check the SRF case first
               * because we must know whether we have any postponed SRFs.
               */
!             if (expression_returns_set((Node *) expr))
              {
                  /* We'll decide below whether these are postponable */
                  col_is_srf[i] = true;
--- 5001,5008 ----
               * Check for SRF or volatile functions.  Check the SRF case first
               * because we must know whether we have any postponed SRFs.
               */
!             if (parse->hasTargetSRFs &&
!                 expression_returns_set((Node *) expr))
              {
                  /* We'll decide below whether these are postponable */
                  col_is_srf[i] = true;
*************** make_sort_input_target(PlannerInfo *root
*** 5034,5039 ****
--- 5041,5047 ----
          {
              /* For sortgroupref cols, just check if any contain SRFs */
              if (!have_srf_sortcols &&
+                 parse->hasTargetSRFs &&
                  expression_returns_set((Node *) expr))
                  have_srf_sortcols = true;
          }
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 6edefb1..b5d3e94 100644
*** a/src/backend/optimizer/plan/subselect.c
--- b/src/backend/optimizer/plan/subselect.c
*************** simplify_EXISTS_query(PlannerInfo *root,
*** 1562,1568 ****
  {
      /*
       * We don't try to simplify at all if the query uses set operations,
!      * aggregates, grouping sets, modifying CTEs, HAVING, OFFSET, or FOR
       * UPDATE/SHARE; none of these seem likely in normal usage and their
       * possible effects are complex.  (Note: we could ignore an "OFFSET 0"
       * clause, but that traditionally is used as an optimization fence, so we
--- 1562,1568 ----
  {
      /*
       * We don't try to simplify at all if the query uses set operations,
!      * aggregates, SRFs, grouping sets, modifying CTEs, HAVING, OFFSET, or FOR
       * UPDATE/SHARE; none of these seem likely in normal usage and their
       * possible effects are complex.  (Note: we could ignore an "OFFSET 0"
       * clause, but that traditionally is used as an optimization fence, so we
*************** simplify_EXISTS_query(PlannerInfo *root,
*** 1573,1578 ****
--- 1573,1579 ----
          query->hasAggs ||
          query->groupingSets ||
          query->hasWindowFuncs ||
+         query->hasTargetSRFs ||
          query->hasModifyingCTE ||
          query->havingQual ||
          query->limitOffset ||
*************** simplify_EXISTS_query(PlannerInfo *root,
*** 1614,1626 ****
      }

      /*
-      * Mustn't throw away the targetlist if it contains set-returning
-      * functions; those could affect whether zero rows are returned!
-      */
-     if (expression_returns_set((Node *) query->targetList))
-         return false;
-
-     /*
       * Otherwise, we can throw away the targetlist, as well as any GROUP,
       * WINDOW, DISTINCT, and ORDER BY clauses; none of those clauses will
       * change a nonzero-rows result to zero rows or vice versa.  (Furthermore,
--- 1615,1620 ----
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index a334f15..878db9b 100644
*** a/src/backend/optimizer/prep/prepjointree.c
--- b/src/backend/optimizer/prep/prepjointree.c
*************** pull_up_simple_subquery(PlannerInfo *roo
*** 1188,1195 ****
      parse->hasSubLinks |= subquery->hasSubLinks;

      /*
!      * subquery won't be pulled up if it hasAggs or hasWindowFuncs, so no work
!      * needed on those flags
       */

      /*
--- 1188,1195 ----
      parse->hasSubLinks |= subquery->hasSubLinks;

      /*
!      * subquery won't be pulled up if it hasAggs, hasWindowFuncs, or
!      * hasTargetSRFs, so no work needed on those flags
       */

      /*
*************** is_simple_subquery(Query *subquery, Rang
*** 1419,1426 ****
          return false;

      /*
!      * Can't pull up a subquery involving grouping, aggregation, 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
--- 1419,1426 ----
          return false;

      /*
!      * 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
*************** is_simple_subquery(Query *subquery, Rang
*** 1430,1435 ****
--- 1430,1436 ----
       */
      if (subquery->hasAggs ||
          subquery->hasWindowFuncs ||
+         subquery->hasTargetSRFs ||
          subquery->groupClause ||
          subquery->groupingSets ||
          subquery->havingQual ||
*************** is_simple_subquery(Query *subquery, Rang
*** 1543,1557 ****
      }

      /*
-      * 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;
-
-     /*
       * Don't pull up a subquery that has any volatile functions in its
       * targetlist.  Otherwise we might introduce multiple evaluations of these
       * functions, if they get copied to multiple places in the upper query,
--- 1544,1549 ----
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index e1baf71..663ffe0 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** inline_function(Oid funcid, Oid result_t
*** 4449,4454 ****
--- 4449,4455 ----
          querytree->utilityStmt ||
          querytree->hasAggs ||
          querytree->hasWindowFuncs ||
+         querytree->hasTargetSRFs ||
          querytree->hasSubLinks ||
          querytree->cteList ||
          querytree->rtable ||
*************** inline_function(Oid funcid, Oid result_t
*** 4489,4505 ****
      Assert(!modifyTargetList);

      /*
!      * Additional validity checks on the expression.  It mustn't return a set,
!      * and it mustn't be more volatile than the surrounding function (this is
!      * to avoid breaking hacks that involve pretending a function is immutable
!      * when it really ain't).  If the surrounding function is declared strict,
!      * then the expression must contain only strict constructs and must use
!      * all of the function parameters (this is overkill, but an exact analysis
!      * is hard).
       */
-     if (expression_returns_set(newexpr))
-         goto fail;
-
      if (funcform->provolatile == PROVOLATILE_IMMUTABLE &&
          contain_mutable_functions(newexpr))
          goto fail;
--- 4490,4502 ----
      Assert(!modifyTargetList);

      /*
!      * Additional validity checks on the expression.  It mustn't be more
!      * volatile than the surrounding function (this is to avoid breaking hacks
!      * that involve pretending a function is immutable when it really ain't).
!      * If the surrounding function is declared strict, then the expression
!      * must contain only strict constructs and must use all of the function
!      * parameters (this is overkill, but an exact analysis is hard).
       */
      if (funcform->provolatile == PROVOLATILE_IMMUTABLE &&
          contain_mutable_functions(newexpr))
          goto fail;
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index eac86cc..870fae3 100644
*** a/src/backend/parser/analyze.c
--- b/src/backend/parser/analyze.c
*************** transformDeleteStmt(ParseState *pstate,
*** 417,422 ****
--- 417,423 ----

      qry->hasSubLinks = pstate->p_hasSubLinks;
      qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
+     qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
      qry->hasAggs = pstate->p_hasAggs;
      if (pstate->p_hasAggs)
          parseCheckAggregates(pstate, qry);
*************** transformInsertStmt(ParseState *pstate,
*** 819,824 ****
--- 820,826 ----
      qry->rtable = pstate->p_rtable;
      qry->jointree = makeFromExpr(pstate->p_joinlist, NULL);

+     qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
      qry->hasSubLinks = pstate->p_hasSubLinks;

      assign_query_collations(pstate, qry);
*************** transformSelectStmt(ParseState *pstate,
*** 1231,1236 ****
--- 1233,1239 ----

      qry->hasSubLinks = pstate->p_hasSubLinks;
      qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
+     qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
      qry->hasAggs = pstate->p_hasAggs;
      if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
          parseCheckAggregates(pstate, qry);
*************** transformSetOperationStmt(ParseState *ps
*** 1691,1696 ****
--- 1694,1700 ----

      qry->hasSubLinks = pstate->p_hasSubLinks;
      qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
+     qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
      qry->hasAggs = pstate->p_hasAggs;
      if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
          parseCheckAggregates(pstate, qry);
*************** transformUpdateStmt(ParseState *pstate,
*** 2170,2175 ****
--- 2174,2180 ----
      qry->rtable = pstate->p_rtable;
      qry->jointree = makeFromExpr(pstate->p_joinlist, qual);

+     qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
      qry->hasSubLinks = pstate->p_hasSubLinks;

      assign_query_collations(pstate, qry);
*************** CheckSelectLocking(Query *qry, LockClaus
*** 2565,2571 ****
            translator: %s is a SQL row locking clause such as FOR UPDATE */
                   errmsg("%s is not allowed with window functions",
                          LCS_asString(strength))));
!     if (expression_returns_set((Node *) qry->targetList))
          ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
          /*------
--- 2570,2576 ----
            translator: %s is a SQL row locking clause such as FOR UPDATE */
                   errmsg("%s is not allowed with window functions",
                          LCS_asString(strength))));
!     if (qry->hasTargetSRFs)
          ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
          /*------
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 61af484..56c9a42 100644
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
***************
*** 25,30 ****
--- 25,31 ----
  #include "parser/parse_agg.h"
  #include "parser/parse_clause.h"
  #include "parser/parse_coerce.h"
+ #include "parser/parse_expr.h"
  #include "parser/parse_func.h"
  #include "parser/parse_relation.h"
  #include "parser/parse_target.h"
*************** ParseFuncOrColumn(ParseState *pstate, Li
*** 625,630 ****
--- 626,635 ----
                                        exprLocation((Node *) llast(fargs)))));
      }

+     /* if it returns a set, check that's OK */
+     if (retset)
+         check_srf_call_placement(pstate, location);
+
      /* build the appropriate output structure */
      if (fdresult == FUNCDETAIL_NORMAL)
      {
*************** LookupAggNameTypeNames(List *aggname, Li
*** 2040,2042 ****
--- 2045,2190 ----

      return oid;
  }
+
+
+ /*
+  * check_srf_call_placement
+  *        Verify that a set-returning function is called in a valid place,
+  *        and throw a nice error if not.
+  *
+  * A side-effect is to set pstate->p_hasTargetSRFs true if appropriate.
+  */
+ void
+ check_srf_call_placement(ParseState *pstate, int location)
+ {
+     const char *err;
+     bool        errkind;
+
+     /*
+      * Check to see if the set-returning function is in an invalid place
+      * within the query.  Basically, we don't allow SRFs anywhere except in
+      * the targetlist (which includes GROUP BY/ORDER BY expressions), VALUES,
+      * and functions in FROM.
+      *
+      * For brevity we support two schemes for reporting an error here: set
+      * "err" to a custom message, or set "errkind" true if the error context
+      * is sufficiently identified by what ParseExprKindName will return, *and*
+      * what it will return is just a SQL keyword.  (Otherwise, use a custom
+      * message to avoid creating translation problems.)
+      */
+     err = NULL;
+     errkind = false;
+     switch (pstate->p_expr_kind)
+     {
+         case EXPR_KIND_NONE:
+             Assert(false);        /* can't happen */
+             break;
+         case EXPR_KIND_OTHER:
+             /* Accept SRF here; caller must throw error if wanted */
+             break;
+         case EXPR_KIND_JOIN_ON:
+         case EXPR_KIND_JOIN_USING:
+             err = _("set-returning functions are not allowed in JOIN conditions");
+             break;
+         case EXPR_KIND_FROM_SUBSELECT:
+             /* can't get here, but just in case, throw an error */
+             errkind = true;
+             break;
+         case EXPR_KIND_FROM_FUNCTION:
+             /* okay ... but we can't check nesting here */
+             break;
+         case EXPR_KIND_WHERE:
+             errkind = true;
+             break;
+         case EXPR_KIND_POLICY:
+             err = _("set-returning functions are not allowed in policy expressions");
+             break;
+         case EXPR_KIND_HAVING:
+             errkind = true;
+             break;
+         case EXPR_KIND_FILTER:
+             errkind = true;
+             break;
+         case EXPR_KIND_WINDOW_PARTITION:
+         case EXPR_KIND_WINDOW_ORDER:
+             /* okay, these are effectively GROUP BY/ORDER BY */
+             pstate->p_hasTargetSRFs = true;
+             break;
+         case EXPR_KIND_WINDOW_FRAME_RANGE:
+         case EXPR_KIND_WINDOW_FRAME_ROWS:
+             err = _("set-returning functions are not allowed in window definitions");
+             break;
+         case EXPR_KIND_SELECT_TARGET:
+         case EXPR_KIND_INSERT_TARGET:
+             /* okay */
+             pstate->p_hasTargetSRFs = true;
+             break;
+         case EXPR_KIND_UPDATE_SOURCE:
+         case EXPR_KIND_UPDATE_TARGET:
+             /* disallowed because it would be ambiguous what to do */
+             errkind = true;
+             break;
+         case EXPR_KIND_GROUP_BY:
+         case EXPR_KIND_ORDER_BY:
+             /* okay */
+             pstate->p_hasTargetSRFs = true;
+             break;
+         case EXPR_KIND_DISTINCT_ON:
+             /* okay */
+             pstate->p_hasTargetSRFs = true;
+             break;
+         case EXPR_KIND_LIMIT:
+         case EXPR_KIND_OFFSET:
+             errkind = true;
+             break;
+         case EXPR_KIND_RETURNING:
+             errkind = true;
+             break;
+         case EXPR_KIND_VALUES:
+             /* okay */
+             break;
+         case EXPR_KIND_CHECK_CONSTRAINT:
+         case EXPR_KIND_DOMAIN_CHECK:
+             err = _("set-returning functions are not allowed in check constraints");
+             break;
+         case EXPR_KIND_COLUMN_DEFAULT:
+         case EXPR_KIND_FUNCTION_DEFAULT:
+             err = _("set-returning functions are not allowed in DEFAULT expressions");
+             break;
+         case EXPR_KIND_INDEX_EXPRESSION:
+             err = _("set-returning functions are not allowed in index expressions");
+             break;
+         case EXPR_KIND_INDEX_PREDICATE:
+             err = _("set-returning functions are not allowed in index predicates");
+             break;
+         case EXPR_KIND_ALTER_COL_TRANSFORM:
+             err = _("set-returning functions are not allowed in transform expressions");
+             break;
+         case EXPR_KIND_EXECUTE_PARAMETER:
+             err = _("set-returning functions are not allowed in EXECUTE parameters");
+             break;
+         case EXPR_KIND_TRIGGER_WHEN:
+             err = _("set-returning functions are not allowed in trigger WHEN conditions");
+             break;
+
+             /*
+              * There is intentionally no default: case here, so that the
+              * compiler will warn if we add a new ParseExprKind without
+              * extending this switch.  If we do see an unrecognized value at
+              * runtime, the behavior will be the same as for EXPR_KIND_OTHER,
+              * which is sane anyway.
+              */
+     }
+     if (err)
+         ereport(ERROR,
+                 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                  errmsg_internal("%s", err),
+                  parser_errposition(pstate, location)));
+     if (errkind)
+         ereport(ERROR,
+                 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+         /* translator: %s is name of a SQL construct, eg GROUP BY */
+                  errmsg("set-returning functions are not allowed in %s",
+                         ParseExprKindName(pstate->p_expr_kind)),
+                  parser_errposition(pstate, location)));
+ }
diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c
index e913d05..aecda6d 100644
*** a/src/backend/parser/parse_oper.c
--- b/src/backend/parser/parse_oper.c
*************** make_op(ParseState *pstate, List *opname
*** 839,844 ****
--- 839,848 ----
      result->args = args;
      result->location = location;

+     /* if it returns a set, check that's OK */
+     if (result->opretset)
+         check_srf_call_placement(pstate, location);
+
      ReleaseSysCache(tup);

      return (Expr *) result;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 7a2950e..eaffc49 100644
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
*************** transformIndexStmt(Oid relid, IndexStmt
*** 2106,2122 ****

              /*
               * transformExpr() should have already rejected subqueries,
!              * aggregates, and window functions, based on the EXPR_KIND_ for
!              * an index expression.
               *
-              * Also reject expressions returning sets; this is for consistency
-              * with what transformWhereClause() checks for the predicate.
               * DefineIndex() will make more checks.
               */
-             if (expression_returns_set(ielem->expr))
-                 ereport(ERROR,
-                         (errcode(ERRCODE_DATATYPE_MISMATCH),
-                          errmsg("index expression cannot return a set")));
          }
      }

--- 2106,2116 ----

              /*
               * transformExpr() should have already rejected subqueries,
!              * aggregates, window functions, and SRFs, based on the EXPR_KIND_
!              * for an index expression.
               *
               * DefineIndex() will make more checks.
               */
          }
      }

*************** transformAlterTableStmt(Oid relid, Alter
*** 2594,2605 ****
                          def->cooked_default =
                              transformExpr(pstate, def->raw_default,
                                            EXPR_KIND_ALTER_COL_TRANSFORM);
-
-                         /* it can't return a set */
-                         if (expression_returns_set(def->cooked_default))
-                             ereport(ERROR,
-                                     (errcode(ERRCODE_DATATYPE_MISMATCH),
-                                      errmsg("transform expression must not return a set")));
                      }

                      newcmds = lappend(newcmds, cmd);
--- 2588,2593 ----
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index a22a11e..b828e3c 100644
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
*************** view_query_is_auto_updatable(Query *view
*** 2221,2227 ****
      if (viewquery->hasWindowFuncs)
          return gettext_noop("Views that return window functions are not automatically updatable.");

!     if (expression_returns_set((Node *) viewquery->targetList))
          return gettext_noop("Views that return set-returning functions are not automatically updatable.");

      /*
--- 2221,2227 ----
      if (viewquery->hasWindowFuncs)
          return gettext_noop("Views that return window functions are not automatically updatable.");

!     if (viewquery->hasTargetSRFs)
          return gettext_noop("Views that return set-returning functions are not automatically updatable.");

      /*
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 8d3dcf4..6de2cab 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef struct Query
*** 116,121 ****
--- 116,122 ----

      bool        hasAggs;        /* has aggregates in tlist or havingQual */
      bool        hasWindowFuncs; /* has window functions in tlist */
+     bool        hasTargetSRFs;    /* has set-returning functions in tlist */
      bool        hasSubLinks;    /* has subquery SubLink */
      bool        hasDistinctOn;    /* distinctClause is from DISTINCT ON */
      bool        hasRecursive;    /* WITH RECURSIVE was specified */
diff --git a/src/include/parser/parse_func.h b/src/include/parser/parse_func.h
index 0cefdf1..ed16d36 100644
*** a/src/include/parser/parse_func.h
--- b/src/include/parser/parse_func.h
*************** extern Oid LookupFuncNameTypeNames(List
*** 67,70 ****
--- 67,72 ----
  extern Oid LookupAggNameTypeNames(List *aggname, List *argtypes,
                         bool noError);

+ extern void check_srf_call_placement(ParseState *pstate, int location);
+
  #endif   /* PARSE_FUNC_H */
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index e3e359c..6633586 100644
*** a/src/include/parser/parse_node.h
--- b/src/include/parser/parse_node.h
***************
*** 27,33 ****
   * by extension code that might need to call transformExpr().  The core code
   * will not enforce any context-driven restrictions on EXPR_KIND_OTHER
   * expressions, so the caller would have to check for sub-selects, aggregates,
!  * and window functions if those need to be disallowed.
   */
  typedef enum ParseExprKind
  {
--- 27,33 ----
   * by extension code that might need to call transformExpr().  The core code
   * will not enforce any context-driven restrictions on EXPR_KIND_OTHER
   * expressions, so the caller would have to check for sub-selects, aggregates,
!  * window functions, SRFs, etc if those need to be disallowed.
   */
  typedef enum ParseExprKind
  {
*************** struct ParseState
*** 150,155 ****
--- 150,156 ----
      Node       *p_value_substitute;        /* what to replace VALUE with, if any */
      bool        p_hasAggs;
      bool        p_hasWindowFuncs;
+     bool        p_hasTargetSRFs;
      bool        p_hasSubLinks;
      bool        p_hasModifyingCTE;
      bool        p_is_insert;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 6141b7a..470cf93 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** exec_simple_check_plan(PLpgSQL_execstate
*** 6799,6804 ****
--- 6799,6805 ----
       */
      if (query->hasAggs ||
          query->hasWindowFuncs ||
+         query->hasTargetSRFs ||
          query->hasSubLinks ||
          query->hasForUpdate ||
          query->cteList ||
diff --git a/src/test/regress/expected/tsrf.out b/src/test/regress/expected/tsrf.out
index 805e8db..622f755 100644
*** a/src/test/regress/expected/tsrf.out
--- b/src/test/regress/expected/tsrf.out
*************** SELECT * FROM fewmore;
*** 359,373 ****
      5
  (5 rows)

! -- nonsense that seems to be allowed
  UPDATE fewmore SET data = generate_series(4,9);
  -- SRFs are not allowed in RETURNING
  INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3);
! ERROR:  set-valued function called in context that cannot accept a set
  -- nor aggregate arguments
  SELECT count(generate_series(1,3)) FROM few;
  ERROR:  set-valued function called in context that cannot accept a set
! -- nor proper VALUES
  VALUES(1, generate_series(1,2));
  ERROR:  set-valued function called in context that cannot accept a set
  -- DISTINCT ON is evaluated before tSRF evaluation if SRF is not
--- 359,378 ----
      5
  (5 rows)

! -- SRFs are not allowed in UPDATE (they once were, but it was nonsense)
  UPDATE fewmore SET data = generate_series(4,9);
+ ERROR:  set-returning functions are not allowed in UPDATE
+ LINE 1: UPDATE fewmore SET data = generate_series(4,9);
+                                   ^
  -- SRFs are not allowed in RETURNING
  INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3);
! ERROR:  set-returning functions are not allowed in RETURNING
! LINE 1: INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3)...
!                                                 ^
  -- nor aggregate arguments
  SELECT count(generate_series(1,3)) FROM few;
  ERROR:  set-valued function called in context that cannot accept a set
! -- nor standalone VALUES (but surely this is a bug?)
  VALUES(1, generate_series(1,2));
  ERROR:  set-valued function called in context that cannot accept a set
  -- DISTINCT ON is evaluated before tSRF evaluation if SRF is not
*************** SELECT a, generate_series(1,2) FROM (VAL
*** 457,463 ****

  -- SRFs are not allowed in LIMIT.
  SELECT 1 LIMIT generate_series(1,3);
! ERROR:  argument of LIMIT must not return a set
  LINE 1: SELECT 1 LIMIT generate_series(1,3);
                         ^
  -- tSRF in correlated subquery, referencing table outside
--- 462,468 ----

  -- SRFs are not allowed in LIMIT.
  SELECT 1 LIMIT generate_series(1,3);
! ERROR:  set-returning functions are not allowed in LIMIT
  LINE 1: SELECT 1 LIMIT generate_series(1,3);
                         ^
  -- tSRF in correlated subquery, referencing table outside
diff --git a/src/test/regress/sql/tsrf.sql b/src/test/regress/sql/tsrf.sql
index 5247795..c28dd01 100644
*** a/src/test/regress/sql/tsrf.sql
--- b/src/test/regress/sql/tsrf.sql
*************** CREATE TABLE fewmore AS SELECT generate_
*** 68,81 ****
  INSERT INTO fewmore VALUES(generate_series(4,5));
  SELECT * FROM fewmore;

! -- nonsense that seems to be allowed
  UPDATE fewmore SET data = generate_series(4,9);

  -- SRFs are not allowed in RETURNING
  INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3);
  -- nor aggregate arguments
  SELECT count(generate_series(1,3)) FROM few;
! -- nor proper VALUES
  VALUES(1, generate_series(1,2));

  -- DISTINCT ON is evaluated before tSRF evaluation if SRF is not
--- 68,81 ----
  INSERT INTO fewmore VALUES(generate_series(4,5));
  SELECT * FROM fewmore;

! -- SRFs are not allowed in UPDATE (they once were, but it was nonsense)
  UPDATE fewmore SET data = generate_series(4,9);

  -- SRFs are not allowed in RETURNING
  INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3);
  -- nor aggregate arguments
  SELECT count(generate_series(1,3)) FROM few;
! -- nor standalone VALUES (but surely this is a bug?)
  VALUES(1, generate_series(1,2));

  -- DISTINCT ON is evaluated before tSRF evaluation if SRF is not