From 99be6feec9717058b9ac9d80f3584e0a9a6e8580 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 7 Mar 2023 18:21:37 -0500 Subject: [PATCH] Fix more bugs caused by adding columns to the end of a view. If a view is defined atop another view, and then CREATE OR REPLACE VIEW is used to add columns to the lower view, then when the upper view's referencing RTE is expanded by ApplyRetrieveRule we will have a subquery RTE with fewer eref->colnames than output columns. This confuses various code that assumes those lists are always in sync, as they are in plain parser output. We have seen such problems before (cf commit d5b760ecb), and now I think the time has come to do what was speculated about in that commit: let's make ApplyRetrieveRule synthesize some column names to preserve the invariant that holds in parser output. Otherwise we'll be chasing this class of bugs indefinitely. Moreover, it appears from testing that this actually gives us better results in the test case d5b760ecb added, and likely in other corner cases that we lack coverage for. In HEAD, I replaced d5b760ecb's hack to make expandRTE exit early with an elog(ERROR) call, since the case is now presumably unreachable. But it seems like changing that in back branches would bring more risk than benefit, so there I just updated the comment. Per bug #17811 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/17811-d31686b78f0dffc9@postgresql.org --- src/backend/parser/parse_relation.c | 15 +++++---- src/backend/rewrite/rewriteHandler.c | 16 +++++++++ src/test/regress/expected/alter_table.out | 41 +++++++++++++++++++---- src/test/regress/sql/alter_table.sql | 18 ++++++++++ 4 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c index de355dd246..41d60494b9 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -2697,15 +2697,16 @@ expandRTE(RangeTblEntry *rte, int rtindex, int sublevels_up, Assert(varattno == te->resno); /* - * In scenarios where columns have been added to a view - * since the outer query was originally parsed, there can - * be more items in the subquery tlist than the outer - * query expects. We should ignore such extra column(s) - * --- compare the behavior for composite-returning - * functions, in the RTE_FUNCTION case below. + * Formerly it was possible for the subquery tlist to have + * more non-junk entries than the colnames list does (if + * this RTE has been expanded from a view that has more + * columns than it did when the current query was parsed). + * Now that ApplyRetrieveRule cleans up such cases, we + * shouldn't see that anymore, but let's just check. */ if (!aliasp_item) - break; + elog(ERROR, "too few column names for subquery %s", + rte->eref->aliasname); if (colnames) { 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..27b4d7dc96 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -2551,21 +2551,50 @@ 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; drop view at_view_1; drop table at_base_table; +-- related case (bug #17811) +begin; +create temp table t1 as select * from int8_tbl; +create temp view v1 as select 1::int8 as q1; +create temp view v2 as select * from v1; +create or replace temp view v1 with (security_barrier = true) + as select * from t1; +create temp table log (q1 int8, q2 int8); +create rule v1_upd_rule as on update to v1 + do also insert into log values (new.*); +update v2 set q1 = q1 + 1 where q1 = 123; +select * from t1; + q1 | q2 +------------------+------------------- + 4567890123456789 | 123 + 4567890123456789 | 4567890123456789 + 4567890123456789 | -4567890123456789 + 124 | 456 + 124 | 4567890123456789 +(5 rows) + +select * from log; + q1 | q2 +-----+------------------ + 124 | 456 + 124 | 4567890123456789 +(2 rows) + +rollback; -- check adding a column not itself requiring a rewrite, together with -- a column requiring a default (bug #16038) -- ensure that rewrites aren't silently optimized away, removing the diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index b5d57a771a..7dc9e3d632 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1635,6 +1635,24 @@ drop view at_view_2; drop view at_view_1; drop table at_base_table; +-- related case (bug #17811) +begin; +create temp table t1 as select * from int8_tbl; +create temp view v1 as select 1::int8 as q1; +create temp view v2 as select * from v1; +create or replace temp view v1 with (security_barrier = true) + as select * from t1; + +create temp table log (q1 int8, q2 int8); +create rule v1_upd_rule as on update to v1 + do also insert into log values (new.*); + +update v2 set q1 = q1 + 1 where q1 = 123; + +select * from t1; +select * from log; +rollback; + -- check adding a column not itself requiring a rewrite, together with -- a column requiring a default (bug #16038)