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 <[email protected]>
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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
