From 22f180dd9e51bdcc44fefe4b8186b59d6622f3dc Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" Date: Tue, 27 Oct 2026 21:40:40 -0700 Subject: [PATCH v1 4/6] Apply FOR PORTION OF to DO INSTEAD rules on views On a view with a DO INSTEAD rule, FOR PORTION OF was silently ignored. The rule actions were built before the view's query got the overlaps qual and range assignment, so they fired for every row and NEW held the whole range. Built them from a copy that has those details, as tables already did. Now the actions fire only for overlapping rows, NEW holds the targeted portion, and no leftovers are inserted. Reported-by: Ewan Young Author: Paul A. Jungwirth Discussion: https://postgr.es/m/CAON2xHO5FJ1jC87cQhGg%2BKMG5QR7ZaifJwN24rPjMqCGYn-hzA%40mail.gmail.com --- doc/src/sgml/dml.sgml | 11 ++ src/backend/rewrite/rewriteHandler.c | 177 ++++++++++-------- src/test/regress/expected/updatable_views.out | 118 ++++++++++++ src/test/regress/sql/updatable_views.sql | 78 ++++++++ 4 files changed, 301 insertions(+), 83 deletions(-) diff --git a/doc/src/sgml/dml.sgml b/doc/src/sgml/dml.sgml index 10ea4889e53..18d819121d8 100644 --- a/doc/src/sgml/dml.sgml +++ b/doc/src/sgml/dml.sgml @@ -396,6 +396,17 @@ DELETE FROM products policies still apply to the leftovers. + + If a temporal update or delete is replaced by an unconditional DO + INSTEAD rule (see ), the rule's + actions run only for rows that overlap the targeted portion. In those + actions, the application-time value in NEW is narrowed + based on the FOR PORTION OF target, while + OLD has the original value. The original command does + not run, so no temporal leftovers are inserted. This is the same whether + the rule is on a table or a view. + + In READ COMMITTED mode, a temporal update or delete can fail with a serialization failure when it touches a row that another diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index e3814ab2bee..1db1e2188fd 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -97,6 +97,7 @@ static List *matchLocks(CmdType event, Relation relation, static Query *fireRIRrules(Query *parsetree, List *activeRIRs); static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist); static List *get_generated_columns(Relation rel, int rt_index, bool include_stored); +static void addForPortionOfDetails(Query *parsetree, Relation rel); /* @@ -4020,6 +4021,64 @@ rewriteTargetView(Query *parsetree, Relation view) } +/* + * addForPortionOfDetails - + * add the FOR PORTION OF restriction to an UPDATE or DELETE + * + * The query is limited to rows that overlap the target range, and an UPDATE + * also sets the range column(s) to the targeted portion. rel is the target + * relation, named in the error for assigning the range column twice. + */ +static void +addForPortionOfDetails(Query *parsetree, Relation rel) +{ + ListCell *tl; + + Assert(parsetree->forPortionOf); + + /* + * Add qual: UPDATE/DELETE FOR PORTION OF should be limited to rows that + * overlap the target range. + */ + AddQual(parsetree, parsetree->forPortionOf->overlapsExpr); + + /* Update FOR PORTION OF column(s) automatically. */ + if (parsetree->commandType == CMD_UPDATE) + { + foreach(tl, parsetree->forPortionOf->rangeTargetList) + { + TargetEntry *tle = (TargetEntry *) lfirst(tl); + ListCell *tl2; + + /* + * Detect multiple-assignment early, so that we can report the + * same error as against a regular table. If we detect it later, + * the error message will use the column names of the base table, + * not the view. + */ + foreach(tl2, parsetree->targetList) + { + TargetEntry *prior = (TargetEntry *) lfirst(tl2); + + if (prior->resjunk || prior->resno != tle->resno) + continue; + + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("cannot update column \"%s\" because it is used in FOR PORTION OF", + prior->resname), + strcmp(prior->resname, tle->resname) != 0 ? + errdetail("Columns \"%s\" and \"%s\" are the same column of relation \"%s\".", + prior->resname, tle->resname, + RelationGetRelationName(rel)) : 0)); + } + + parsetree->targetList = lappend(parsetree->targetList, tle); + } + } +} + + /* * RewriteQuery - * rewrites the query and apply the rules again on the queries rewritten @@ -4150,6 +4209,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length, List *locks; int product_orig_rt_length; List *product_queries; + Query *rule_parsetree; bool hasUpdate = false; int values_rte_index = 0; bool defaults_remaining = false; @@ -4252,59 +4312,17 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length, { Assert(parsetree->override == OVERRIDING_NOT_SET); - if (parsetree->forPortionOf) - { - /* - * Don't add FOR PORTION OF details until we're done rewriting - * a view update, so that we don't add the same qual and TLE - * on the recursion. - * - * Views don't need to do anything special here to remap Vars; - * that is handled by the tree walker. - */ - if (rt_entry_relation->rd_rel->relkind != RELKIND_VIEW) - { - ListCell *tl; - - /* - * Add qual: UPDATE FOR PORTION OF should be limited to - * rows that overlap the target range. - */ - AddQual(parsetree, parsetree->forPortionOf->overlapsExpr); - - /* Update FOR PORTION OF column(s) automatically. */ - foreach(tl, parsetree->forPortionOf->rangeTargetList) - { - TargetEntry *tle = (TargetEntry *) lfirst(tl); - ListCell *tl2; - - /* - * Detect multiple-assignment early, so that we can - * report the same error as against a regular table. - * If we detect it later, the error message will use - * the column names of the base table, not the view. - */ - foreach(tl2, parsetree->targetList) - { - TargetEntry *prior = (TargetEntry *) lfirst(tl2); - - if (prior->resjunk || prior->resno != tle->resno) - continue; - - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("cannot update column \"%s\" because it is used in FOR PORTION OF", - prior->resname), - strcmp(prior->resname, tle->resname) != 0 ? - errdetail("Columns \"%s\" and \"%s\" are the same column of relation \"%s\".", - prior->resname, tle->resname, - RelationGetRelationName(rt_entry_relation)) : 0)); - } - - parsetree->targetList = lappend(parsetree->targetList, tle); - } - } - } + /* + * Don't add FOR PORTION OF details to a view's query, so that we + * don't add the same qual and TLE again when we recurse on the + * auto-updated query. Rule actions get them below. + * + * Views don't need to do anything special here to remap Vars; + * that is handled by the tree walker. + */ + if (parsetree->forPortionOf && + rt_entry_relation->rd_rel->relkind != RELKIND_VIEW) + addForPortionOfDetails(parsetree, rt_entry_relation); parsetree->targetList = rewriteTargetListIU(parsetree->targetList, @@ -4351,25 +4369,10 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length, } else if (event == CMD_DELETE) { - if (parsetree->forPortionOf) - { - /* - * Don't add FOR PORTION OF details until we're done rewriting - * a view delete, so that we don't add the same qual on the - * recursion. - * - * Views don't need to do anything special here to remap Vars; - * that is handled by the tree walker. - */ - if (rt_entry_relation->rd_rel->relkind != RELKIND_VIEW) - { - /* - * Add qual: DELETE FOR PORTION OF should be limited to - * rows that overlap the target range. - */ - AddQual(parsetree, parsetree->forPortionOf->overlapsExpr); - } - } + /* As for UPDATE, a view's query doesn't get these yet. */ + if (parsetree->forPortionOf && + rt_entry_relation->rd_rel->relkind != RELKIND_VIEW) + addForPortionOfDetails(parsetree, rt_entry_relation); } else elog(ERROR, "unrecognized commandType: %d", (int) event); @@ -4380,8 +4383,25 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length, locks = matchLocks(event, rt_entry_relation, result_relation, parsetree, &hasUpdate); + /* + * Rule actions on a view must see the FOR PORTION OF restriction, + * just as they do on a table: they should only fire for rows that + * overlap the target range, and an UPDATE rule's NEW should hold the + * targeted portion of the range. We didn't add those details to the + * view's query above, so build the rule actions from a copy that has + * them. If the view gets auto-updated below, the original query gets + * them when we recurse on the base relation. + */ + rule_parsetree = parsetree; + if (parsetree->forPortionOf && + rt_entry_relation->rd_rel->relkind == RELKIND_VIEW) + { + rule_parsetree = copyObject(parsetree); + addForPortionOfDetails(rule_parsetree, rt_entry_relation); + } + product_orig_rt_length = list_length(parsetree->rtable); - product_queries = fireRules(parsetree, + product_queries = fireRules(rule_parsetree, result_relation, event, locks, @@ -4473,16 +4493,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length, parsetree->mergeActionList, gettext_noop("Views with conditional DO INSTEAD rules are not automatically updatable.")); - /* - * We don't support FOR PORTION OF on views with DO ALSO rules - * either. product_queries holds their actions, and fireRules() - * built those from a query that does not carry the FOR PORTION OF - * restriction yet: we add the overlaps qual and the range - * assignment only after the view has been rewritten down to its - * base relation, so as not to add them twice. So a rule action - * would see an unnarrowed NEW range and would not be limited to - * the rows the statement targets. - */ + /* We don't support FOR PORTION OF on views with DO ALSO rules. */ if (parsetree->forPortionOf && product_queries != NIL) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out index 81818b680d6..77c68640e92 100644 --- a/src/test/regress/expected/updatable_views.out +++ b/src/test/regress/expected/updatable_views.out @@ -4266,6 +4266,124 @@ select * from base_tab order by a; drop view base_tab_view; drop table base_tab; +-- FOR PORTION OF on a view with DO INSTEAD rules. As on a table, the rule +-- actions fire only for rows that overlap the target range, NEW holds the +-- targeted portion of the range, and OLD holds the whole row. The original +-- statement doesn't run, so there are no temporal leftovers. +create table uv_fpo_rinst_tab (id int4range, valid_at daterange, b int) + partition by list (id); +create table uv_fpo_rinst_tab1 partition of uv_fpo_rinst_tab + for values in ('[1,1]'); +create table uv_fpo_rinst_tab2 partition of uv_fpo_rinst_tab + for values in ('[2,2]'); +insert into uv_fpo_rinst_tab values + ('[1,1]', '[2000-01-01,2010-01-01)', 0), + ('[2,2]', '[2020-01-01,2030-01-01)', 0); +create table uv_fpo_rinst_log (tag text, old_valid_at daterange, + new_valid_at daterange, b int); +create view uv_fpo_rinst_view as select id, valid_at, b from uv_fpo_rinst_tab; +create rule uv_fpo_rinst_upd as on update to uv_fpo_rinst_view + do instead insert into uv_fpo_rinst_log + values ('upd', old.valid_at, new.valid_at, new.b) + returning null::int4range, new_valid_at, b; +create rule uv_fpo_rinst_del as on delete to uv_fpo_rinst_view + do instead insert into uv_fpo_rinst_log + values ('del', old.valid_at, null, old.b) + returning null::int4range, old_valid_at, b; +update uv_fpo_rinst_view + for portion of valid_at from '2002-01-01' to '2004-01-01' set b = 1 + returning *; + id | valid_at | b +----+-------------------------+--- + | [01-01-2002,01-01-2004) | 1 +(1 row) + +delete from uv_fpo_rinst_view + for portion of valid_at from '2006-01-01' to '2008-01-01' + returning *; + id | valid_at | b +----+-------------------------+--- + | [01-01-2000,01-01-2010) | 0 +(1 row) + +update uv_fpo_rinst_view + for portion of valid_at from '2012-01-01' to '2014-01-01' set b = 2; +-- through an auto-updatable view with the columns in a different order +create view uv_fpo_rinst_outer as select b, valid_at, id from uv_fpo_rinst_view; +update uv_fpo_rinst_outer + for portion of valid_at from '2024-01-01' to '2026-01-01' set b = 3; +drop view uv_fpo_rinst_outer; +select * from uv_fpo_rinst_log order by tag, old_valid_at; + tag | old_valid_at | new_valid_at | b +-----+-------------------------+-------------------------+--- + del | [01-01-2000,01-01-2010) | | 0 + upd | [01-01-2000,01-01-2010) | [01-01-2002,01-01-2004) | 1 + upd | [01-01-2020,01-01-2030) | [01-01-2024,01-01-2026) | 3 +(3 rows) + +select * from uv_fpo_rinst_tab order by id, valid_at; + id | valid_at | b +-------+-------------------------+--- + [1,2) | [01-01-2000,01-01-2010) | 0 + [2,3) | [01-01-2020,01-01-2030) | 0 +(2 rows) + +truncate uv_fpo_rinst_log; +-- The rules also apply to statements parsed before the rules existed. +drop rule uv_fpo_rinst_upd on uv_fpo_rinst_view; +create function uv_fpo_rinst_fn() returns void language sql begin atomic + update uv_fpo_rinst_view + for portion of valid_at from '2002-01-01' to '2004-01-01' set b = 4; +end; +prepare uv_fpo_rinst_prep as update uv_fpo_rinst_view + for portion of valid_at from '2004-01-01' to '2006-01-01' set b = 5; +execute uv_fpo_rinst_prep; -- auto-updates the view, making leftovers +create rule uv_fpo_rinst_upd as on update to uv_fpo_rinst_view + do instead insert into uv_fpo_rinst_log + values ('upd', old.valid_at, new.valid_at, new.b); +select uv_fpo_rinst_fn(); + uv_fpo_rinst_fn +----------------- + +(1 row) + +execute uv_fpo_rinst_prep; +select * from uv_fpo_rinst_log order by tag, old_valid_at; + tag | old_valid_at | new_valid_at | b +-----+-------------------------+-------------------------+--- + upd | [01-01-2000,01-01-2004) | [01-01-2002,01-01-2004) | 4 + upd | [01-01-2004,01-01-2006) | [01-01-2004,01-01-2006) | 5 +(2 rows) + +deallocate uv_fpo_rinst_prep; +drop function uv_fpo_rinst_fn(); +truncate uv_fpo_rinst_log; +-- A conditional DO INSTEAD rule needs an unconditional one too. Its action +-- also sees the FOR PORTION OF restriction. +drop rule uv_fpo_rinst_upd on uv_fpo_rinst_view; +create rule uv_fpo_rinst_cond as on update to uv_fpo_rinst_view + where old.id = '[2,2]' + do instead insert into uv_fpo_rinst_log + values ('cond', old.valid_at, new.valid_at, new.b); +update uv_fpo_rinst_view + for portion of valid_at from '2026-01-01' to '2028-01-01' set b = 6; -- error +ERROR: cannot update view "uv_fpo_rinst_view" +DETAIL: Views with conditional DO INSTEAD rules are not automatically updatable. +HINT: To enable updating the view, provide an INSTEAD OF UPDATE trigger or an unconditional ON UPDATE DO INSTEAD rule. +create rule uv_fpo_rinst_upd as on update to uv_fpo_rinst_view + do instead insert into uv_fpo_rinst_log + values ('upd', old.valid_at, new.valid_at, new.b); +update uv_fpo_rinst_view + for portion of valid_at from '2026-01-01' to '2028-01-01' set b = 6; +select * from uv_fpo_rinst_log order by tag, old_valid_at; + tag | old_valid_at | new_valid_at | b +------+-------------------------+-------------------------+--- + cond | [01-01-2020,01-01-2030) | [01-01-2026,01-01-2028) | 6 + upd | [01-01-2020,01-01-2030) | [01-01-2026,01-01-2028) | 6 +(2 rows) + +drop view uv_fpo_rinst_view; +drop table uv_fpo_rinst_tab, uv_fpo_rinst_log; -- FOR PORTION OF is not supported on views with INSTEAD OF triggers create view uv_fpo_instead_view as select id, valid_at, b from uv_fpo_tab; create function uv_fpo_instead_trig() returns trigger language plpgsql as diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql index a0603557257..afccdcd8143 100644 --- a/src/test/regress/sql/updatable_views.sql +++ b/src/test/regress/sql/updatable_views.sql @@ -2211,6 +2211,84 @@ select * from base_tab order by a; drop view base_tab_view; drop table base_tab; +-- FOR PORTION OF on a view with DO INSTEAD rules. As on a table, the rule +-- actions fire only for rows that overlap the target range, NEW holds the +-- targeted portion of the range, and OLD holds the whole row. The original +-- statement doesn't run, so there are no temporal leftovers. +create table uv_fpo_rinst_tab (id int4range, valid_at daterange, b int) + partition by list (id); +create table uv_fpo_rinst_tab1 partition of uv_fpo_rinst_tab + for values in ('[1,1]'); +create table uv_fpo_rinst_tab2 partition of uv_fpo_rinst_tab + for values in ('[2,2]'); +insert into uv_fpo_rinst_tab values + ('[1,1]', '[2000-01-01,2010-01-01)', 0), + ('[2,2]', '[2020-01-01,2030-01-01)', 0); +create table uv_fpo_rinst_log (tag text, old_valid_at daterange, + new_valid_at daterange, b int); +create view uv_fpo_rinst_view as select id, valid_at, b from uv_fpo_rinst_tab; +create rule uv_fpo_rinst_upd as on update to uv_fpo_rinst_view + do instead insert into uv_fpo_rinst_log + values ('upd', old.valid_at, new.valid_at, new.b) + returning null::int4range, new_valid_at, b; +create rule uv_fpo_rinst_del as on delete to uv_fpo_rinst_view + do instead insert into uv_fpo_rinst_log + values ('del', old.valid_at, null, old.b) + returning null::int4range, old_valid_at, b; +update uv_fpo_rinst_view + for portion of valid_at from '2002-01-01' to '2004-01-01' set b = 1 + returning *; +delete from uv_fpo_rinst_view + for portion of valid_at from '2006-01-01' to '2008-01-01' + returning *; +update uv_fpo_rinst_view + for portion of valid_at from '2012-01-01' to '2014-01-01' set b = 2; +-- through an auto-updatable view with the columns in a different order +create view uv_fpo_rinst_outer as select b, valid_at, id from uv_fpo_rinst_view; +update uv_fpo_rinst_outer + for portion of valid_at from '2024-01-01' to '2026-01-01' set b = 3; +drop view uv_fpo_rinst_outer; +select * from uv_fpo_rinst_log order by tag, old_valid_at; +select * from uv_fpo_rinst_tab order by id, valid_at; +truncate uv_fpo_rinst_log; + +-- The rules also apply to statements parsed before the rules existed. +drop rule uv_fpo_rinst_upd on uv_fpo_rinst_view; +create function uv_fpo_rinst_fn() returns void language sql begin atomic + update uv_fpo_rinst_view + for portion of valid_at from '2002-01-01' to '2004-01-01' set b = 4; +end; +prepare uv_fpo_rinst_prep as update uv_fpo_rinst_view + for portion of valid_at from '2004-01-01' to '2006-01-01' set b = 5; +execute uv_fpo_rinst_prep; -- auto-updates the view, making leftovers +create rule uv_fpo_rinst_upd as on update to uv_fpo_rinst_view + do instead insert into uv_fpo_rinst_log + values ('upd', old.valid_at, new.valid_at, new.b); +select uv_fpo_rinst_fn(); +execute uv_fpo_rinst_prep; +select * from uv_fpo_rinst_log order by tag, old_valid_at; +deallocate uv_fpo_rinst_prep; +drop function uv_fpo_rinst_fn(); +truncate uv_fpo_rinst_log; + +-- A conditional DO INSTEAD rule needs an unconditional one too. Its action +-- also sees the FOR PORTION OF restriction. +drop rule uv_fpo_rinst_upd on uv_fpo_rinst_view; +create rule uv_fpo_rinst_cond as on update to uv_fpo_rinst_view + where old.id = '[2,2]' + do instead insert into uv_fpo_rinst_log + values ('cond', old.valid_at, new.valid_at, new.b); +update uv_fpo_rinst_view + for portion of valid_at from '2026-01-01' to '2028-01-01' set b = 6; -- error +create rule uv_fpo_rinst_upd as on update to uv_fpo_rinst_view + do instead insert into uv_fpo_rinst_log + values ('upd', old.valid_at, new.valid_at, new.b); +update uv_fpo_rinst_view + for portion of valid_at from '2026-01-01' to '2028-01-01' set b = 6; +select * from uv_fpo_rinst_log order by tag, old_valid_at; +drop view uv_fpo_rinst_view; +drop table uv_fpo_rinst_tab, uv_fpo_rinst_log; + -- FOR PORTION OF is not supported on views with INSTEAD OF triggers create view uv_fpo_instead_view as select id, valid_at, b from uv_fpo_tab; -- 2.47.3