From: Tom Lane <tgl@sss.pgh.pa.us>
To: exclusion@gmail.com
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17811: Replacing an underlying view breaks OLD/NEW tuple
 when accessing it via upper-level view
In-reply-to: <17811-d31686b78f0dffc9@postgresql.org>
References: <17811-d31686b78f0dffc9@postgresql.org>
Comments: In-reply-to PG Bug reporting form <noreply@postgresql.org>
	message dated "Mon, 27 Feb 2023 20:00:03 +0000"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <4022262.1678214568.0@sss.pgh.pa.us>
Date: Tue, 07 Mar 2023 13:43:05 -0500
Message-ID: <4022282.1678214585@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/4022282.1678214585%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4022262.1678214568.1@sss.pgh.pa.us>

PG Bug reporting form <noreply@postgresql.org> writes:
> I've discovered an issue with replacing a view when there is another
> updatable view defined on top of it and the new underlying view has
> more columns than the previous one.

I've looked into this a bit more, and both of these symptoms reduce
to the same thing: after we've plugged the modified view's query
into the upper query, we have an RTE_SUBQUERY RTE whose eref->colnames
list is shorter than the number of columns actually available from the
subquery.  This breaks assorted code that is expecting that it can
use list_length(eref->colnames) as a quick guide to the number of
output columns.

We have seen this before (bug #14876, commit d5b760ecb) and at the
time our response was to patch up the code making such an assumption.
But you've just demonstrated at least two other places with the same
issue, so now I'm feeling that we'd be best advised to fix it
centrally in the rewriter code that plugs in the view definition.
The attached fix causes a change in the regression test output for
bug #14876's test case, but AFAICS the change is a strict improvement:
we get something sane, not a NULL, for the added column.

I'm not sure whether to change the code added to expandRTE in
d5b760ecb or leave it be.  We could make it into elog(ERROR) now,
perhaps.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-bug-17811.patch"; charset="us-ascii"
Content-ID: <4022262.1678214568.2@sss.pgh.pa.us>
Content-Description: fix-bug-17811.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/re=
writeHandler.c
index a614e3f5bd..980dc1816f 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -26,6 +26,7 @@
 #include "catalog/dependency.h"
 #include "catalog/pg_type.h"
 #include "commands/trigger.h"
+#include "executor/executor.h"
 #include "foreign/fdwapi.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
@@ -1730,6 +1731,7 @@ ApplyRetrieveRule(Query *parsetree,
 	Query	   *rule_action;
 	RangeTblEntry *rte;
 	RowMarkClause *rc;
+	int			numCols;
 =

 	if (list_length(rule->actions) !=3D 1)
 		elog(ERROR, "expected just one rule action");
@@ -1855,6 +1857,20 @@ ApplyRetrieveRule(Query *parsetree,
 	rte->tablesample =3D NULL;
 	rte->inh =3D false;			/* must not be set for a subquery */
 =

+	/*
+	 * Since we allow CREATE OR REPLACE VIEW to add columns to a view, the
+	 * rule_action might emit more columns than we expected when the current
+	 * query was parsed.  Various places expect rte->eref->colnames to be
+	 * consistent with the non-junk output columns of the subquery, so patch
+	 * things up if necessary by adding some dummy column names.
+	 */
+	numCols =3D ExecCleanTargetListLength(rule_action->targetList);
+	while (list_length(rte->eref->colnames) < numCols)
+	{
+		rte->eref->colnames =3D lappend(rte->eref->colnames,
+									  makeString(pstrdup("?column?")));
+	}
+
 	return parsetree;
 }
 =

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/=
expected/alter_table.out
index 97bfc3475b..5e5b31f6ed 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2551,16 +2551,16 @@ View definition:
    FROM at_view_1 v1;
 =

 explain (verbose, costs off) select * from at_view_2;
-                           QUERY PLAN                           =

-----------------------------------------------------------------
+                         QUERY PLAN                          =

+-------------------------------------------------------------
  Seq Scan on public.at_base_table bt
-   Output: bt.id, bt.stuff, to_json(ROW(bt.id, bt.stuff, NULL))
+   Output: bt.id, bt.stuff, to_json(ROW(bt.id, bt.stuff, 4))
 (2 rows)
 =

 select * from at_view_2;
- id | stuff  |                   j                    =

-----+--------+----------------------------------------
- 23 | skidoo | {"id":23,"stuff":"skidoo","more":null}
+ id | stuff  |                  j                  =

+----+--------+-------------------------------------
+ 23 | skidoo | {"id":23,"stuff":"skidoo","more":4}
 (1 row)
 =

 drop view at_view_2;

------- =_aaaaaaaaaa0--