Thread: Attribute of type record has wrong type error with MERGE ... WHEN NOT MATCHED BY SOURCE THEN DELETE

Postgresql version 17.4 (Ubuntu 17.4-1.pgdg24.10+2) on x86_64-pc-linux-gnu

To reproduce, execute the statements in the attached file cr.sql.  I get:

duncan=> \i cr.sql
CREATE TABLE
CREATE TABLE
CREATE VIEW
CREATE TABLE
COPY 1
COPY 2
COPY 1
psql:cr.sql:42: ERROR:  attribute 2 of type record has wrong type
DETAIL:  Table has type _country_or_region, but query expects record.

I attribute it to the "WHEN NOT MATCHED BY SOURCE THEN DELETE" part of the MERGE 
as it doesn't happen if that part is left off.

Best wishes, Duncan.

Tender Wang <tndrwang@gmail.com> writes:
> When the query has NOT MATCHED BY SOURCE, commit d7d297f84 add "src IS NOT
> NULL" join condition.
> In this case, the src is view(e.g. subquery), so in makeWholeRowVar(), it
> will call below code:
> result = makeVar(varno,
>                       InvalidAttrNumber,
>                       RECORDOID,
>                       -1,
>                       InvalidOid,
>                       varlevelsup);

> the vartype is RECORDOID, but te reltype of src is not RECORDOID, so
> $SUBJECT error reports.

Hmm.  I tried adjusting the example to make _country_or_region
be a materialized view or plain table instead of a view, and
those cases did not fail.  I wonder why the difference...

> I add the below codes to makeWholeRowVar() default branch:
> if (rte->relkind == RELKIND_VIEW)
>         toid = get_rel_type_id(rte->relid);
> else
>         toid = RECORDOID;
> It can work.

I'm quite uncomfortable with the idea of changing makeWholeRowVar()
itself in this way --- the potential blast radius from that seems
rather large.  A localized fix in transform_MERGE_to_join() might
be a wiser answer.

            regards, tom lane

On Mon, 10 Mar 2025 at 13:46, Tender Wang <tndrwang@gmail.com> wrote:
>
> When the query has NOT MATCHED BY SOURCE, commit d7d297f84 add "src IS NOT NULL" join condition.
> In this case, the src is view(e.g. subquery), so in makeWholeRowVar(), it will call below code:
> result = makeVar(varno,
>                       InvalidAttrNumber,
>                       RECORDOID,
>                       -1,
>                       InvalidOid,
>                       varlevelsup);
>
> the vartype is RECORDOID, but te reltype of src is not RECORDOID, so $SUBJECT error reports.
>
> I add the below codes to makeWholeRowVar() default branch:
>
> if (rte->relkind == RELKIND_VIEW)
>         toid = get_rel_type_id(rte->relid);
> else
>         toid = RECORDOID;
>
> It can work.
>

Yes, I reached the same conclusion.

When the parser processes the "AND qq_src IS DISTINCT FROM qq_tgt"
clause, it creates a whole-row Var for qq_src whose type is the view
type. Then transform_MERGE_to_join() adds another whole-row Var for
qq_src, but by this time the RTE has been expanded into a subquery
RTE, so its type becomes RECORDOID. The executor then grumbles because
it has 2 Vars with the same varno and varattno, but different
vartypes.

Fixing that by having makeWholeRowVar() set the type based on
rte->relid for subquery RTEs that used to be views seems like a good
fix. However, it looks like that fix will only work as far back as v16
(where 47bb9db7599 and 0f8cfaf8921 were added).

Unfortunately, it looks like this bug pre-dates MERGE WHEN NOT MATCHED
BY SOURCE, and even MERGE itself. All that's needed to trigger it is a
query that causes 2 whole-row Vars to be added, one before and one
after view expansion. That can be made to happen via the rowmarking
mechanism in all supported branches as follows:

create table foo (a int, b int);
create view foo_v as select * from foo offset 0;
insert into foo values (1,2);
update foo set b = foo_v.b from foo_v where foo_v.a = foo.a returning foo_v;

which fails in the same way, with

ERROR:  attribute 3 of type record has wrong type
DETAIL:  Table has type record, but query expects foo_v.

Reading the commit message for 47bb9db7599 suggests that maybe it
would be OK to further back-patch the changes to ApplyRetrieveRule()
to retain relkind and relid on subquery RTEs for this purpose. That
wouldn't affect stored rules, but I haven't looked to see what else it
might affect.

Thoughts?

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Unfortunately, it looks like this bug pre-dates MERGE WHEN NOT MATCHED
> BY SOURCE, and even MERGE itself. All that's needed to trigger it is a
> query that causes 2 whole-row Vars to be added, one before and one
> after view expansion. That can be made to happen via the rowmarking
> mechanism in all supported branches as follows:

Ugh, right.  So I withdraw my objection to fixing this in
makeWholeRowVar: all of the post-rewrite calls have need for this
behavior.  However, the proposed code change is wrong in detail.
The existing places that are checking for this situation are doing
tests like
    (rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)))
I don't believe that checking relkind instead is an improvement.

> Reading the commit message for 47bb9db7599 suggests that maybe it
> would be OK to further back-patch the changes to ApplyRetrieveRule()
> to retain relkind and relid on subquery RTEs for this purpose. That
> wouldn't affect stored rules, but I haven't looked to see what else it
> might affect.

Yeah, I think we can likely get away with that.  We cannot back-patch
the changes that added relid to the outfuncs/readfuncs representation,
which means that the RTE's relid won't propagate to parallel workers,
but I don't see why they'd need it.  We only need that info to get
as far as planning.  I've not tried though.

Draft HEAD patch attached.

            regards, tom lane

diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index dbbc2f1e30d..f54d85e7bba 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -161,6 +161,34 @@ makeWholeRowVar(RangeTblEntry *rte,
                              varlevelsup);
             break;

+        case RTE_SUBQUERY:
+
+            /*
+             * For a standard subquery, the Var should be of RECORD type.
+             * However, if we're looking at a subquery that was expanded from
+             * a view (only possible during planning), we must use the view's
+             * rowtype, so that the resulting Var has the same type that we
+             * would have produced from the original RTE_RELATION RTE.
+             */
+            if (OidIsValid(rte->relid))
+            {
+                toid = get_rel_type_id(rte->relid);
+                if (!OidIsValid(toid))
+                    ereport(ERROR,
+                            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                             errmsg("relation \"%s\" does not have a composite type",
+                                    get_rel_name(rte->relid))));
+            }
+            else
+                toid = RECORDOID;
+            result = makeVar(varno,
+                             InvalidAttrNumber,
+                             toid,
+                             -1,
+                             InvalidOid,
+                             varlevelsup);
+            break;
+
         case RTE_FUNCTION:

             /*
@@ -217,8 +245,8 @@ makeWholeRowVar(RangeTblEntry *rte,
         default:

             /*
-             * RTE is a join, subselect, tablefunc, or VALUES.  We represent
-             * this as a whole-row Var of RECORD type. (Note that in most
+             * RTE is a join, tablefunc, VALUES, CTE, etc.  We represent these
+             * cases as a whole-row Var of RECORD type.  (Note that in most
              * cases the Var will be expanded to a RowExpr during planning,
              * but that is not our concern here.)
              */
diff --git a/src/test/regress/expected/returning.out b/src/test/regress/expected/returning.out
index d1394c67833..6de764e9dad 100644
--- a/src/test/regress/expected/returning.out
+++ b/src/test/regress/expected/returning.out
@@ -286,6 +286,23 @@ SELECT * FROM voo;
  16 | zoo2
 (2 rows)

+-- Check use of an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+      foo_v
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
 -- Try a join case
 CREATE TEMP TABLE joinme (f2j text, other int);
 INSERT INTO joinme VALUES('more', 12345);
@@ -726,8 +743,9 @@ NOTICE:  UPDATE: (3,zoo2,58,99,54321) -> (3,zoo2,59,7,54321)

 -- Test wholerow & dropped column handling
 ALTER TABLE foo DROP COLUMN f3 CASCADE;
-NOTICE:  drop cascades to 3 other objects
+NOTICE:  drop cascades to 4 other objects
 DETAIL:  drop cascades to rule voo_i on view voo
+drop cascades to view foo_v
 drop cascades to view joinview
 drop cascades to rule foo_del_rule on table foo
 UPDATE foo SET f4 = f4 + 1 RETURNING old.f3;  -- should fail
diff --git a/src/test/regress/sql/returning.sql b/src/test/regress/sql/returning.sql
index 54caf56244c..df11caef502 100644
--- a/src/test/regress/sql/returning.sql
+++ b/src/test/regress/sql/returning.sql
@@ -132,6 +132,12 @@ DELETE FROM foo WHERE f2 = 'zit' RETURNING *;
 SELECT * FROM foo;
 SELECT * FROM voo;

+-- Check use of an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+SELECT * FROM foo;
+
 -- Try a join case

 CREATE TEMP TABLE joinme (f2j text, other int);

I wrote:
> Yeah, I think we can likely get away with that.  We cannot back-patch
> the changes that added relid to the outfuncs/readfuncs representation,
> which means that the RTE's relid won't propagate to parallel workers,
> but I don't see why they'd need it.  We only need that info to get
> as far as planning.  I've not tried though.

OK, the attached patch for v15 passes check-world, with or without
force_parallel_mode.  I'm inclined to commit the rewriteHandler.c
and parsenodes.h bits in a separate patch for commit log visibility.

It would be easy enough to leave the RTE's relkind and/or rellockmode
alone too, but I think the conservative approach is to not change
more than we have to in these old branches.  There is some attraction
to making the behavior more like the newer branches, but still ...

            regards, tom lane

diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index c85d8fe9751..c16263056e3 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -157,6 +157,34 @@ makeWholeRowVar(RangeTblEntry *rte,
                              varlevelsup);
             break;

+        case RTE_SUBQUERY:
+
+            /*
+             * For a standard subquery, the Var should be of RECORD type.
+             * However, if we're looking at a subquery that was expanded from
+             * a view (only possible during planning), we must use the view's
+             * rowtype, so that the resulting Var has the same type that we
+             * would have produced from the original RTE_RELATION RTE.
+             */
+            if (OidIsValid(rte->relid))
+            {
+                toid = get_rel_type_id(rte->relid);
+                if (!OidIsValid(toid))
+                    ereport(ERROR,
+                            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                             errmsg("relation \"%s\" does not have a composite type",
+                                    get_rel_name(rte->relid))));
+            }
+            else
+                toid = RECORDOID;
+            result = makeVar(varno,
+                             InvalidAttrNumber,
+                             toid,
+                             -1,
+                             InvalidOid,
+                             varlevelsup);
+            break;
+
         case RTE_FUNCTION:

             /*
@@ -213,8 +241,8 @@ makeWholeRowVar(RangeTblEntry *rte,
         default:

             /*
-             * RTE is a join, subselect, tablefunc, or VALUES.  We represent
-             * this as a whole-row Var of RECORD type. (Note that in most
+             * RTE is a join, tablefunc, VALUES, CTE, etc.  We represent these
+             * cases as a whole-row Var of RECORD type.  (Note that in most
              * cases the Var will be expanded to a RowExpr during planning,
              * but that is not our concern here.)
              */
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index d2a0e501d1e..30ae22e43df 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1859,8 +1859,12 @@ ApplyRetrieveRule(Query *parsetree,
     rte->rtekind = RTE_SUBQUERY;
     rte->subquery = rule_action;
     rte->security_barrier = RelationIsSecurityView(relation);
-    /* Clear fields that should not be set in a subquery RTE */
-    rte->relid = InvalidOid;
+
+    /*
+     * Clear fields that should not be set in a subquery RTE.  However, we
+     * retain the relid to support correct operation of makeWholeRowVar during
+     * planning.
+     */
     rte->relkind = 0;
     rte->rellockmode = 0;
     rte->tablesample = NULL;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 6944362c7ac..4b9e41cb6b6 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1017,6 +1017,12 @@ typedef struct RangeTblEntry
     /*
      * Fields valid for a plain relation RTE (else zero):
      *
+     * As a special case, relid can also be set in RTE_SUBQUERY RTEs.  This
+     * happens when a RTE_RELATION RTE for a view is transformed to a
+     * RTE_SUBQUERY during rewriting.  We keep the relid because it is useful
+     * during planning, cf makeWholeRowVar.  (It cannot be relied on during
+     * execution, because it will not propagate to parallel workers.)
+     *
      * As a special case, RTE_NAMEDTUPLESTORE can also set relid to indicate
      * that the tuple format of the tuplestore is the same as the referenced
      * relation.  This allows plans referencing AFTER trigger transition
diff --git a/src/test/regress/expected/returning.out b/src/test/regress/expected/returning.out
index cb51bb86876..82d8c0ed6d1 100644
--- a/src/test/regress/expected/returning.out
+++ b/src/test/regress/expected/returning.out
@@ -286,6 +286,23 @@ SELECT * FROM voo;
  16 | zoo2
 (2 rows)

+-- Check use of an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+      foo_v
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
 -- Try a join case
 CREATE TEMP TABLE joinme (f2j text, other int);
 INSERT INTO joinme VALUES('more', 12345);
diff --git a/src/test/regress/sql/returning.sql b/src/test/regress/sql/returning.sql
index a460f82fb7c..0b224b54419 100644
--- a/src/test/regress/sql/returning.sql
+++ b/src/test/regress/sql/returning.sql
@@ -132,6 +132,12 @@ DELETE FROM foo WHERE f2 = 'zit' RETURNING *;
 SELECT * FROM foo;
 SELECT * FROM voo;

+-- Check use of an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+SELECT * FROM foo;
+
 -- Try a join case

 CREATE TEMP TABLE joinme (f2j text, other int);

On Mon, 10 Mar 2025 at 19:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Yeah, I think we can likely get away with that.  We cannot back-patch
> > the changes that added relid to the outfuncs/readfuncs representation,
> > which means that the RTE's relid won't propagate to parallel workers,
> > but I don't see why they'd need it.  We only need that info to get
> > as far as planning.  I've not tried though.
>
> OK, the attached patch for v15 passes check-world, with or without
> force_parallel_mode.  I'm inclined to commit the rewriteHandler.c
> and parsenodes.h bits in a separate patch for commit log visibility.
>

That looks good to me, on a quick read-through.

However, that's not quite the end of it -- preprocess_function_rtes()
/ inline_set_returning_function() can turn a function RTE into a
subquery RTE, leading to a similar problem:

create table foo (a int, b int);
insert into foo values (1,2);
create or replace function f() returns setof foo as
  $$ select * from foo offset 0 $$ language sql stable;
update foo set b = f.b from f() as f(a,b) where f.a = foo.a returning f;

ERROR:  attribute 3 of type record has wrong type
DETAIL:  Table has type record, but query expects foo.

I tried looking for other places that change an RTE's rtekind, and
haven't managed to find any other problems, but may have missed
something.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> However, that's not quite the end of it -- preprocess_function_rtes()
> / inline_set_returning_function() can turn a function RTE into a
> subquery RTE, leading to a similar problem:

> create table foo (a int, b int);
> insert into foo values (1,2);
> create or replace function f() returns setof foo as
>   $$ select * from foo offset 0 $$ language sql stable;
> update foo set b = f.b from f() as f(a,b) where f.a = foo.a returning f;

> ERROR:  attribute 3 of type record has wrong type
> DETAIL:  Table has type record, but query expects foo.

Double ugh.  I guess we could get preprocess_function_rtes to
insert the appropriate relid ...

            regards, tom lane

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> I tried looking for other places that change an RTE's rtekind, and
> haven't managed to find any other problems, but may have missed
> something.

The only other place I can find that converts some other kind of RTE
to a subquery is inline_cte(), which converts RTE_CTE to RTE_SUBQUERY.
That seems immune to the present problem because the appropriate
vartype would be RECORD in either case.

            regards, tom lane

I wrote:
> Double ugh.  I guess we could get preprocess_function_rtes to
> insert the appropriate relid ...

OK, that was less painful than I feared.  makeWholeRowVar has
several different special cases for RTE_FUNCTION, but most of them
don't bear on this problem, because we wouldn't have applied inlining
when they did.  It seems sufficient to fetch pg_type.typrelid for
the function's nominal return type and store that if it's not 0.

Patches for HEAD and v15 attached.  I haven't checked other branches
but I expect the deltas won't be big.

            regards, tom lane

diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index dbbc2f1e30d..71632512380 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -161,6 +161,34 @@ makeWholeRowVar(RangeTblEntry *rte,
                              varlevelsup);
             break;

+        case RTE_SUBQUERY:
+
+            /*
+             * For a standard subquery, the Var should be of RECORD type.
+             * However, if we're looking at a subquery that was expanded from
+             * a view or SRF (only possible during planning), we must use the
+             * appropriate rowtype, so that the resulting Var has the same
+             * type that we would have produced from the original RTE.
+             */
+            if (OidIsValid(rte->relid))
+            {
+                toid = get_rel_type_id(rte->relid);
+                if (!OidIsValid(toid))
+                    ereport(ERROR,
+                            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                             errmsg("relation \"%s\" does not have a composite type",
+                                    get_rel_name(rte->relid))));
+            }
+            else
+                toid = RECORDOID;
+            result = makeVar(varno,
+                             InvalidAttrNumber,
+                             toid,
+                             -1,
+                             InvalidOid,
+                             varlevelsup);
+            break;
+
         case RTE_FUNCTION:

             /*
@@ -217,8 +245,8 @@ makeWholeRowVar(RangeTblEntry *rte,
         default:

             /*
-             * RTE is a join, subselect, tablefunc, or VALUES.  We represent
-             * this as a whole-row Var of RECORD type. (Note that in most
+             * RTE is a join, tablefunc, VALUES, CTE, etc.  We represent these
+             * cases as a whole-row Var of RECORD type.  (Note that in most
              * cases the Var will be expanded to a RowExpr during planning,
              * but that is not our concern here.)
              */
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index bcc40dd5a84..477ff3fa76d 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -43,6 +43,7 @@
 #include "parser/parsetree.h"
 #include "rewrite/rewriteHandler.h"
 #include "rewrite/rewriteManip.h"
+#include "utils/lsyscache.h"
 #include "utils/rel.h"


@@ -911,9 +912,28 @@ preprocess_function_rtes(PlannerInfo *root)
             if (funcquery)
             {
                 /* Successful expansion, convert the RTE to a subquery */
+                Node       *fexpr;
+                Oid            toid;
+
                 rte->rtekind = RTE_SUBQUERY;
                 rte->subquery = funcquery;
                 rte->security_barrier = false;
+
+                /*
+                 * If the SRF returned a named composite type (not RECORD), we
+                 * must also set rte->relid so that makeWholeRowVar can still
+                 * produce the same output for the RTE as it did before.  This
+                 * code relies on the fact that inline_set_returning_function
+                 * won't have succeeded unless there is exactly one entry in
+                 * rte->functions.  Also, this would likely be the wrong thing
+                 * for domain-over-composite, but functions returning those
+                 * won't get inlined either.
+                 */
+                fexpr = ((RangeTblFunction *) linitial(rte->functions))->funcexpr;
+                toid = get_typ_typrelid(exprType(fexpr));
+                if (OidIsValid(toid))
+                    rte->relid = toid;
+
                 /* Clear fields that should not be set in a subquery RTE */
                 rte->functions = NIL;
                 rte->funcordinality = false;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 23c9e3c5abf..ef8e4eab9e0 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1086,7 +1086,10 @@ typedef struct RangeTblEntry
      * containing the view's query.  We still need to perform run-time locking
      * and permission checks on the view, even though it's not directly used
      * in the query anymore, and the most expedient way to do that is to
-     * retain these fields from the old state of the RTE.
+     * retain these fields from the old state of the RTE.  relid (but not the
+     * other fields) is also set when converting an RTE_FUNCTION RTE to an
+     * RTE_SUBQUERY by inlining a set-returning function that returns a named
+     * composite type.
      *
      * As a special case, RTE_NAMEDTUPLESTORE can also set relid to indicate
      * that the tuple format of the tuplestore is the same as the referenced
diff --git a/src/test/regress/expected/returning.out b/src/test/regress/expected/returning.out
index d1394c67833..9659cae9ce7 100644
--- a/src/test/regress/expected/returning.out
+++ b/src/test/regress/expected/returning.out
@@ -286,6 +286,42 @@ SELECT * FROM voo;
  16 | zoo2
 (2 rows)

+-- Check use of a whole-row variable for an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+      foo_v
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+-- Check use of a whole-row variable for an inlined set-returning function
+CREATE FUNCTION foo_f() RETURNS SETOF foo AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+      foo_f
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+DROP FUNCTION foo_f();
 -- Try a join case
 CREATE TEMP TABLE joinme (f2j text, other int);
 INSERT INTO joinme VALUES('more', 12345);
@@ -726,8 +762,9 @@ NOTICE:  UPDATE: (3,zoo2,58,99,54321) -> (3,zoo2,59,7,54321)

 -- Test wholerow & dropped column handling
 ALTER TABLE foo DROP COLUMN f3 CASCADE;
-NOTICE:  drop cascades to 3 other objects
+NOTICE:  drop cascades to 4 other objects
 DETAIL:  drop cascades to rule voo_i on view voo
+drop cascades to view foo_v
 drop cascades to view joinview
 drop cascades to rule foo_del_rule on table foo
 UPDATE foo SET f4 = f4 + 1 RETURNING old.f3;  -- should fail
diff --git a/src/test/regress/sql/returning.sql b/src/test/regress/sql/returning.sql
index 54caf56244c..9aea7fb609f 100644
--- a/src/test/regress/sql/returning.sql
+++ b/src/test/regress/sql/returning.sql
@@ -132,6 +132,20 @@ DELETE FROM foo WHERE f2 = 'zit' RETURNING *;
 SELECT * FROM foo;
 SELECT * FROM voo;

+-- Check use of a whole-row variable for an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+SELECT * FROM foo;
+
+-- Check use of a whole-row variable for an inlined set-returning function
+CREATE FUNCTION foo_f() RETURNS SETOF foo AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+SELECT * FROM foo;
+DROP FUNCTION foo_f();
+
 -- Try a join case

 CREATE TEMP TABLE joinme (f2j text, other int);
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index c85d8fe9751..691950857b6 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -157,6 +157,34 @@ makeWholeRowVar(RangeTblEntry *rte,
                              varlevelsup);
             break;

+        case RTE_SUBQUERY:
+
+            /*
+             * For a standard subquery, the Var should be of RECORD type.
+             * However, if we're looking at a subquery that was expanded from
+             * a view or SRF (only possible during planning), we must use the
+             * appropriate rowtype, so that the resulting Var has the same
+             * type that we would have produced from the original RTE.
+             */
+            if (OidIsValid(rte->relid))
+            {
+                toid = get_rel_type_id(rte->relid);
+                if (!OidIsValid(toid))
+                    ereport(ERROR,
+                            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                             errmsg("relation \"%s\" does not have a composite type",
+                                    get_rel_name(rte->relid))));
+            }
+            else
+                toid = RECORDOID;
+            result = makeVar(varno,
+                             InvalidAttrNumber,
+                             toid,
+                             -1,
+                             InvalidOid,
+                             varlevelsup);
+            break;
+
         case RTE_FUNCTION:

             /*
@@ -213,8 +241,8 @@ makeWholeRowVar(RangeTblEntry *rte,
         default:

             /*
-             * RTE is a join, subselect, tablefunc, or VALUES.  We represent
-             * this as a whole-row Var of RECORD type. (Note that in most
+             * RTE is a join, tablefunc, VALUES, CTE, etc.  We represent these
+             * cases as a whole-row Var of RECORD type.  (Note that in most
              * cases the Var will be expanded to a RowExpr during planning,
              * but that is not our concern here.)
              */
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 0efcc3b24bc..aa578b61f00 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -39,6 +39,7 @@
 #include "parser/parse_relation.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteManip.h"
+#include "utils/lsyscache.h"


 typedef struct pullup_replace_vars_context
@@ -741,9 +742,28 @@ preprocess_function_rtes(PlannerInfo *root)
             if (funcquery)
             {
                 /* Successful expansion, convert the RTE to a subquery */
+                Node       *fexpr;
+                Oid            toid;
+
                 rte->rtekind = RTE_SUBQUERY;
                 rte->subquery = funcquery;
                 rte->security_barrier = false;
+
+                /*
+                 * If the SRF returned a named composite type (not RECORD), we
+                 * must also set rte->relid so that makeWholeRowVar can still
+                 * produce the same output for the RTE as it did before.  This
+                 * code relies on the fact that inline_set_returning_function
+                 * won't have succeeded unless there is exactly one entry in
+                 * rte->functions.  Also, this would likely be the wrong thing
+                 * for domain-over-composite, but functions returning those
+                 * won't get inlined either.
+                 */
+                fexpr = ((RangeTblFunction *) linitial(rte->functions))->funcexpr;
+                toid = get_typ_typrelid(exprType(fexpr));
+                if (OidIsValid(toid))
+                    rte->relid = toid;
+
                 /* Clear fields that should not be set in a subquery RTE */
                 rte->functions = NIL;
                 rte->funcordinality = false;
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index d2a0e501d1e..30ae22e43df 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1859,8 +1859,12 @@ ApplyRetrieveRule(Query *parsetree,
     rte->rtekind = RTE_SUBQUERY;
     rte->subquery = rule_action;
     rte->security_barrier = RelationIsSecurityView(relation);
-    /* Clear fields that should not be set in a subquery RTE */
-    rte->relid = InvalidOid;
+
+    /*
+     * Clear fields that should not be set in a subquery RTE.  However, we
+     * retain the relid to support correct operation of makeWholeRowVar during
+     * planning.
+     */
     rte->relkind = 0;
     rte->rellockmode = 0;
     rte->tablesample = NULL;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 6944362c7ac..056902dccef 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1017,6 +1017,13 @@ typedef struct RangeTblEntry
     /*
      * Fields valid for a plain relation RTE (else zero):
      *
+     * As a special case, relid can also be set in RTE_SUBQUERY RTEs.  This
+     * happens when an RTE for a view or composite-returning function is
+     * transformed to an RTE_SUBQUERY during rewriting.  We keep the relid
+     * because it is useful during planning, cf makeWholeRowVar.  (It cannot
+     * be relied on during execution, because it will not propagate to
+     * parallel workers.)
+     *
      * As a special case, RTE_NAMEDTUPLESTORE can also set relid to indicate
      * that the tuple format of the tuplestore is the same as the referenced
      * relation.  This allows plans referencing AFTER trigger transition
diff --git a/src/test/regress/expected/returning.out b/src/test/regress/expected/returning.out
index cb51bb86876..461f9fdefa6 100644
--- a/src/test/regress/expected/returning.out
+++ b/src/test/regress/expected/returning.out
@@ -286,6 +286,42 @@ SELECT * FROM voo;
  16 | zoo2
 (2 rows)

+-- Check use of a whole-row variable for an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+      foo_v
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+-- Check use of a whole-row variable for an inlined set-returning function
+CREATE FUNCTION foo_f() RETURNS SETOF foo AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+      foo_f
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+DROP FUNCTION foo_f();
 -- Try a join case
 CREATE TEMP TABLE joinme (f2j text, other int);
 INSERT INTO joinme VALUES('more', 12345);
diff --git a/src/test/regress/sql/returning.sql b/src/test/regress/sql/returning.sql
index a460f82fb7c..08bfdec2d1a 100644
--- a/src/test/regress/sql/returning.sql
+++ b/src/test/regress/sql/returning.sql
@@ -132,6 +132,20 @@ DELETE FROM foo WHERE f2 = 'zit' RETURNING *;
 SELECT * FROM foo;
 SELECT * FROM voo;

+-- Check use of a whole-row variable for an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+SELECT * FROM foo;
+
+-- Check use of a whole-row variable for an inlined set-returning function
+CREATE FUNCTION foo_f() RETURNS SETOF foo AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+SELECT * FROM foo;
+DROP FUNCTION foo_f();
+
 -- Try a join case

 CREATE TEMP TABLE joinme (f2j text, other int);

On Tue, 11 Mar 2025 at 17:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Double ugh.  I guess we could get preprocess_function_rtes to
> > insert the appropriate relid ...
>
> OK, that was less painful than I feared.  makeWholeRowVar has
> several different special cases for RTE_FUNCTION, but most of them
> don't bear on this problem, because we wouldn't have applied inlining
> when they did.  It seems sufficient to fetch pg_type.typrelid for
> the function's nominal return type and store that if it's not 0.
>

Hmm, this introduces a new problem. Testing a case with a composite type:

create table foo (a int, b int);
insert into foo values (1,2);
create type t as (a int, b int);
create or replace function f() returns setof t as
  $$ select 1,2 from foo offset 0 $$ language sql stable;

then doing

update foo set b = f.b from f() where f.a = foo.a returning f;

or even just

select f from f();

triggers an Assert() in relation_open() from
get_relation_data_width(), from set_rel_width() because now that the
RTE has a relid, it attempts to open it, without having previously
locked it. Maybe we need to not clear rte->functions, and use that
instead.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Hmm, this introduces a new problem. Testing a case with a composite type:

> create table foo (a int, b int);
> insert into foo values (1,2);
> create type t as (a int, b int);
> create or replace function f() returns setof t as
>   $$ select 1,2 from foo offset 0 $$ language sql stable;
> then doing
> update foo set b = f.b from f() where f.a = foo.a returning f;
> or even just
> select f from f();
> triggers an Assert() in relation_open() from
> get_relation_data_width(), from set_rel_width() because now that the
> RTE has a relid, it attempts to open it, without having previously
> locked it.

Geez, our regression tests seem quite lacking in this area.

I'm wondering why we're trying to do relation_open on a SUBQUERY
RTE, relid or no relid.  It seems kind of accidental that
set_rel_width() doesn't fail on subqueries given this coding.
Even without actual failure, looking to the referenced relation for
width estimates for a subquery seems like a pretty broken idea.
However, this does indicate that putting a composite type's relid
into the RTE is more dangerous than I thought --- there may be
other code out there doing things similar to set_rel_width().

> Maybe we need to not clear rte->functions, and use that
> instead.

At first I didn't like that idea a bit, and it's still rather ugly,
but it does have two advantages:

* it seems less likely to break anyone's assumptions about the data
structure;

* we'd avoid a purely speculative catcache fetch in
preprocess_function_rtes, which most of the time is a waste of effort
because no subsequent makeWholeRowVar will happen.

We could still clear rte->functions during setrefs.c, so that this
abuse of the data structure is purely local to the planner.

I'll have a go at coding it that way...

            regards, tom lane

I wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> Maybe we need to not clear rte->functions, and use that
>> instead.

> I'll have a go at coding it that way...

Yeah, that is a better way.  In exchange for a slightly dirtier
data structure, we now have essentially all the relevant code in
makeWholeRowVar: correctness only depends on its different case
branches agreeing, rather than on some not-terribly-similar code
way over in prepjointree.c.

I also took the opportunity to split off the old-branch adjustment
of rewriteHandler.c, so that the HEAD and v15 versions of the main
bug fix patch are nearly the same.

            regards, tom lane

diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index dbbc2f1e30d..939444b6c0d 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -161,6 +161,49 @@ makeWholeRowVar(RangeTblEntry *rte,
                              varlevelsup);
             break;

+        case RTE_SUBQUERY:
+
+            /*
+             * For a standard subquery, the Var should be of RECORD type.
+             * However, if we're looking at a subquery that was expanded from
+             * a view or SRF (only possible during planning), we must use the
+             * appropriate rowtype, so that the resulting Var has the same
+             * type that we would have produced from the original RTE.
+             */
+            if (OidIsValid(rte->relid))
+            {
+                /* Subquery was expanded from a view */
+                toid = get_rel_type_id(rte->relid);
+                if (!OidIsValid(toid))
+                    ereport(ERROR,
+                            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                             errmsg("relation \"%s\" does not have a composite type",
+                                    get_rel_name(rte->relid))));
+            }
+            else if (rte->functions)
+            {
+                /*
+                 * Subquery was expanded from a set-returning function.  That
+                 * would not have happened if there's more than one function
+                 * or ordinality was requested.  We also needn't worry about
+                 * the allowScalar case, since the planner doesn't use that.
+                 */
+                Assert(!allowScalar);
+                fexpr = ((RangeTblFunction *) linitial(rte->functions))->funcexpr;
+                toid = exprType(fexpr);
+                if (!type_is_rowtype(toid))
+                    toid = RECORDOID;
+            }
+            else
+                toid = RECORDOID;
+            result = makeVar(varno,
+                             InvalidAttrNumber,
+                             toid,
+                             -1,
+                             InvalidOid,
+                             varlevelsup);
+            break;
+
         case RTE_FUNCTION:

             /*
@@ -217,8 +260,8 @@ makeWholeRowVar(RangeTblEntry *rte,
         default:

             /*
-             * RTE is a join, subselect, tablefunc, or VALUES.  We represent
-             * this as a whole-row Var of RECORD type. (Note that in most
+             * RTE is a join, tablefunc, VALUES, CTE, etc.  We represent these
+             * cases as a whole-row Var of RECORD type.  (Note that in most
              * cases the Var will be expanded to a RowExpr during planning,
              * but that is not our concern here.)
              */
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index bcc40dd5a84..a9efc0b23a2 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -914,8 +914,14 @@ preprocess_function_rtes(PlannerInfo *root)
                 rte->rtekind = RTE_SUBQUERY;
                 rte->subquery = funcquery;
                 rte->security_barrier = false;
-                /* Clear fields that should not be set in a subquery RTE */
-                rte->functions = NIL;
+
+                /*
+                 * Clear fields that should not be set in a subquery RTE.
+                 * However, we leave rte->functions filled in for the moment,
+                 * in case makeWholeRowVar needs to consult it.  We'll clear
+                 * it in setrefs.c (see add_rte_to_flat_rtable) so that this
+                 * abuse of the data structure doesn't escape the planner.
+                 */
                 rte->funcordinality = false;
             }
         }
diff --git a/src/test/regress/expected/returning.out b/src/test/regress/expected/returning.out
index d1394c67833..341b689f766 100644
--- a/src/test/regress/expected/returning.out
+++ b/src/test/regress/expected/returning.out
@@ -286,6 +286,63 @@ SELECT * FROM voo;
  16 | zoo2
 (2 rows)

+-- Check use of a whole-row variable for an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+      foo_v
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+-- Check use of a whole-row variable for an inlined set-returning function
+CREATE FUNCTION foo_f() RETURNS SETOF foo AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+      foo_f
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+DROP FUNCTION foo_f();
+-- As above, but SRF is defined to return a composite type
+CREATE TYPE foo_t AS (f1 int, f2 text, f3 int, f4 int8);
+CREATE FUNCTION foo_f() RETURNS SETOF foo_t AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+      foo_f
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+DROP FUNCTION foo_f();
+DROP TYPE foo_t;
 -- Try a join case
 CREATE TEMP TABLE joinme (f2j text, other int);
 INSERT INTO joinme VALUES('more', 12345);
@@ -726,8 +783,9 @@ NOTICE:  UPDATE: (3,zoo2,58,99,54321) -> (3,zoo2,59,7,54321)

 -- Test wholerow & dropped column handling
 ALTER TABLE foo DROP COLUMN f3 CASCADE;
-NOTICE:  drop cascades to 3 other objects
+NOTICE:  drop cascades to 4 other objects
 DETAIL:  drop cascades to rule voo_i on view voo
+drop cascades to view foo_v
 drop cascades to view joinview
 drop cascades to rule foo_del_rule on table foo
 UPDATE foo SET f4 = f4 + 1 RETURNING old.f3;  -- should fail
diff --git a/src/test/regress/sql/returning.sql b/src/test/regress/sql/returning.sql
index 54caf56244c..cc99cb53f63 100644
--- a/src/test/regress/sql/returning.sql
+++ b/src/test/regress/sql/returning.sql
@@ -132,6 +132,30 @@ DELETE FROM foo WHERE f2 = 'zit' RETURNING *;
 SELECT * FROM foo;
 SELECT * FROM voo;

+-- Check use of a whole-row variable for an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+SELECT * FROM foo;
+
+-- Check use of a whole-row variable for an inlined set-returning function
+CREATE FUNCTION foo_f() RETURNS SETOF foo AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+SELECT * FROM foo;
+DROP FUNCTION foo_f();
+
+-- As above, but SRF is defined to return a composite type
+CREATE TYPE foo_t AS (f1 int, f2 text, f3 int, f4 int8);
+CREATE FUNCTION foo_f() RETURNS SETOF foo_t AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+SELECT * FROM foo;
+DROP FUNCTION foo_f();
+DROP TYPE foo_t;
+
 -- Try a join case

 CREATE TEMP TABLE joinme (f2j text, other int);
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index d2a0e501d1e..30ae22e43df 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1859,8 +1859,12 @@ ApplyRetrieveRule(Query *parsetree,
     rte->rtekind = RTE_SUBQUERY;
     rte->subquery = rule_action;
     rte->security_barrier = RelationIsSecurityView(relation);
-    /* Clear fields that should not be set in a subquery RTE */
-    rte->relid = InvalidOid;
+
+    /*
+     * Clear fields that should not be set in a subquery RTE.  However, we
+     * retain the relid to support correct operation of makeWholeRowVar during
+     * planning.
+     */
     rte->relkind = 0;
     rte->rellockmode = 0;
     rte->tablesample = NULL;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 6944362c7ac..abc87c59e6f 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1017,6 +1017,12 @@ typedef struct RangeTblEntry
     /*
      * Fields valid for a plain relation RTE (else zero):
      *
+     * As a special case, relid can also be set in RTE_SUBQUERY RTEs.  This
+     * happens when an RTE_RELATION RTE for a view is transformed to an
+     * RTE_SUBQUERY during rewriting.  We keep the relid because it is useful
+     * during planning, cf makeWholeRowVar.  (It cannot be relied on during
+     * execution, because it will not propagate to parallel workers.)
+     *
      * As a special case, RTE_NAMEDTUPLESTORE can also set relid to indicate
      * that the tuple format of the tuplestore is the same as the referenced
      * relation.  This allows plans referencing AFTER trigger transition
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index c85d8fe9751..90f726d265e 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -157,6 +157,49 @@ makeWholeRowVar(RangeTblEntry *rte,
                              varlevelsup);
             break;

+        case RTE_SUBQUERY:
+
+            /*
+             * For a standard subquery, the Var should be of RECORD type.
+             * However, if we're looking at a subquery that was expanded from
+             * a view or SRF (only possible during planning), we must use the
+             * appropriate rowtype, so that the resulting Var has the same
+             * type that we would have produced from the original RTE.
+             */
+            if (OidIsValid(rte->relid))
+            {
+                /* Subquery was expanded from a view */
+                toid = get_rel_type_id(rte->relid);
+                if (!OidIsValid(toid))
+                    ereport(ERROR,
+                            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                             errmsg("relation \"%s\" does not have a composite type",
+                                    get_rel_name(rte->relid))));
+            }
+            else if (rte->functions)
+            {
+                /*
+                 * Subquery was expanded from a set-returning function.  That
+                 * would not have happened if there's more than one function
+                 * or ordinality was requested.  We also needn't worry about
+                 * the allowScalar case, since the planner doesn't use that.
+                 */
+                Assert(!allowScalar);
+                fexpr = ((RangeTblFunction *) linitial(rte->functions))->funcexpr;
+                toid = exprType(fexpr);
+                if (!type_is_rowtype(toid))
+                    toid = RECORDOID;
+            }
+            else
+                toid = RECORDOID;
+            result = makeVar(varno,
+                             InvalidAttrNumber,
+                             toid,
+                             -1,
+                             InvalidOid,
+                             varlevelsup);
+            break;
+
         case RTE_FUNCTION:

             /*
@@ -213,8 +256,8 @@ makeWholeRowVar(RangeTblEntry *rte,
         default:

             /*
-             * RTE is a join, subselect, tablefunc, or VALUES.  We represent
-             * this as a whole-row Var of RECORD type. (Note that in most
+             * RTE is a join, tablefunc, VALUES, CTE, etc.  We represent these
+             * cases as a whole-row Var of RECORD type.  (Note that in most
              * cases the Var will be expanded to a RowExpr during planning,
              * but that is not our concern here.)
              */
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 0efcc3b24bc..a91c5c1e361 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -744,8 +744,14 @@ preprocess_function_rtes(PlannerInfo *root)
                 rte->rtekind = RTE_SUBQUERY;
                 rte->subquery = funcquery;
                 rte->security_barrier = false;
-                /* Clear fields that should not be set in a subquery RTE */
-                rte->functions = NIL;
+
+                /*
+                 * Clear fields that should not be set in a subquery RTE.
+                 * However, we leave rte->functions filled in for the moment,
+                 * in case makeWholeRowVar needs to consult it.  We'll clear
+                 * it in setrefs.c (see add_rte_to_flat_rtable) so that this
+                 * abuse of the data structure doesn't escape the planner.
+                 */
                 rte->funcordinality = false;
             }
         }
diff --git a/src/test/regress/expected/returning.out b/src/test/regress/expected/returning.out
index cb51bb86876..a5ebc8acc0f 100644
--- a/src/test/regress/expected/returning.out
+++ b/src/test/regress/expected/returning.out
@@ -286,6 +286,63 @@ SELECT * FROM voo;
  16 | zoo2
 (2 rows)

+-- Check use of a whole-row variable for an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+      foo_v
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+-- Check use of a whole-row variable for an inlined set-returning function
+CREATE FUNCTION foo_f() RETURNS SETOF foo AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+      foo_f
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+DROP FUNCTION foo_f();
+-- As above, but SRF is defined to return a composite type
+CREATE TYPE foo_t AS (f1 int, f2 text, f3 int, f4 int8);
+CREATE FUNCTION foo_f() RETURNS SETOF foo_t AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+      foo_f
+-----------------
+ (2,more,42,141)
+ (16,zoo2,57,99)
+(2 rows)
+
+SELECT * FROM foo;
+ f1 |  f2  | f3 | f4
+----+------+----+-----
+  2 | more | 42 | 141
+ 16 | zoo2 | 57 |  99
+(2 rows)
+
+DROP FUNCTION foo_f();
+DROP TYPE foo_t;
 -- Try a join case
 CREATE TEMP TABLE joinme (f2j text, other int);
 INSERT INTO joinme VALUES('more', 12345);
diff --git a/src/test/regress/sql/returning.sql b/src/test/regress/sql/returning.sql
index a460f82fb7c..8a2a2a5861d 100644
--- a/src/test/regress/sql/returning.sql
+++ b/src/test/regress/sql/returning.sql
@@ -132,6 +132,30 @@ DELETE FROM foo WHERE f2 = 'zit' RETURNING *;
 SELECT * FROM foo;
 SELECT * FROM voo;

+-- Check use of a whole-row variable for an un-flattenable view
+CREATE TEMP VIEW foo_v AS SELECT * FROM foo OFFSET 0;
+UPDATE foo SET f2 = foo_v.f2 FROM foo_v WHERE foo_v.f1 = foo.f1
+  RETURNING foo_v;
+SELECT * FROM foo;
+
+-- Check use of a whole-row variable for an inlined set-returning function
+CREATE FUNCTION foo_f() RETURNS SETOF foo AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+SELECT * FROM foo;
+DROP FUNCTION foo_f();
+
+-- As above, but SRF is defined to return a composite type
+CREATE TYPE foo_t AS (f1 int, f2 text, f3 int, f4 int8);
+CREATE FUNCTION foo_f() RETURNS SETOF foo_t AS
+  $$ SELECT * FROM foo OFFSET 0 $$ LANGUAGE sql STABLE;
+UPDATE foo SET f2 = foo_f.f2 FROM foo_f() WHERE foo_f.f1 = foo.f1
+  RETURNING foo_f;
+SELECT * FROM foo;
+DROP FUNCTION foo_f();
+DROP TYPE foo_t;
+
 -- Try a join case

 CREATE TEMP TABLE joinme (f2j text, other int);

On Tue, 11 Mar 2025 at 21:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Yeah, that is a better way.  In exchange for a slightly dirtier
> data structure, we now have essentially all the relevant code in
> makeWholeRowVar: correctness only depends on its different case
> branches agreeing, rather than on some not-terribly-similar code
> way over in prepjointree.c.

Agreed. Having all the code in one place makes it easier to see that
it's doing the same thing before and after RTE expansion.

> I also took the opportunity to split off the old-branch adjustment
> of rewriteHandler.c, so that the HEAD and v15 versions of the main
> bug fix patch are nearly the same.

LGTM. I did some more testing and thought about it a little more, and
I can't see any other ways to break it.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> LGTM. I did some more testing and thought about it a little more, and
> I can't see any other ways to break it.

Thanks for the careful review and testing!  Pushed after fooling
with the comments a tiny bit more.

            regards, tom lane

Hi,

On 2025-03-12 11:49:29 -0400, Tom Lane wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> > LGTM. I did some more testing and thought about it a little more, and
> > I can't see any other ways to break it.
>
> Thanks for the careful review and testing!  Pushed after fooling
> with the comments a tiny bit more.

This seems to have introduce some breakage for 13-15. E.g. on
sifaka:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sifaka&dt=2025-03-12%2016%3A58%3A51
which has
    'CPPFLAGS' => '-DWRITE_READ_PARSE_PLAN_TREES -DSTRESS_SORT_INT_MIN -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS'

diff -U3 /Users/buildfarm/bf-data/REL_14_STABLE/pgsql.build/src/test/regress/expected/tablespace.out
/Users/buildfarm/bf-data/REL_14_STABLE/pgsql.build/src/test/regress/results/tablespace.out
--- /Users/buildfarm/bf-data/REL_14_STABLE/pgsql.build/src/test/regress/expected/tablespace.out    2025-03-12 12:59:22
+++ /Users/buildfarm/bf-data/REL_14_STABLE/pgsql.build/src/test/regress/results/tablespace.out    2025-03-12 12:59:23
@@ -242,6 +242,7 @@

 -- check \\d output
 \\d testschema.foo
+WARNING:  outfuncs/readfuncs failed to produce equal parse tree
               Table "testschema.foo"
  Column |  Type   | Collation | Nullable | Default
 --------+---------+-----------+----------+---------
@@ -320,6 +321,7 @@
 (3 rows)
...

Greetings,

Andres

Andres Freund <andres@anarazel.de> writes:
> This seems to have introduce some breakage for 13-15. E.g. on
> sifaka:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sifaka&dt=2025-03-12%2016%3A58%3A51
> which has
>     'CPPFLAGS' => '-DWRITE_READ_PARSE_PLAN_TREES -DSTRESS_SORT_INT_MIN -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS'

Ugh.  I supposed that it was okay that 317aba70e etc. didn't touch
outfuncs/readfuncs, but I did not think of
-DWRITE_READ_PARSE_PLAN_TREES.

Perhaps a good hack to deal with that is to make setrefs.c clear
out relid for RTE_SUBQUERY RTEs in those branches.  Then, in the
same way that the rte->function hack doesn't escape the planner,
this one wouldn't either.

            regards, tom lane

I wrote:
> Ugh.  I supposed that it was okay that 317aba70e etc. didn't touch
> outfuncs/readfuncs, but I did not think of
> -DWRITE_READ_PARSE_PLAN_TREES.

> Perhaps a good hack to deal with that is to make setrefs.c clear
> out relid for RTE_SUBQUERY RTEs in those branches.  Then, in the
> same way that the rte->function hack doesn't escape the planner,
> this one wouldn't either.

Double ugh: that doesn't fix it, because we also do a round of
WRITE_READ_PARSE_PLAN_TREES checks on the rewriter output.
Not sure how to fix this, unless we lobotomize that write/read
check somehow.

            regards, tom lane

I wrote:
> Double ugh: that doesn't fix it, because we also do a round of
> WRITE_READ_PARSE_PLAN_TREES checks on the rewriter output.
> Not sure how to fix this, unless we lobotomize that write/read
> check somehow.

After some thought, I'm inclined to suggest that we just remove
the post-rewrite check in the affected branches.  This is certainly
not an ideal solution, but unless someone can think of a
fundamentally different way to fix the original bug in these branches,
I don't see a better way.  There are some mitigating points:

* Applying WRITE_READ_PARSE_PLAN_TREES here does not correspond to
any actual system functionality requirement.  We have to be able
to write/read post-parse-analysis trees to store views and rules;
and we have to be able to write/read plan trees to transfer them
to parallel workers.  But there's no real reason why we have to
be able to do it for the purely-transient output of rewriting.

* We aren't going to be changing the outfuncs/readfuncs code
in these branches anymore anyway, for precisely the same reasons
that this patchset can't do that.  So it's not clear what the
test is doing for us.

So that's the best I've got for tonight.  I've verified that the
attached patch for v15 fixes this failure.

            regards, tom lane

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 4d05d59c5bd..59387c21b51 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -512,6 +512,15 @@ add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte)
     newrte->colcollations = NIL;
     newrte->securityQuals = NIL;

+    /*
+     * Also, if it's a subquery RTE, lose the relid that may have been kept to
+     * signal that it had been a view.  We don't want that to escape the
+     * planner, mainly because doing so breaks -DWRITE_READ_PARSE_PLAN_TREES
+     * testing thanks to outfuncs/readfuncs not preserving it.
+     */
+    if (newrte->rtekind == RTE_SUBQUERY)
+        newrte->relid = InvalidOid;
+
     glob->finalrtable = lappend(glob->finalrtable, newrte);

     /*
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 30ae22e43df..aa0a3bfe89b 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1862,8 +1862,8 @@ ApplyRetrieveRule(Query *parsetree,

     /*
      * Clear fields that should not be set in a subquery RTE.  However, we
-     * retain the relid to support correct operation of makeWholeRowVar during
-     * planning.
+     * retain the relid for now, to support correct operation of
+     * makeWholeRowVar during planning.
      */
     rte->relkind = 0;
     rte->rellockmode = 0;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8a464fcf150..a6a13dc71ba 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -813,45 +813,10 @@ pg_rewrite_query(Query *query)
     }
 #endif

-#ifdef WRITE_READ_PARSE_PLAN_TREES
-    /* Optional debugging check: pass querytree through outfuncs/readfuncs */
-    {
-        List       *new_list = NIL;
-        ListCell   *lc;
-
-        /*
-         * We currently lack outfuncs/readfuncs support for most utility
-         * statement types, so only attempt to write/read non-utility queries.
-         */
-        foreach(lc, querytree_list)
-        {
-            Query       *query = lfirst_node(Query, lc);
-
-            if (query->commandType != CMD_UTILITY)
-            {
-                char       *str = nodeToString(query);
-                Query       *new_query = stringToNodeWithLocations(str);
-
-                /*
-                 * queryId is not saved in stored rules, but we must preserve
-                 * it here to avoid breaking pg_stat_statements.
-                 */
-                new_query->queryId = query->queryId;
-
-                new_list = lappend(new_list, new_query);
-                pfree(str);
-            }
-            else
-                new_list = lappend(new_list, query);
-        }
-
-        /* This checks both outfuncs/readfuncs and the equal() routines... */
-        if (!equal(new_list, querytree_list))
-            elog(WARNING, "outfuncs/readfuncs failed to produce equal parse tree");
-        else
-            querytree_list = new_list;
-    }
-#endif
+    /*
+     * We don't apply WRITE_READ_PARSE_PLAN_TREES to rewritten query trees,
+     * because it breaks the hack of preserving relid for rewritten views.
+     */

     if (Debug_print_rewritten)
         elog_node_display(LOG, "rewritten parse tree", querytree_list,
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index abc87c59e6f..a77b5ba1b14 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1020,8 +1020,8 @@ typedef struct RangeTblEntry
      * As a special case, relid can also be set in RTE_SUBQUERY RTEs.  This
      * happens when an RTE_RELATION RTE for a view is transformed to an
      * RTE_SUBQUERY during rewriting.  We keep the relid because it is useful
-     * during planning, cf makeWholeRowVar.  (It cannot be relied on during
-     * execution, because it will not propagate to parallel workers.)
+     * during planning, cf makeWholeRowVar.  (It will not be passed on to the
+     * executor, however.)
      *
      * As a special case, RTE_NAMEDTUPLESTORE can also set relid to indicate
      * that the tuple format of the tuplestore is the same as the referenced

On Thu, 13 Mar 2025 at 01:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> After some thought, I'm inclined to suggest that we just remove
> the post-rewrite check in the affected branches.
>

I don't really have any better ideas. I guess that if we wanted to
keep this check in back branches, we could copy across the subquery
RTE relids onto the new query, like we do for queryIds. But it would
be somewhat messy, because we'd have to do something like walk each
query pulling a list of its RTE_SUBQUERY RTEs, and then walk both
lists, copying relids over. So maybe not worth the effort.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Thu, 13 Mar 2025 at 01:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> After some thought, I'm inclined to suggest that we just remove
>> the post-rewrite check in the affected branches.

> I don't really have any better ideas. I guess that if we wanted to
> keep this check in back branches, we could copy across the subquery
> RTE relids onto the new query, like we do for queryIds. But it would
> be somewhat messy, because we'd have to do something like walk each
> query pulling a list of its RTE_SUBQUERY RTEs, and then walk both
> lists, copying relids over. So maybe not worth the effort.

Yeah, I don't think it's worth it either -- testing something that's
not actually the behavior of the code-under-test seems pointless.

I've not thought of any better way, so pushed that one to get the
buildfarm back to green.  There's always "git revert" if somebody
has a better idea.

            regards, tom lane