Thread: BUG #17811: Replacing an underlying view breaks OLD/NEW tuple when accessing it via upper-level view

The following bug has been logged on the website:

Bug reference:      17811
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 15.2
Operating system:   Ubuntu 22.04
Description:

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.

First example with triggers:
CREATE TABLE t (id int, f1 text);
CREATE VIEW v1 AS SELECT id FROM t;
CREATE VIEW v2 AS SELECT * FROM v1;
CREATE OR REPLACE VIEW v1 AS SELECT * FROM t;
CREATE FUNCTION v1_trig_fn() RETURNS trigger AS $$
BEGIN 
  RAISE NOTICE 'old.id: %, old.f1: %', old.id, old.f1;
  UPDATE t SET id=new.id WHERE id=old.id; 
  RETURN new;
END; $$ LANGUAGE plpgsql;
CREATE TRIGGER v1_upd_trig INSTEAD OF UPDATE ON v1
 FOR EACH ROW EXECUTE FUNCTION v1_trig_fn();
INSERT INTO t VALUES (1, 11), (2, 22);
SELECT * FROM t;
UPDATE v2 SET id = 3 WHERE id = 2;
SELECT * FROM t;

 id | f1 
----+----
  1 | 11
  2 | 22
(2 rows)

NOTICE:  old.id: 2, old.f1: <NULL> -- ???
UPDATE 1
 id | f1 
----+----
  1 | 11
  3 | 22
(2 rows)


but if new v1  defined with security_barrier:
CREATE OR REPLACE VIEW v1 WITH (security_barrier = true) AS SELECT * FROM
t;
...
NOTICE:  old.id: 2, old.f1: 22, old.f2: 222 -- OK

but on master (after 47bb9db75)
...
NOTICE:  old.id: 2, old.f1: 22, old.f2: 222
WARNING:  problem in alloc set MessageContext: req size > alloc size for
chunk 0x62900003ccb0 in block 0x62900003c200
WARNING:  problem in alloc set MessageContext: req size > alloc size for
chunk 0x62900003ccb0 in block 0x62900003c200
(I've seen also runtime errors detected by asan.)

Second example with rules (REL_15_STABLE without asserts):
CREATE TABLE t(id int, f1 int, f2 int, f3 int, f4 int);
CREATE TABLE tc(id int, f int);

CREATE VIEW v1 AS SELECT 1 AS id;
CREATE VIEW v2 AS SELECT * FROM v1;
CREATE OR REPLACE VIEW v1 WITH (security_barrier = true) AS SELECT * FROM
t;

CREATE RULE v1_upd_rule AS ON UPDATE TO v1
  DO INSTEAD (INSERT INTO tc VALUES (NEW.id, NEW.f1); UPDATE t SET id =
NEW.id WHERE id = OLD.id;);

INSERT INTO t VALUES (1, 11, 111, 1111, 11111), (2, 22, 222, 2222, 22222);

SELECT * FROM t;
SELECT * FROM v2;

UPDATE v2 SET id = 3 WHERE id = 2;
SELECT * FROM t;
SELECT * FROM tc;

 id 
----
  1
  2
(2 rows)

server closed the connection unexpectedly
...
Program terminated with signal SIGSEGV, Segmentation fault.

#0  0x0000555b19cd22b6 in bms_add_members (a=0x8, b=b@entry=0x555b1a645990)
at bitmapset.c:808
808             if (a->nwords < b->nwords)
(gdb) bt
#0  0x0000555b19cd22b6 in bms_add_members (a=0x8, b=b@entry=0x555b1a645990)
at bitmapset.c:808
#1  0x0000555b19d29273 in add_vars_to_targetlist (root=0x555b1a645088,
vars=0x555b1a645940, 
    where_needed=0x555b1a645990, create_new_ph=true) at initsplan.c:259
#2  0x0000555b19d2931d in build_base_rel_tlists (root=0x555b1a645088,
final_tlist=<optimized out>) at initsplan.c:192
#3  0x0000555b19d2be26 in query_planner (root=root@entry=0x555b1a645088, 
    qp_callback=qp_callback@entry=0x555b19d2c430 <standard_qp_callback>,
qp_extra=qp_extra@entry=0x7fffd186cd10)
    at planmain.c:178
...

But with:
CREATE RULE v1_upd_rule AS ON UPDATE TO v1
  DO INSTEAD (INSERT INTO tc VALUES (NEW.id, NEW.f3); UPDATE t SET id =
NEW.id WHERE id = OLD.id;);

...
 id 
----
  1
  2
(2 rows)

UPDATE 1
 id | f1 | f2  |  f3  |  f4   
----+----+-----+------+-------
  1 | 11 | 111 | 1111 | 11111
  3 | 22 | 222 | 2222 | 22222
(2 rows)

 id |  f   
----+------
  3 | 2222
(1 row)
(Maybe it succeeded accidentally.)

REL_15_STABLE with asserts:
TRAP: FailedAssertion("attno >= rel->min_attr && attno <= rel->max_attr",
File: "initsplan.c", Line: 249, PID: 266170)

Interestingly enough, but replacing an underlying view with incompatible
fields 
is prohibited (thanks to a check in checkViewTupleDesc()):
CREATE VIEW v1 AS SELECT 1::text AS id;
CREATE VIEW v2 AS SELECT * FROM v1;
CREATE OR REPLACE VIEW v1 WITH (security_barrier = true) AS SELECT * FROM
t;
ERROR:  cannot change data type of view column "id" from text to integer

Just as decreasing of a number of columns:
CREATE VIEW v1 AS SELECT 1 AS id, 11 AS f1, 111 AS f2, 1111 AS f3, 11111 AS
f4, 111111 AS f5;
CREATE VIEW v2 AS SELECT * FROM v1;
CREATE OR REPLACE VIEW v1 WITH (security_barrier = true) AS SELECT * FROM
t;
ERROR:  cannot drop columns from view

Maybe replacing a view with increasing number of columns should be
prohibited too?

PG Bug reporting form <noreply@postgresql.org> writes:
> Maybe replacing a view with increasing number of columns should be
> prohibited too?

It's intentional that we allow that case; it'd be sad if we have
to remove the feature because of bugs.

            regards, tom lane

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

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.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) != 1)
         elog(ERROR, "expected just one rule action");
@@ -1855,6 +1857,20 @@ ApplyRetrieveRule(Query *parsetree,
     rte->tablesample = NULL;
     rte->inh = 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 = ExecCleanTargetListLength(rule_action->targetList);
+    while (list_length(rte->eref->colnames) < numCols)
+    {
+        rte->eref->colnames = 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;

07.03.2023 21:43, Tom Lane wrote:
> 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.

Thanks for the fix!
I've tested all my cases on REL_15_STABLE, master and found no
anomalies in this area.

Best regards,
Alexander