On Sat, May 30, 2015 at 1:07 AM, Peter Geoghegan <p...@heroku.com> wrote:
> My fix for this issue
> (0001-Fix-bug-with-whole-row-Vars-in-excluded-targetlist.patch) still
> missed something. There needs to be additional handling in
> ruleutils.c:
Debugging this allowed me to come up with a significantly simplified
approach. Attached is a new version of the original fix. Details are
in commit message -- there is no actual need to have
search_indexed_tlist_for_var() care about Vars being resjunk in a
special way, which is a good thing. There is also no need for further
ruleutils.c specialization, as I implied before.
Some deparsing tests are now included on top of what was already in
the first version.
--
Peter Geoghegan
From 9d2f2b51ed3640a6a89313b7be3365168746ee00 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <peter.geoghega...@gmail.com>
Date: Mon, 25 May 2015 01:08:30 -0700
Subject: [PATCH 1/6] Fix bug with whole row Vars in excluded targetlist
Follow the approach taken with RETURNING clauses containing references
to multiple relations; pull up Vars referencing non-target relations
(limited here to the ON CONFLICT DO UPDATE excluded pseudo-relation)
that appear in ON CONFLICT DO UPDATE, and add the Vars to the excluded
relation targetlist as resjunk.
setrefs.c is also changed to call build_tlist_index_other_vars() rather
than build_tlist_index(); this isn't important for correctness, but it
seems preferable to make the code more consistent with the well
established set_returning_clause_references() case.
In passing, make a few minor stylistic tweaks, and reject Vars
referencing the excluded.* pseudo relation in an invalid manner. These
system column Vars are never useful or meaningful, since, of course,
excluded is not a real table.
---
src/backend/executor/execMain.c | 2 +
src/backend/executor/nodeModifyTable.c | 9 +++-
src/backend/optimizer/plan/planner.c | 14 +++--
src/backend/optimizer/plan/setrefs.c | 18 ++++---
src/backend/optimizer/prep/preptlist.c | 74 +++++++++++++++++++++++++--
src/include/optimizer/prep.h | 4 +-
src/test/regress/expected/insert_conflict.out | 50 ++++++++++++++++++
src/test/regress/expected/rules.out | 18 +++----
src/test/regress/sql/insert_conflict.sql | 24 +++++++++
src/test/regress/sql/rules.sql | 2 +-
10 files changed, 183 insertions(+), 32 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index a1561ce..f2c0c64 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1246,6 +1246,8 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_ConstraintExprs = NULL;
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
+ resultRelInfo->ri_onConflictSetProj = NULL;
+ resultRelInfo->ri_onConflictSetWhere = NIL;
}
/*
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 874ca6a..30a092b 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1505,13 +1505,18 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
mtstate->resultRelInfo = estate->es_result_relations + node->resultRelIndex;
mtstate->mt_arowmarks = (List **) palloc0(sizeof(List *) * nplans);
mtstate->mt_nplans = nplans;
- mtstate->mt_onconflict = node->onConflictAction;
- mtstate->mt_arbiterindexes = node->arbiterIndexes;
/* set up epqstate with dummy subplan data for the moment */
EvalPlanQualInit(&mtstate->mt_epqstate, estate, NULL, NIL, node->epqParam);
mtstate->fireBSTriggers = true;
+ /* initialize ON CONFLICT data */
+ mtstate->mt_onconflict = node->onConflictAction;
+ mtstate->mt_arbiterindexes = node->arbiterIndexes;
+ mtstate->mt_existing = NULL;
+ mtstate->mt_excludedtlist = NIL;
+ mtstate->mt_conflproj = NULL;
+
/*
* call ExecInitNode on each of the plans to be executed and save the
* results into the array "mt_plans". This is also a convenient place to
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 8afde2b..4ce6ee85 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -488,7 +488,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
EXPRKIND_TARGET);
parse->onConflict->onConflictWhere =
- preprocess_expression(root, (Node *) parse->onConflict->onConflictWhere,
+ preprocess_expression(root, parse->onConflict->onConflictWhere,
EXPRKIND_QUAL);
}
@@ -1273,7 +1273,6 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
bool use_hashed_grouping = false;
WindowFuncLists *wflists = NULL;
List *activeWindows = NIL;
- OnConflictExpr *onconfl;
int maxref = 0;
int *tleref_to_colnum_map;
List *rollup_lists = NIL;
@@ -1364,12 +1363,11 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
/* Preprocess targetlist */
tlist = preprocess_targetlist(root, tlist);
- onconfl = parse->onConflict;
- if (onconfl)
- onconfl->onConflictSet =
- preprocess_onconflict_targetlist(onconfl->onConflictSet,
- parse->resultRelation,
- parse->rtable);
+ /* Preprocess targetlist for ON CONFLICT DO UPDATE */
+ if (parse->onConflict)
+ preprocess_onconflict_targetlist(parse->onConflict,
+ parse->resultRelation,
+ parse->rtable);
/*
* Expand any rangetable entries that have security barrier quals.
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index a7f65dd..24d50bf 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -106,6 +106,8 @@ static void set_join_references(PlannerInfo *root, Join *join, int rtoffset);
static void set_upper_references(PlannerInfo *root, Plan *plan, int rtoffset);
static void set_dummy_tlist_references(Plan *plan, int rtoffset);
static indexed_tlist *build_tlist_index(List *tlist);
+static indexed_tlist *build_tlist_index_other_vars(List *tlist,
+ Index ignore_rel);
static Var *search_indexed_tlist_for_var(Var *var,
indexed_tlist *itlist,
Index newvarno,
@@ -762,7 +764,9 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
{
indexed_tlist *itlist;
- itlist = build_tlist_index(splan->exclRelTlist);
+ itlist = build_tlist_index_other_vars(splan->exclRelTlist,
+ linitial_int(splan->resultRelations));
+
splan->onConflictSet =
fix_join_expr(root, splan->onConflictSet,
@@ -775,6 +779,7 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
NULL, itlist,
linitial_int(splan->resultRelations),
rtoffset);
+ pfree(itlist);
splan->exclRelTlist =
fix_scan_list(root, splan->exclRelTlist, rtoffset);
@@ -1917,10 +1922,11 @@ search_indexed_tlist_for_sortgroupref(Node *node,
*
* This is used in two different scenarios: a normal join clause, where all
* the Vars in the clause *must* be replaced by OUTER_VAR or INNER_VAR
- * references; and a RETURNING clause, which may contain both Vars of the
- * target relation and Vars of other relations. In the latter case we want
- * to replace the other-relation Vars by OUTER_VAR references, while leaving
- * target Vars alone.
+ * references; and a RETURNING/ON CONFLICT DO UPDATE SET/ON CONFLICT DO UPDATE
+ * WHERE clause, which may contain both Vars of the target relation and Vars of
+ * other relations. In the latter case we want to replace the other-relation
+ * Vars by OUTER_VAR references (or INNER_VAR references for ON CONFLICT),
+ * while leaving target Vars alone.
*
* For a normal join, acceptable_rel should be zero so that any failure to
* match a Var will be reported as an error. For the RETURNING case, pass
@@ -1978,7 +1984,7 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
return (Node *) newvar;
}
- /* Then in the outer */
+ /* Then in the inner */
if (context->inner_itlist)
{
newvar = search_indexed_tlist_for_var(var,
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index 6b0c689..6b7adb7 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -38,6 +38,8 @@
static List *expand_targetlist(List *tlist, int command_type,
Index result_relation, List *range_table);
+static void pull_var_targetlist_clause(OnConflictExpr *onconfl, List* clause,
+ int result_relation);
/*
@@ -185,12 +187,28 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
* preprocess_onconflict_targetlist
* Process ON CONFLICT SET targetlist.
*
- * Returns the new targetlist.
+ * Modifies passed ON CONFLICT expression in-place.
*/
-List *
-preprocess_onconflict_targetlist(List *tlist, int result_relation, List *range_table)
+void
+preprocess_onconflict_targetlist(OnConflictExpr *onconfl, int result_relation,
+ List *range_table)
{
- return expand_targetlist(tlist, CMD_UPDATE, result_relation, range_table);
+ List *tlist;
+
+ tlist = expand_targetlist(onconfl->onConflictSet, CMD_UPDATE,
+ result_relation, range_table);
+
+ /*
+ * Perform similar pull up process to that performed by
+ * preprocess_targetlist() for multi-relation RETURNING lists. This is
+ * only necessary for the benefit of excluded.* whole row Vars, and to
+ * enforce that excluded.* system column references are disallowed.
+ */
+ pull_var_targetlist_clause(onconfl, tlist, result_relation);
+ pull_var_targetlist_clause(onconfl, (List *) onconfl->onConflictWhere,
+ result_relation);
+
+ onconfl->onConflictSet = tlist;
}
@@ -376,6 +394,54 @@ expand_targetlist(List *tlist, int command_type,
return new_tlist;
}
+/*
+ * pull_var_targetlist_clause
+ * Given an ON CONFLICT clause as generated by the parser and a result
+ * relation, add resjunk entries for any Vars used that are not
+ * associated with the target and are not excluded.* non-resjunk
+ * TLE Vars.
+ */
+static void
+pull_var_targetlist_clause(OnConflictExpr *onconfl, List* clause,
+ int result_relation)
+{
+ List *vars;
+ ListCell *l;
+
+ vars = pull_var_clause((Node *) clause,
+ PVC_RECURSE_AGGREGATES,
+ PVC_INCLUDE_PLACEHOLDERS);
+ foreach(l, vars)
+ {
+ Var *var = (Var *) lfirst(l);
+ TargetEntry *tle;
+
+ if (IsA(var, Var) && var->varno == result_relation)
+ continue; /* don't need it */
+
+ if (IsA(var, Var) && var->varno == onconfl->exclRelIndex)
+ {
+ if (var->varattno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+ errmsg("system columns from excluded table cannot be referenced")));
+ else if (var->varattno > 0) /* don't need it */
+ continue;
+ }
+
+ if (tlist_member((Node *) var, onconfl->exclRelTlist))
+ continue; /* already got it */
+
+ if (var->varattno != InvalidAttrNumber)
+ elog(ERROR, "cannot pull up non-wholerow Var in excluded targetlist");
+
+ tle = makeTargetEntry((Expr *) var, var->varattno, NULL, true);
+
+ onconfl->exclRelTlist = lappend(onconfl->exclRelTlist, tle);
+ }
+
+ list_free(vars);
+}
/*
* Locate PlanRowMark for given RT index, or return NULL if none
diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h
index 7b8c0a9..8d61c7c 100644
--- a/src/include/optimizer/prep.h
+++ b/src/include/optimizer/prep.h
@@ -45,8 +45,8 @@ extern void expand_security_quals(PlannerInfo *root, List *tlist);
*/
extern List *preprocess_targetlist(PlannerInfo *root, List *tlist);
-extern List *preprocess_onconflict_targetlist(List *tlist,
- int result_relation, List *range_table);
+extern void preprocess_onconflict_targetlist(OnConflictExpr *onconfl,
+ int result_relation, List *range_table);
extern PlanRowMark *get_plan_rowmark(List *rowmarks, Index rtindex);
diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out
index eca9690..0d8b26b 100644
--- a/src/test/regress/expected/insert_conflict.out
+++ b/src/test/regress/expected/insert_conflict.out
@@ -363,6 +363,56 @@ 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: system columns from excluded table cannot be referenced
+drop index plain;
-- Cleanup
drop table insertconflicttest;
-- ******************************************************************
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 60c1f40..23dda7d 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2893,7 +2893,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
@@ -2901,7 +2901,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)
@@ -2949,19 +2949,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)
@@ -2988,12 +2988,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 a0bdd7f..42a9b54 100644
--- a/src/test/regress/sql/insert_conflict.sql
+++ b/src/test/regress/sql/insert_conflict.sql
@@ -212,6 +212,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;
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;
--
1.9.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
