On Wed, 23 Nov 2022 at 18:56, Tom Lane <t...@sss.pgh.pa.us> wrote: > > I wonder if somehow we could just make one pass over > all the VALUES RTEs, and process each one as needed? The problem > is to identify the relevant target relation, I guess. >
I have been thinking about that some more, but I think it would be pretty difficult to achieve. Part of the problem is that the targetlist processing and VALUES RTE processing are quite closely coupled (because of things like GENERATED ALWAYS columns). Both rewriteTargetListIU() and rewriteValuesRTE() rely on being passed the VALUES RTE that the targetlist is reading from, and rewriteValuesRTE() then relies on extra information returned by rewriteTargetListIU(). Also, there's the way that DEFAULTs from updatable views work, which means that the DEFAULTs in a VALUES RTE won't necessarily all come from the same target relation. So I think it would be much harder to do the VALUES RTE processing anywhere other than where it's being done right now, and even if it could be done elsewhere, it would be a very invasive change, and therefore hard to back-patch. That, of course, leaves the problem of identifying the right VALUES RTE to process. A different way to do this, without relying on the contents of the targetlist, is to note that, while processing a product query, what we really want to do is ignore any VALUES RTEs from the original query, since they will have already been processed. There should then never be more than one VALUES RTE left to process -- the one from the rule action. This can be done by exploiting the fact that in product queries, the rtable always consists of the rtable from the original query followed by the rtable from the rule action, so we just need to ignore the right number of RTEs at the start of the rtable. Of course that would break if we ever changed the way rewriteRuleAction() worked, but at least it only depends on that one other place in the code, which has been stable for a long time, so the risk of future breakage seems managable. Regards, Dean
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c new file mode 100644 index fb0c687..1e3efbb --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -418,6 +418,10 @@ rewriteRuleAction(Query *parsetree, * NOTE: because planner will destructively alter rtable, we must ensure * that rule action's rtable is separate and shares no substructure with * the main rtable. Hence do a deep copy here. + * + * Note also that RewriteQuery() relies on the fact that RT entries from + * the original query appear at the start of the expanded rtable, so + * beware of changing this. */ sub_action->rtable = list_concat(copyObject(parsetree->rtable), sub_action->rtable); @@ -3622,9 +3626,13 @@ rewriteTargetView(Query *parsetree, Rela * * rewrite_events is a list of open query-rewrite actions, so we can detect * infinite recursion. + * + * orig_rt_length is the length of the originating query's rtable, for product + * queries created by fireRules(), and 0 otherwise. This is used to skip any + * already-processed VALUES RTEs from the original query. */ static List * -RewriteQuery(Query *parsetree, List *rewrite_events) +RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length) { CmdType event = parsetree->commandType; bool instead = false; @@ -3648,7 +3656,7 @@ RewriteQuery(Query *parsetree, List *rew if (ctequery->commandType == CMD_SELECT) continue; - newstuff = RewriteQuery(ctequery, rewrite_events); + newstuff = RewriteQuery(ctequery, rewrite_events, 0); /* * Currently we can only handle unconditional, single-statement DO @@ -3722,6 +3730,7 @@ RewriteQuery(Query *parsetree, List *rew RangeTblEntry *rt_entry; Relation rt_entry_relation; List *locks; + int product_orig_rt_length; List *product_queries; bool hasUpdate = false; int values_rte_index = 0; @@ -3743,23 +3752,30 @@ RewriteQuery(Query *parsetree, List *rew */ if (event == CMD_INSERT) { + ListCell *lc2; RangeTblEntry *values_rte = NULL; /* - * If it's an INSERT ... VALUES (...), (...), ... there will be a - * single RTE for the VALUES targetlists. + * Test if it's a multi-row INSERT ... VALUES (...), (...), ... by + * looking for a VALUES RTE in the fromlist. For product queries, + * we must ignore any already-processed VALUES RTEs from the + * original query. These appear at the start of the rangetable. */ - if (list_length(parsetree->jointree->fromlist) == 1) + foreach(lc2, parsetree->jointree->fromlist) { - RangeTblRef *rtr = (RangeTblRef *) linitial(parsetree->jointree->fromlist); + RangeTblRef *rtr = (RangeTblRef *) lfirst(lc2); - if (IsA(rtr, RangeTblRef)) + if (IsA(rtr, RangeTblRef) && rtr->rtindex > orig_rt_length) { RangeTblEntry *rte = rt_fetch(rtr->rtindex, parsetree->rtable); if (rte->rtekind == RTE_VALUES) { + /* should not find more than one VALUES RTE */ + if (values_rte != NULL) + elog(ERROR, "more than one VALUES RTE found"); + values_rte = rte; values_rte_index = rtr->rtindex; } @@ -3837,7 +3853,11 @@ RewriteQuery(Query *parsetree, List *rew break; case CMD_UPDATE: case CMD_INSERT: - /* XXX is it possible to have a VALUES clause? */ + + /* + * MERGE actions do not permit multi-row INSERTs, so + * there is no VALUES RTE to deal with here. + */ action->targetList = rewriteTargetListIU(action->targetList, action->commandType, @@ -3864,6 +3884,7 @@ RewriteQuery(Query *parsetree, List *rew locks = matchLocks(event, rt_entry_relation->rd_rules, result_relation, parsetree, &hasUpdate); + product_orig_rt_length = list_length(parsetree->rtable); product_queries = fireRules(parsetree, result_relation, event, @@ -4020,7 +4041,19 @@ RewriteQuery(Query *parsetree, List *rew Query *pt = (Query *) lfirst(n); List *newstuff; - newstuff = RewriteQuery(pt, rewrite_events); + /* + * For an updatable view, pt might be the rewritten version of + * the original query, in which case we pass on orig_rt_length + * to finish processing any VALUES RTE it contained. + * + * Otherwise, we have a product query created by fireRules(). + * Any VALUES RTEs from the original query have been fully + * processed, and must be skipped when we recurse. + */ + newstuff = RewriteQuery(pt, rewrite_events, + pt == parsetree ? + orig_rt_length : + product_orig_rt_length); rewritten = list_concat(rewritten, newstuff); } @@ -4172,7 +4205,7 @@ QueryRewrite(Query *parsetree) * * Apply all non-SELECT rules possibly getting 0 or many queries */ - querylist = RewriteQuery(parsetree, NIL); + querylist = RewriteQuery(parsetree, NIL, 0); /* * Step 2 diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out new file mode 100644 index 624d0e5..9c28dd9 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -2938,11 +2938,11 @@ select pg_get_viewdef('shoe'::regclass,0 -- -- check multi-row VALUES in rules -- -create table rules_src(f1 int, f2 int); -create table rules_log(f1 int, f2 int, tag text); +create table rules_src(f1 int, f2 int default 0); +create table rules_log(f1 int, f2 int, tag text, id serial); insert into rules_src values(1,2), (11,12); create rule r1 as on update to rules_src do also - insert into rules_log values(old.*, 'old'), (new.*, 'new'); + insert into rules_log values(old.*, 'old', default), (new.*, 'new', default); update rules_src set f2 = f2 + 1; update rules_src set f2 = f2 * 10; select * from rules_src; @@ -2953,16 +2953,16 @@ select * from rules_src; (2 rows) select * from rules_log; - f1 | f2 | tag -----+-----+----- - 1 | 2 | old - 1 | 3 | new - 11 | 12 | old - 11 | 13 | new - 1 | 3 | old - 1 | 30 | new - 11 | 13 | old - 11 | 130 | new + f1 | f2 | tag | id +----+-----+-----+---- + 1 | 2 | old | 1 + 1 | 3 | new | 2 + 11 | 12 | old | 3 + 11 | 13 | new | 4 + 1 | 3 | old | 5 + 1 | 30 | new | 6 + 11 | 13 | old | 7 + 11 | 130 | new | 8 (8 rows) create rule r2 as on update to rules_src do also @@ -2976,71 +2976,84 @@ update rules_src set f2 = f2 / 10; 11 | 13 | new (4 rows) +create rule r3 as on insert to rules_src do also + insert into rules_log values(null, null, '-', default), (new.*, 'new', default); +insert into rules_src values(22,23), (33,default); select * from rules_src; f1 | f2 ----+---- 1 | 3 11 | 13 -(2 rows) + 22 | 23 + 33 | 0 +(4 rows) select * from rules_log; - f1 | f2 | tag -----+-----+----- - 1 | 2 | old - 1 | 3 | new - 11 | 12 | old - 11 | 13 | new - 1 | 3 | old - 1 | 30 | new - 11 | 13 | old - 11 | 130 | new - 1 | 30 | old - 1 | 3 | new - 11 | 130 | old - 11 | 13 | new -(12 rows) + f1 | f2 | tag | id +----+-----+-----+---- + 1 | 2 | old | 1 + 1 | 3 | new | 2 + 11 | 12 | old | 3 + 11 | 13 | new | 4 + 1 | 3 | old | 5 + 1 | 30 | new | 6 + 11 | 13 | old | 7 + 11 | 130 | new | 8 + 1 | 30 | old | 9 + 1 | 3 | new | 10 + 11 | 130 | old | 11 + 11 | 13 | new | 12 + | | - | 13 + 22 | 23 | new | 14 + | | - | 15 + 33 | 0 | new | 16 +(16 rows) -create rule r3 as on delete to rules_src do notify rules_src_deletion; +create rule r4 as on delete to rules_src do notify rules_src_deletion; \d+ rules_src Table "public.rules_src" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description --------+---------+-----------+----------+---------+---------+--------------+------------- f1 | integer | | | | plain | | - f2 | integer | | | | plain | | + f2 | integer | | | 0 | plain | | Rules: r1 AS - ON UPDATE TO rules_src DO INSERT INTO rules_log (f1, f2, tag) VALUES (old.f1,old.f2,'old'::text), (new.f1,new.f2,'new'::text) + ON UPDATE TO rules_src DO INSERT INTO rules_log (f1, f2, tag, id) VALUES (old.f1,old.f2,'old'::text,DEFAULT), (new.f1,new.f2,'new'::text,DEFAULT) r2 AS ON UPDATE TO rules_src DO VALUES (old.f1,old.f2,'old'::text), (new.f1,new.f2,'new'::text) r3 AS + ON INSERT TO rules_src DO INSERT INTO rules_log (f1, f2, tag, id) VALUES (NULL::integer,NULL::integer,'-'::text,DEFAULT), (new.f1,new.f2,'new'::text,DEFAULT) + r4 AS ON DELETE TO rules_src DO NOTIFY rules_src_deletion -- -- Ensure an aliased target relation for insert is correctly deparsed. -- -create rule r4 as on insert to rules_src do instead insert into rules_log AS trgt SELECT NEW.* RETURNING trgt.f1, trgt.f2; -create rule r5 as on update to rules_src do instead UPDATE rules_log AS trgt SET tag = 'updated' WHERE trgt.f1 = new.f1; +create rule r5 as on insert to rules_src do instead insert into rules_log AS trgt SELECT NEW.* RETURNING trgt.f1, trgt.f2; +create rule r6 as on update to rules_src do instead UPDATE rules_log AS trgt SET tag = 'updated' WHERE trgt.f1 = new.f1; \d+ rules_src Table "public.rules_src" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description --------+---------+-----------+----------+---------+---------+--------------+------------- f1 | integer | | | | plain | | - f2 | integer | | | | plain | | + f2 | integer | | | 0 | plain | | Rules: r1 AS - ON UPDATE TO rules_src DO INSERT INTO rules_log (f1, f2, tag) VALUES (old.f1,old.f2,'old'::text), (new.f1,new.f2,'new'::text) + ON UPDATE TO rules_src DO INSERT INTO rules_log (f1, f2, tag, id) VALUES (old.f1,old.f2,'old'::text,DEFAULT), (new.f1,new.f2,'new'::text,DEFAULT) r2 AS ON UPDATE TO rules_src DO VALUES (old.f1,old.f2,'old'::text), (new.f1,new.f2,'new'::text) r3 AS + ON INSERT TO rules_src DO INSERT INTO rules_log (f1, f2, tag, id) VALUES (NULL::integer,NULL::integer,'-'::text,DEFAULT), (new.f1,new.f2,'new'::text,DEFAULT) + r4 AS ON DELETE TO rules_src DO NOTIFY rules_src_deletion - r4 AS + r5 AS ON INSERT TO rules_src DO INSTEAD INSERT INTO rules_log AS trgt (f1, f2) SELECT new.f1, new.f2 RETURNING trgt.f1, trgt.f2 - r5 AS + r6 AS ON UPDATE TO rules_src DO INSTEAD UPDATE rules_log trgt SET tag = 'updated'::text WHERE trgt.f1 = new.f1 diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql new file mode 100644 index bfb5f3b..ada535a --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -1016,11 +1016,11 @@ select pg_get_viewdef('shoe'::regclass,0 -- check multi-row VALUES in rules -- -create table rules_src(f1 int, f2 int); -create table rules_log(f1 int, f2 int, tag text); +create table rules_src(f1 int, f2 int default 0); +create table rules_log(f1 int, f2 int, tag text, id serial); insert into rules_src values(1,2), (11,12); create rule r1 as on update to rules_src do also - insert into rules_log values(old.*, 'old'), (new.*, 'new'); + insert into rules_log values(old.*, 'old', default), (new.*, 'new', default); update rules_src set f2 = f2 + 1; update rules_src set f2 = f2 * 10; select * from rules_src; @@ -1028,16 +1028,19 @@ select * from rules_log; create rule r2 as on update to rules_src do also values(old.*, 'old'), (new.*, 'new'); update rules_src set f2 = f2 / 10; +create rule r3 as on insert to rules_src do also + insert into rules_log values(null, null, '-', default), (new.*, 'new', default); +insert into rules_src values(22,23), (33,default); select * from rules_src; select * from rules_log; -create rule r3 as on delete to rules_src do notify rules_src_deletion; +create rule r4 as on delete to rules_src do notify rules_src_deletion; \d+ rules_src -- -- Ensure an aliased target relation for insert is correctly deparsed. -- -create rule r4 as on insert to rules_src do instead insert into rules_log AS trgt SELECT NEW.* RETURNING trgt.f1, trgt.f2; -create rule r5 as on update to rules_src do instead UPDATE rules_log AS trgt SET tag = 'updated' WHERE trgt.f1 = new.f1; +create rule r5 as on insert to rules_src do instead insert into rules_log AS trgt SELECT NEW.* RETURNING trgt.f1, trgt.f2; +create rule r6 as on update to rules_src do instead UPDATE rules_log AS trgt SET tag = 'updated' WHERE trgt.f1 = new.f1; \d+ rules_src --