From 26e66184d6136643d16f6fb167db517fb18b8f89 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 11 May 2016 16:20:03 -0400 Subject: [PATCH] Fix assorted missing infrastructure for ON CONFLICT. subquery_planner() failed to apply expression preprocessing to the arbiterElems and arbiterWhere fields of an OnConflictExpr. No doubt the theory was that this wasn't necessary because we don't actually try to execute those expressions; but that's wrong, because it results in failure to match to index expressions or index predicates that are changed at all by preprocessing. Per bug #14132 from Reynold Smith. Also add pullup_replace_vars processing for onConflictWhere. Perhaps it's impossible to have a subquery reference there, but I'm not exactly convinced; and even if true today it's a failure waiting to happen. Also add some comments to other places where one or another field of OnConflictExpr is intentionally ignored, with explanation as to why it's okay to do so. Also, catalog/dependency.c failed to record any dependency on the named constraint in ON CONFLICT ON CONSTRAINT, allowing such a constraint to be dropped while rules exist that depend on it, and allowing pg_dump to dump such a rule before the constraint it refers to. The normal execution path managed to error out reasonably for a dangling constraint reference, but ruleutils.c dumped core; so in addition to fixing the omission, add a protective check in ruleutils.c, since we can't retroactively add a dependency in existing databases. Back-patch to 9.5 where this code was introduced. Report: <20160510190350.2608.48667@wrigleys.postgresql.org> --- src/backend/catalog/dependency.c | 9 +++++++ src/backend/optimizer/plan/planner.c | 23 +++++++++++----- src/backend/optimizer/plan/subselect.c | 1 + src/backend/optimizer/prep/prepjointree.c | 26 +++++++++++++++++-- src/backend/optimizer/util/plancat.c | 9 +++---- src/backend/utils/adt/ruleutils.c | 7 +++-- src/test/regress/expected/insert_conflict.out | 15 +++++++++++ src/test/regress/sql/insert_conflict.sql | 22 ++++++++++++++++ 8 files changed, 97 insertions(+), 15 deletions(-) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index a6180a64f2..04d7840290 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -1789,6 +1789,15 @@ find_expr_references_walker(Node *node, add_object_address(OCLASS_TYPE, cd->resulttype, 0, context->addrs); } + else if (IsA(node, OnConflictExpr)) + { + OnConflictExpr *onconflict = (OnConflictExpr *) node; + + if (OidIsValid(onconflict->constraint)) + add_object_address(OCLASS_CONSTRAINT, onconflict->constraint, 0, + context->addrs); + /* fall through to examine arguments */ + } else if (IsA(node, SortGroupClause)) { SortGroupClause *sgc = (SortGroupClause *) node; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 6770836dc8..75d93c08db 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -77,6 +77,7 @@ create_upper_paths_hook_type create_upper_paths_hook = NULL; #define EXPRKIND_APPINFO 7 #define EXPRKIND_PHV 8 #define EXPRKIND_TABLESAMPLE 9 +#define EXPRKIND_ARBITER_ELEM 10 /* Passthrough data for standard_qp_callback */ typedef struct @@ -620,13 +621,23 @@ subquery_planner(PlannerGlobal *glob, Query *parse, if (parse->onConflict) { - parse->onConflict->onConflictSet = (List *) - preprocess_expression(root, (Node *) parse->onConflict->onConflictSet, - EXPRKIND_TARGET); - - parse->onConflict->onConflictWhere = - preprocess_expression(root, (Node *) parse->onConflict->onConflictWhere, + parse->onConflict->arbiterElems = (List *) + preprocess_expression(root, + (Node *) parse->onConflict->arbiterElems, + EXPRKIND_ARBITER_ELEM); + parse->onConflict->arbiterWhere = + preprocess_expression(root, + parse->onConflict->arbiterWhere, EXPRKIND_QUAL); + parse->onConflict->onConflictSet = (List *) + preprocess_expression(root, + (Node *) parse->onConflict->onConflictSet, + EXPRKIND_TARGET); + parse->onConflict->onConflictWhere = + preprocess_expression(root, + parse->onConflict->onConflictWhere, + EXPRKIND_QUAL); + /* exclRelTlist contains only Vars, so no preprocessing needed */ } root->append_rel_list = (List *) diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 1ff430229d..0849b1d563 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -2507,6 +2507,7 @@ finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params, &context); finalize_primnode((Node *) mtplan->onConflictWhere, &context); + /* exclRelTlist contains only Vars, doesn't need examination */ foreach(l, mtplan->plans) { context.paramids = diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index 75577bce2c..a334f15773 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -1039,8 +1039,19 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte, parse->returningList = (List *) pullup_replace_vars((Node *) parse->returningList, &rvcontext); if (parse->onConflict) + { parse->onConflict->onConflictSet = (List *) - pullup_replace_vars((Node *) parse->onConflict->onConflictSet, &rvcontext); + pullup_replace_vars((Node *) parse->onConflict->onConflictSet, + &rvcontext); + parse->onConflict->onConflictWhere = + pullup_replace_vars(parse->onConflict->onConflictWhere, + &rvcontext); + + /* + * We assume ON CONFLICT's arbiterElems, arbiterWhere, exclRelTlist + * can't contain any references to a subquery + */ + } replace_vars_in_jointree((Node *) parse->jointree, &rvcontext, lowest_nulling_outer_join); Assert(parse->setOperations == NULL); @@ -1633,8 +1644,19 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte) parse->returningList = (List *) pullup_replace_vars((Node *) parse->returningList, &rvcontext); if (parse->onConflict) + { parse->onConflict->onConflictSet = (List *) - pullup_replace_vars((Node *) parse->onConflict->onConflictSet, &rvcontext); + pullup_replace_vars((Node *) parse->onConflict->onConflictSet, + &rvcontext); + parse->onConflict->onConflictWhere = + pullup_replace_vars(parse->onConflict->onConflictWhere, + &rvcontext); + + /* + * We assume ON CONFLICT's arbiterElems, arbiterWhere, exclRelTlist + * can't contain any references to a subquery + */ + } replace_vars_in_jointree((Node *) parse->jointree, &rvcontext, NULL); Assert(parse->setOperations == NULL); parse->havingQual = pullup_replace_vars(parse->havingQual, &rvcontext); diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 9c11b09cbc..4f399a5b85 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -623,7 +623,6 @@ infer_arbiter_indexes(PlannerInfo *root) Bitmapset *indexedAttrs = NULL; List *idxExprs; List *predExprs; - List *whereExplicit; AttrNumber natt; ListCell *el; @@ -685,6 +684,7 @@ infer_arbiter_indexes(PlannerInfo *root) { int attno = idxRel->rd_index->indkey.values[natt]; + /* XXX broken */ if (attno < 0) elog(ERROR, "system column in index"); @@ -745,13 +745,12 @@ infer_arbiter_indexes(PlannerInfo *root) goto next; /* - * Any user-supplied ON CONFLICT unique index inference WHERE clause - * need only be implied by the cataloged index definitions predicate. + * If it's a partial index, its predicate must be implied by the ON + * CONFLICT's WHERE clause. */ predExprs = RelationGetIndexPredicate(idxRel); - whereExplicit = make_ands_implicit((Expr *) onconflict->arbiterWhere); - if (!predicate_implied_by(predExprs, whereExplicit)) + if (!predicate_implied_by(predExprs, (List *) onconflict->arbiterWhere)) goto next; results = lappend_oid(results, idxForm->indexrelid); diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 6dfa6b9a31..8cb3075e78 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -5570,12 +5570,15 @@ get_insert_query_def(Query *query, deparse_context *context) context->varprefix = save_varprefix; } } - else if (confl->constraint != InvalidOid) + else if (OidIsValid(confl->constraint)) { char *constraint = get_constraint_name(confl->constraint); + if (!constraint) + elog(ERROR, "cache lookup failed for constraint %u", + confl->constraint); appendStringInfo(buf, " ON CONSTRAINT %s", - quote_qualified_identifier(NULL, constraint)); + quote_identifier(constraint)); } if (confl->action == ONCONFLICT_NOTHING) diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out index f56cbd642d..e5c8b4aaac 100644 --- a/src/test/regress/expected/insert_conflict.out +++ b/src/test/regress/expected/insert_conflict.out @@ -454,6 +454,21 @@ ERROR: column excluded.oid does not exist LINE 1: ...values (1) on conflict (key) do update set data = excluded.o... ^ drop table syscolconflicttest; +-- +-- Previous tests all managed to not test any expressions requiring +-- planner preprocessing ... +-- +create table insertconflict (a bigint, b bigint); +create unique index insertconflicti1 on insertconflict(coalesce(a, 0)); +create unique index insertconflicti2 on insertconflict(b) + where coalesce(a, 1) > 0; +insert into insertconflict values (1, 2) +on conflict (coalesce(a, 0)) do nothing; +insert into insertconflict values (1, 2) +on conflict (b) where coalesce(a, 1) > 0 do nothing; +insert into insertconflict values (1, 2) +on conflict (b) where coalesce(a, 1) > 1 do nothing; +drop table insertconflict; -- ****************************************************************** -- * * -- * Test inheritance (example taken from tutorial) * diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql index 8d846f51df..94b1b0aade 100644 --- a/src/test/regress/sql/insert_conflict.sql +++ b/src/test/regress/sql/insert_conflict.sql @@ -261,6 +261,28 @@ insert into syscolconflicttest values (1) on conflict (key) do update set data = insert into syscolconflicttest values (1) on conflict (key) do update set data = excluded.oid::text; drop table syscolconflicttest; +-- +-- Previous tests all managed to not test any expressions requiring +-- planner preprocessing ... +-- +create table insertconflict (a bigint, b bigint); + +create unique index insertconflicti1 on insertconflict(coalesce(a, 0)); + +create unique index insertconflicti2 on insertconflict(b) + where coalesce(a, 1) > 0; + +insert into insertconflict values (1, 2) +on conflict (coalesce(a, 0)) do nothing; + +insert into insertconflict values (1, 2) +on conflict (b) where coalesce(a, 1) > 0 do nothing; + +insert into insertconflict values (1, 2) +on conflict (b) where coalesce(a, 1) > 1 do nothing; + +drop table insertconflict; + -- ****************************************************************** -- * *