Skip to content

Commit ad77039

Browse files
tglsfdctglsfdc
committed
Calculate extraUpdatedCols in query rewriter, not parser.
It's unsafe to do this at parse time because addition of generated columns to a table would not invalidate stored rules containing UPDATEs on the table ... but there might now be dependent generated columns that were not there when the rule was made. This also fixes an oversight that rewriteTargetView failed to update extraUpdatedCols when transforming an UPDATE on an updatable view. (Since the new calculation is downstream of that, rewriteTargetView doesn't actually need to do anything; but before, there was a demonstrable bug there.) In v13 and HEAD, this leads to easily-visible bugs because (since commit c6679e4) we won't recalculate generated columns that aren't listed in extraUpdatedCols. In v12 this bitmap is mostly just used for trigger-firing decisions, so you'd only notice a problem if a trigger cared whether a generated column had been updated. I'd complained about this back in May, but then forgot about it until bug #16671 from Michael Paul Killian revived the issue. Back-patch to v12 where this field was introduced. If existing stored rules contain any extraUpdatedCols values, they'll be ignored because the rewriter will overwrite them, so the bug will be fixed even for existing rules. (But note that if someone were to update to 13.1 or 12.5, store some rules with UPDATEs on tables having generated columns, and then downgrade to a prior minor version, they might observe issues similar to what this patch fixes. That seems unlikely enough to not be worth going to a lot of effort to fix.) Discussion: https://postgr.es/m/10206.1588964727@sss.pgh.pa.us Discussion: https://postgr.es/m/16671-2fa55851859fb166@postgresql.org
1 parent 36b9312 commit ad77039

File tree

9 files changed

+115
-47
lines changed

9 files changed

+115
-47
lines changed

‎src/backend/optimizer/plan/setrefs.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -438,9 +438,9 @@ flatten_rtes_walker(Node *node, PlannerGlobal *glob)
438438
* In the flat rangetable, we zero out substructure pointers that are not
439439
* needed by the executor; this reduces the storage space and copying cost
440440
* for cached plans. We keep only the ctename, alias and eref Alias fields,
441-
* which are needed by EXPLAIN, and the selectedCols, insertedCols and
442-
* updatedCols bitmaps, which are needed for executor-startup permissions
443-
* checking and for trigger event checking.
441+
* which are needed by EXPLAIN, and the selectedCols, insertedCols,
442+
* updatedCols, and extraUpdatedCols bitmaps, which are needed for
443+
* executor-startup permissions checking and for trigger event checking.
444444
*/
445445
static void
446446
add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte)

‎src/backend/parser/analyze.c

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -2282,7 +2282,6 @@ transformUpdateTargetList(ParseState *pstate, List *origTlist)
22822282
RangeTblEntry *target_rte;
22832283
ListCell *orig_tl;
22842284
ListCell *tl;
2285-
TupleDesc tupdesc = pstate->p_target_relation->rd_att;
22862285

22872286
tlist = transformTargetList(pstate, origTlist,
22882287
EXPR_KIND_UPDATE_SOURCE);
@@ -2341,41 +2340,9 @@ transformUpdateTargetList(ParseState *pstate, List *origTlist)
23412340
if (orig_tl != NULL)
23422341
elog(ERROR, "UPDATE target count mismatch --- internal error");
23432342

2344-
fill_extraUpdatedCols(target_rte, tupdesc);
2345-
23462343
return tlist;
23472344
}
23482345

2349-
/*
2350-
* Record in extraUpdatedCols generated columns referencing updated base
2351-
* columns.
2352-
*/
2353-
void
2354-
fill_extraUpdatedCols(RangeTblEntry *target_rte, TupleDesc tupdesc)
2355-
{
2356-
if (tupdesc->constr &&
2357-
tupdesc->constr->has_generated_stored)
2358-
{
2359-
for (int i = 0; i < tupdesc->constr->num_defval; i++)
2360-
{
2361-
AttrDefault defval = tupdesc->constr->defval[i];
2362-
Node *expr;
2363-
Bitmapset *attrs_used = NULL;
2364-
2365-
/* skip if not generated column */
2366-
if (!TupleDescAttr(tupdesc, defval.adnum - 1)->attgenerated)
2367-
continue;
2368-
2369-
expr = stringToNode(defval.adbin);
2370-
pull_varattnos(expr, 1, &attrs_used);
2371-
2372-
if (bms_overlap(target_rte->updatedCols, attrs_used))
2373-
target_rte->extraUpdatedCols = bms_add_member(target_rte->extraUpdatedCols,
2374-
defval.adnum - FirstLowInvalidHeapAttributeNumber);
2375-
}
2376-
}
2377-
}
2378-
23792346
/*
23802347
* transformReturningList -
23812348
* handle a RETURNING clause in INSERT/UPDATE/DELETE

‎src/backend/replication/logical/worker.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,6 @@
8181
#include "miscadmin.h"
8282
#include "nodes/makefuncs.h"
8383
#include "optimizer/optimizer.h"
84-
#include "parser/analyze.h"
85-
#include "parser/parse_relation.h"
8684
#include "pgstat.h"
8785
#include "postmaster/bgworker.h"
8886
#include "postmaster/interrupt.h"
@@ -1323,7 +1321,8 @@ apply_handle_update(StringInfo s)
13231321
}
13241322
}
13251323

1326-
fill_extraUpdatedCols(target_rte, RelationGetDescr(rel->localrel));
1324+
/* Also populate extraUpdatedCols, in case we have generated columns */
1325+
fill_extraUpdatedCols(target_rte, rel->localrel);
13271326

13281327
PushActiveSnapshot(GetTransactionSnapshot());
13291328

‎src/backend/rewrite/rewriteHandler.c

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "miscadmin.h"
3131
#include "nodes/makefuncs.h"
3232
#include "nodes/nodeFuncs.h"
33+
#include "optimizer/optimizer.h"
3334
#include "parser/analyze.h"
3435
#include "parser/parse_coerce.h"
3536
#include "parser/parse_relation.h"
@@ -1508,6 +1509,42 @@ rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
15081509
}
15091510
}
15101511

1512+
/*
1513+
* Record in target_rte->extraUpdatedCols the indexes of any generated columns
1514+
* that depend on any columns mentioned in target_rte->updatedCols.
1515+
*/
1516+
void
1517+
fill_extraUpdatedCols(RangeTblEntry *target_rte, Relation target_relation)
1518+
{
1519+
TupleDesc tupdesc = RelationGetDescr(target_relation);
1520+
TupleConstr *constr = tupdesc->constr;
1521+
1522+
target_rte->extraUpdatedCols = NULL;
1523+
1524+
if (constr && constr->has_generated_stored)
1525+
{
1526+
for (int i = 0; i < constr->num_defval; i++)
1527+
{
1528+
AttrDefault *defval = &constr->defval[i];
1529+
Node *expr;
1530+
Bitmapset *attrs_used = NULL;
1531+
1532+
/* skip if not generated column */
1533+
if (!TupleDescAttr(tupdesc, defval->adnum - 1)->attgenerated)
1534+
continue;
1535+
1536+
/* identify columns this generated column depends on */
1537+
expr = stringToNode(defval->adbin);
1538+
pull_varattnos(expr, 1, &attrs_used);
1539+
1540+
if (bms_overlap(target_rte->updatedCols, attrs_used))
1541+
target_rte->extraUpdatedCols =
1542+
bms_add_member(target_rte->extraUpdatedCols,
1543+
defval->adnum - FirstLowInvalidHeapAttributeNumber);
1544+
}
1545+
}
1546+
}
1547+
15111548

15121549
/*
15131550
* matchLocks -
@@ -1639,6 +1676,7 @@ ApplyRetrieveRule(Query *parsetree,
16391676
rte->selectedCols = NULL;
16401677
rte->insertedCols = NULL;
16411678
rte->updatedCols = NULL;
1679+
rte->extraUpdatedCols = NULL;
16421680

16431681
/*
16441682
* For the most part, Vars referencing the view should remain as
@@ -3617,6 +3655,9 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
36173655
parsetree->override,
36183656
rt_entry_relation,
36193657
parsetree->resultRelation);
3658+
3659+
/* Also populate extraUpdatedCols (for generated columns) */
3660+
fill_extraUpdatedCols(rt_entry, rt_entry_relation);
36203661
}
36213662
else if (event == CMD_DELETE)
36223663
{

‎src/include/nodes/parsenodes.h

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -940,12 +940,16 @@ typedef struct PartitionCmd
940940
*
941941
* updatedCols is also used in some other places, for example, to determine
942942
* which triggers to fire and in FDWs to know which changed columns they
943-
* need to ship off. Generated columns that are caused to be updated by an
944-
* update to a base column are collected in extraUpdatedCols. This is not
945-
* considered for permission checking, but it is useful in those places
946-
* that want to know the full set of columns being updated as opposed to
947-
* only the ones the user explicitly mentioned in the query. (There is
948-
* currently no need for an extraInsertedCols, but it could exist.)
943+
* need to ship off.
944+
*
945+
* Generated columns that are caused to be updated by an update to a base
946+
* column are listed in extraUpdatedCols. This is not considered for
947+
* permission checking, but it is useful in those places that want to know
948+
* the full set of columns being updated as opposed to only the ones the
949+
* user explicitly mentioned in the query. (There is currently no need for
950+
* an extraInsertedCols, but it could exist.) Note that extraUpdatedCols
951+
* is populated during query rewrite, NOT in the parser, since generated
952+
* columns could be added after a rule has been parsed and stored.
949953
*
950954
* securityQuals is a list of security barrier quals (boolean expressions),
951955
* to be tested in the listed order before returning a row from the

‎src/include/parser/analyze.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,4 @@ extern void applyLockingClause(Query *qry, Index rtindex,
4646
extern List *BuildOnConflictExcludedTargetlist(Relation targetrel,
4747
Index exclRelIndex);
4848

49-
extern void fill_extraUpdatedCols(RangeTblEntry *target_rte, TupleDesc tupdesc);
50-
5149
#endif /* ANALYZE_H */

‎src/include/rewrite/rewriteHandler.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ extern Node *build_column_default(Relation rel, int attrno);
2626
extern void rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
2727
Relation target_relation);
2828

29+
extern void fill_extraUpdatedCols(RangeTblEntry *target_rte,
30+
Relation target_relation);
31+
2932
extern Query *get_view_query(Relation view);
3033
extern const char *view_query_is_auto_updatable(Query *viewquery,
3134
bool check_cols);

‎src/test/regress/expected/updatable_views.out

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1467,6 +1467,41 @@ NOTICE: drop cascades to 3 other objects
14671467
DETAIL: drop cascades to view rw_view1
14681468
drop cascades to view rw_view2
14691469
drop cascades to view rw_view3
1470+
-- view on table with GENERATED columns
1471+
CREATE TABLE base_tbl (id int, idplus1 int GENERATED ALWAYS AS (id + 1) STORED);
1472+
CREATE VIEW rw_view1 AS SELECT * FROM base_tbl;
1473+
INSERT INTO base_tbl (id) VALUES (1);
1474+
INSERT INTO rw_view1 (id) VALUES (2);
1475+
INSERT INTO base_tbl (id, idplus1) VALUES (3, DEFAULT);
1476+
INSERT INTO rw_view1 (id, idplus1) VALUES (4, DEFAULT);
1477+
INSERT INTO base_tbl (id, idplus1) VALUES (5, 6); -- error
1478+
ERROR: cannot insert into column "idplus1"
1479+
DETAIL: Column "idplus1" is a generated column.
1480+
INSERT INTO rw_view1 (id, idplus1) VALUES (6, 7); -- error
1481+
ERROR: cannot insert into column "idplus1"
1482+
DETAIL: Column "idplus1" is a generated column.
1483+
SELECT * FROM base_tbl;
1484+
id | idplus1
1485+
----+---------
1486+
1 | 2
1487+
2 | 3
1488+
3 | 4
1489+
4 | 5
1490+
(4 rows)
1491+
1492+
UPDATE base_tbl SET id = 2000 WHERE id = 2;
1493+
UPDATE rw_view1 SET id = 3000 WHERE id = 3;
1494+
SELECT * FROM base_tbl;
1495+
id | idplus1
1496+
------+---------
1497+
1 | 2
1498+
4 | 5
1499+
2000 | 2001
1500+
3000 | 3001
1501+
(4 rows)
1502+
1503+
DROP TABLE base_tbl CASCADE;
1504+
NOTICE: drop cascades to view rw_view1
14701505
-- inheritance tests
14711506
CREATE TABLE base_tbl_parent (a int);
14721507
CREATE TABLE base_tbl_child (CHECK (a > 0)) INHERITS (base_tbl_parent);

‎src/test/regress/sql/updatable_views.sql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -697,6 +697,27 @@ SELECT events & 4 != 0 AS upd,
697697

698698
DROP TABLE base_tbl CASCADE;
699699

700+
-- view on table with GENERATED columns
701+
702+
CREATE TABLE base_tbl (id int, idplus1 int GENERATED ALWAYS AS (id + 1) STORED);
703+
CREATE VIEW rw_view1 AS SELECT * FROM base_tbl;
704+
705+
INSERT INTO base_tbl (id) VALUES (1);
706+
INSERT INTO rw_view1 (id) VALUES (2);
707+
INSERT INTO base_tbl (id, idplus1) VALUES (3, DEFAULT);
708+
INSERT INTO rw_view1 (id, idplus1) VALUES (4, DEFAULT);
709+
INSERT INTO base_tbl (id, idplus1) VALUES (5, 6); -- error
710+
INSERT INTO rw_view1 (id, idplus1) VALUES (6, 7); -- error
711+
712+
SELECT * FROM base_tbl;
713+
714+
UPDATE base_tbl SET id = 2000 WHERE id = 2;
715+
UPDATE rw_view1 SET id = 3000 WHERE id = 3;
716+
717+
SELECT * FROM base_tbl;
718+
719+
DROP TABLE base_tbl CASCADE;
720+
700721
-- inheritance tests
701722

702723
CREATE TABLE base_tbl_parent (a int);

0 commit comments

Comments
 (0)