On 2015-09-24 17:25:21 +0200, Andres Freund wrote: > Stuff I want to fix by tomorrow: > * Whole row var references to exclude > * wrong offsets for columns after dropped ones > * INSTEAD DO UPDATE for tables with oids > > Do you know of anything else?
So, took a bit longer than "tomorrow. I fought for a long while with a mysterious issue, which turned out to be separate bug: The excluded relation was affected by row level security policies, which doesn't make sense. My proposal in this WIP patch is to make it a bit clearer that 'EXCLUDED' isn't a real relation. I played around with adding a different rtekind, but that's too heavy a hammer. What I instead did was to set relkind to composite - which seems to signal pretty well that we're not dealing with a real relation. That immediately fixes the RLS issue as fireRIRrules has the following check: if (rte->rtekind != RTE_RELATION || rte->relkind != RELKIND_RELATION) continue; It also makes it relatively straightforward to fix the system column issue by adding an additional relkind check to scanRTEForColumn's system column handling. WRT to the wholerow issue: There's currently two reasons we need a targetlist entry for excluded wholerow vars: 1) setrefs.c errors out without - that can relativley easily be worked around 2) ruleutils.c expects an entry in the child tlist. That could also be worked around, but it's a bit more verbose. I'm inclined to not go the pullup route but instead simply unconditionally add a wholerow var to the excluded tlist. Peter, what do you think? Andres
>From f11fc12993beabf4d280b979c062682b6612c989 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Tue, 29 Sep 2015 17:08:36 +0200 Subject: [PATCH] wip-upsert --- src/backend/parser/analyze.c | 76 +++++++++++++++---- src/backend/parser/parse_relation.c | 7 +- src/test/regress/expected/insert_conflict.out | 101 ++++++++++++++++++++++++++ src/test/regress/expected/rules.out | 18 ++--- src/test/regress/sql/insert_conflict.sql | 56 ++++++++++++++ src/test/regress/sql/rules.sql | 2 +- 6 files changed, 234 insertions(+), 26 deletions(-) diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index a0dfbf9..528825a 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -891,33 +891,81 @@ transformOnConflictClause(ParseState *pstate, /* Process DO UPDATE */ if (onConflictClause->action == ONCONFLICT_UPDATE) { + Relation targetrel = pstate->p_target_relation; + Var *var; + TargetEntry *te; + int attno; + /* * All INSERT expressions have been parsed, get ready for potentially * existing SET statements that need to be processed like an UPDATE. */ pstate->p_is_insert = false; + /* + * Add range table entry for the EXCLUDED pseudo relation; relkind is + * set to composite to signal that we're not dealing with an actual + * relation. + */ exclRte = addRangeTableEntryForRelation(pstate, - pstate->p_target_relation, + targetrel, makeAlias("excluded", NIL), false, false); + exclRte->relkind = RELKIND_COMPOSITE_TYPE; exclRelIndex = list_length(pstate->p_rtable); /* - * Build a targetlist for the EXCLUDED pseudo relation. Out of - * simplicity we do that here, because expandRelAttrs() happens to - * nearly do the right thing; specifically it also works with views. - * It'd be more proper to instead scan some pseudo scan node, but it - * doesn't seem worth the amount of code required. - * - * The only caveat of this hack is that the permissions expandRelAttrs - * adds have to be reset. markVarForSelectPriv() will add the exact - * required permissions back. + * Build a targetlist for the EXCLUDED pseudo relation. Have to be + * careful to use resnos that correspond to attnos of the underlying + * relation. + */ + Assert(pstate->p_next_resno == 1); + for (attno = 0; attno < targetrel->rd_rel->relnatts; attno++) + { + Form_pg_attribute attr = targetrel->rd_att->attrs[attno]; + char *name; + + if (attr->attisdropped) + { + /* + * can't use atttypid here, but it doesn't really matter + * what type the Const claims to be. + */ + var = (Var *) makeNullConst(INT4OID, -1, InvalidOid); + name = ""; + } + else + { + var = makeVar(exclRelIndex, attno + 1, + attr->atttypid, attr->atttypmod, + attr->attcollation, + 0); + var->location = -1; + + name = NameStr(attr->attname); + } + + Assert(pstate->p_next_resno == attno + 1); + te = makeTargetEntry((Expr *) var, + pstate->p_next_resno++, + name, + false); + + /* don't require select access yet */ + exclRelTlist = lappend(exclRelTlist, te); + } + + /* + * Additionally add a whole row tlist entry for EXCLUDED. That's + * really only needed for ruleutils' benefit, which expects to find + * corresponding entries in child tlists. Alternatively we could do + * this only when required, but that doesn't seem worth the trouble. */ - exclRelTlist = expandRelAttrs(pstate, exclRte, - exclRelIndex, 0, -1); - exclRte->requiredPerms = 0; - exclRte->selectedCols = NULL; + var = makeVar(exclRelIndex, InvalidAttrNumber, + RelationGetRelid(targetrel), + -1, InvalidOid, 0); + te = makeTargetEntry((Expr *) var, 0, NULL, true); + exclRelTlist = lappend(exclRelTlist, te); /* * Add EXCLUDED and the target RTE to the namespace, so that they can diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c index 0b2dacf..270a78c 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -686,9 +686,12 @@ scanRTEForColumn(ParseState *pstate, RangeTblEntry *rte, char *colname, return result; /* - * If the RTE represents a real table, consider system column names. + * If the RTE represents a real relation, consider system column + * names. Composites are only used for pseudo-relations like ON CONFLICT's + * excluded. */ - if (rte->rtekind == RTE_RELATION) + if (rte->rtekind == RTE_RELATION && + rte->relkind != RELKIND_COMPOSITE_TYPE) { /* quick check to see if name could be a system column */ attnum = specialAttNum(colname); diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out index 1399f3c..111029d 100644 --- a/src/test/regress/expected/insert_conflict.out +++ b/src/test/regress/expected/insert_conflict.out @@ -380,6 +380,58 @@ ERROR: there is no unique or exclusion constraint matching the ON CONFLICT spec insert into insertconflicttest values (23, 'Blackberry') on conflict (fruit) where fruit like '%berry' do update set fruit = excluded.fruit; ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification drop index partial_key_index; +-- +-- Test resjunk in excluded.* pseudo-relation +-- +create unique index plain on insertconflicttest(key); +-- Succeeds, updates existing row: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* != excluded.* returning *; + key | fruit +-----+----------- + 23 | Jackfruit +(1 row) + +-- No update this time, though: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* != excluded.* returning *; + key | fruit +-----+------- +(0 rows) + +-- Predicate changed to require match rather than non-match, so updates once more: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* = excluded.* returning *; + key | fruit +-----+----------- + 23 | Jackfruit +(1 row) + +-- Assign: +insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.*::text + returning *; + key | fruit +-----+-------------- + 23 | (23,Avocado) +(1 row) + +-- deparse whole row var in WHERE clause: +explain (costs off) insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.fruit where excluded.* is null; + QUERY PLAN +----------------------------------------- + Insert on insertconflicttest i + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: plain + Conflict Filter: (excluded.* IS NULL) + -> Result +(5 rows) + +-- Forbidden, fails: +insert into insertconflicttest values (23, 'Blackberry') on conflict (key) do update set fruit = excluded.ctid; +ERROR: column excluded.ctid does not exist +LINE 1: ...ckberry') on conflict (key) do update set fruit = excluded.c... + ^ +drop index plain; -- Cleanup drop table insertconflicttest; -- ****************************************************************** @@ -566,3 +618,52 @@ insert into testoids values(3, '2') on conflict (key) do update set data = exclu (1 row) DROP TABLE testoids; +-- check that references to columns after dropped columns are handled correctly +create table dropcol(key int primary key, drop1 int, keep1 text, drop2 numeric, keep2 float); +insert into dropcol(key, drop1, keep1, drop2, keep2) values(1, 1, '1', '1', 1); +-- set using excluded +insert into dropcol(key, drop1, keep1, drop2, keep2) values(1, 2, '2', '2', 2) on conflict(key) + do update set drop1 = excluded.drop1, keep1 = excluded.keep1, drop2 = excluded.drop2, keep2 = excluded.keep2 + where excluded.drop1 is not null and excluded.keep1 is not null and excluded.drop2 is not null and excluded.keep2 is not null + and dropcol.drop1 is not null and dropcol.keep1 is not null and dropcol.drop2 is not null and dropcol.keep2 is not null + returning *; + key | drop1 | keep1 | drop2 | keep2 +-----+-------+-------+-------+------- + 1 | 2 | 2 | 2 | 2 +(1 row) + +; +-- set using existing table +insert into dropcol(key, drop1, keep1, drop2, keep2) values(1, 3, '3', '3', 3) on conflict(key) + do update set drop1 = dropcol.drop1, keep1 = dropcol.keep1, drop2 = dropcol.drop2, keep2 = dropcol.keep2 + returning *; + key | drop1 | keep1 | drop2 | keep2 +-----+-------+-------+-------+------- + 1 | 2 | 2 | 2 | 2 +(1 row) + +; +alter table dropcol drop column drop1, drop column drop2; +-- set using excluded +insert into dropcol(key, keep1, keep2) values(1, '4', 4) on conflict(key) + do update set keep1 = excluded.keep1, keep2 = excluded.keep2 + where excluded.keep1 is not null and excluded.keep2 is not null + and dropcol.keep1 is not null and dropcol.keep2 is not null + returning *; + key | keep1 | keep2 +-----+-------+------- + 1 | 4 | 4 +(1 row) + +; +-- set using existing table +insert into dropcol(key, keep1, keep2) values(1, '5', 5) on conflict(key) + do update set keep1 = dropcol.keep1, keep2 = dropcol.keep2 + returning *; + key | keep1 | keep2 +-----+-------+------- + 1 | 4 | 4 +(1 row) + +; +DROP TABLE dropcol; diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 44c6740..80374e4 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -2900,7 +2900,7 @@ CREATE RULE hat_upsert AS ON INSERT TO hats ON CONFLICT (hat_name) DO UPDATE SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color - WHERE excluded.hat_color <> 'forbidden' + WHERE excluded.hat_color <> 'forbidden' AND hat_data.* != excluded.* RETURNING *; SELECT definition FROM pg_rules WHERE tablename = 'hats' ORDER BY rulename; definition @@ -2908,7 +2908,7 @@ SELECT definition FROM pg_rules WHERE tablename = 'hats' ORDER BY rulename; CREATE RULE hat_upsert AS + ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) + VALUES (new.hat_name, new.hat_color) ON CONFLICT(hat_name) DO UPDATE SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color+ - WHERE (excluded.hat_color <> 'forbidden'::bpchar) + + WHERE ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*)) + RETURNING hat_data.hat_name, + hat_data.hat_color; (1 row) @@ -2956,19 +2956,19 @@ SELECT tablename, rulename, definition FROM pg_rules hats | hat_upsert | CREATE RULE hat_upsert AS + | | ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) + | | VALUES (new.hat_name, new.hat_color) ON CONFLICT(hat_name) DO UPDATE SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color+ - | | WHERE (excluded.hat_color <> 'forbidden'::bpchar) + + | | WHERE ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*)) + | | RETURNING hat_data.hat_name, + | | hat_data.hat_color; (1 row) -- ensure explain works for on insert conflict rules explain (costs off) INSERT INTO hats VALUES ('h8', 'forbidden') RETURNING *; - QUERY PLAN ----------------------------------------------------------------- + QUERY PLAN +------------------------------------------------------------------------------------------------- Insert on hat_data Conflict Resolution: UPDATE Conflict Arbiter Indexes: hat_data_unique_idx - Conflict Filter: (excluded.hat_color <> 'forbidden'::bpchar) + Conflict Filter: ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*)) -> Result (5 rows) @@ -2995,12 +2995,12 @@ EXPLAIN (costs off) WITH data(hat_name, hat_color) AS ( INSERT INTO hats SELECT * FROM data RETURNING *; - QUERY PLAN ----------------------------------------------------------------- + QUERY PLAN +------------------------------------------------------------------------------------------------- Insert on hat_data Conflict Resolution: UPDATE Conflict Arbiter Indexes: hat_data_unique_idx - Conflict Filter: (excluded.hat_color <> 'forbidden'::bpchar) + Conflict Filter: ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*)) CTE data -> Values Scan on "*VALUES*" -> CTE Scan on data diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql index efa902e..5653715 100644 --- a/src/test/regress/sql/insert_conflict.sql +++ b/src/test/regress/sql/insert_conflict.sql @@ -223,6 +223,30 @@ insert into insertconflicttest values (23, 'Blackberry') on conflict (fruit) whe drop index partial_key_index; +-- +-- Test resjunk in excluded.* pseudo-relation +-- +create unique index plain on insertconflicttest(key); + +-- Succeeds, updates existing row: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* != excluded.* returning *; +-- No update this time, though: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* != excluded.* returning *; +-- Predicate changed to require match rather than non-match, so updates once more: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* = excluded.* returning *; +-- Assign: +insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.*::text + returning *; +-- deparse whole row var in WHERE clause: +explain (costs off) insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.fruit where excluded.* is null; +-- Forbidden, fails: +insert into insertconflicttest values (23, 'Blackberry') on conflict (key) do update set fruit = excluded.ctid; + +drop index plain; + -- Cleanup drop table insertconflicttest; @@ -317,3 +341,35 @@ insert into testoids values(3, '1') on conflict (key) do update set data = exclu insert into testoids values(3, '2') on conflict (key) do update set data = excluded.data RETURNING *; DROP TABLE testoids; + + +-- check that references to columns after dropped columns are handled correctly +create table dropcol(key int primary key, drop1 int, keep1 text, drop2 numeric, keep2 float); +insert into dropcol(key, drop1, keep1, drop2, keep2) values(1, 1, '1', '1', 1); +-- set using excluded +insert into dropcol(key, drop1, keep1, drop2, keep2) values(1, 2, '2', '2', 2) on conflict(key) + do update set drop1 = excluded.drop1, keep1 = excluded.keep1, drop2 = excluded.drop2, keep2 = excluded.keep2 + where excluded.drop1 is not null and excluded.keep1 is not null and excluded.drop2 is not null and excluded.keep2 is not null + and dropcol.drop1 is not null and dropcol.keep1 is not null and dropcol.drop2 is not null and dropcol.keep2 is not null + returning *; +; +-- set using existing table +insert into dropcol(key, drop1, keep1, drop2, keep2) values(1, 3, '3', '3', 3) on conflict(key) + do update set drop1 = dropcol.drop1, keep1 = dropcol.keep1, drop2 = dropcol.drop2, keep2 = dropcol.keep2 + returning *; +; +alter table dropcol drop column drop1, drop column drop2; +-- set using excluded +insert into dropcol(key, keep1, keep2) values(1, '4', 4) on conflict(key) + do update set keep1 = excluded.keep1, keep2 = excluded.keep2 + where excluded.keep1 is not null and excluded.keep2 is not null + and dropcol.keep1 is not null and dropcol.keep2 is not null + returning *; +; +-- set using existing table +insert into dropcol(key, keep1, keep2) values(1, '5', 5) on conflict(key) + do update set keep1 = dropcol.keep1, keep2 = dropcol.keep2 + returning *; +; + +DROP TABLE dropcol; diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql index 561e2fd..4299a5b 100644 --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -1105,7 +1105,7 @@ CREATE RULE hat_upsert AS ON INSERT TO hats ON CONFLICT (hat_name) DO UPDATE SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color - WHERE excluded.hat_color <> 'forbidden' + WHERE excluded.hat_color <> 'forbidden' AND hat_data.* != excluded.* RETURNING *; SELECT definition FROM pg_rules WHERE tablename = 'hats' ORDER BY rulename; -- 2.6.0.rc3
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers