Skip to content

Commit 26e6618

Browse files
tglsfdctglsfdc
committed
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>
1 parent 9be58a2 commit 26e6618

File tree

8 files changed

+94
-12
lines changed

8 files changed

+94
-12
lines changed

‎src/backend/catalog/dependency.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1789,6 +1789,15 @@ find_expr_references_walker(Node *node,
17891789
add_object_address(OCLASS_TYPE, cd->resulttype, 0,
17901790
context->addrs);
17911791
}
1792+
else if (IsA(node, OnConflictExpr))
1793+
{
1794+
OnConflictExpr *onconflict = (OnConflictExpr *) node;
1795+
1796+
if (OidIsValid(onconflict->constraint))
1797+
add_object_address(OCLASS_CONSTRAINT, onconflict->constraint, 0,
1798+
context->addrs);
1799+
/* fall through to examine arguments */
1800+
}
17921801
else if (IsA(node, SortGroupClause))
17931802
{
17941803
SortGroupClause *sgc = (SortGroupClause *) node;

‎src/backend/optimizer/plan/planner.c

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ create_upper_paths_hook_type create_upper_paths_hook = NULL;
7777
#define EXPRKIND_APPINFO 7
7878
#define EXPRKIND_PHV 8
7979
#define EXPRKIND_TABLESAMPLE 9
80+
#define EXPRKIND_ARBITER_ELEM 10
8081

8182
/* Passthrough data for standard_qp_callback */
8283
typedef struct
@@ -620,13 +621,23 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
620621

621622
if (parse->onConflict)
622623
{
624+
parse->onConflict->arbiterElems = (List *)
625+
preprocess_expression(root,
626+
(Node *) parse->onConflict->arbiterElems,
627+
EXPRKIND_ARBITER_ELEM);
628+
parse->onConflict->arbiterWhere =
629+
preprocess_expression(root,
630+
parse->onConflict->arbiterWhere,
631+
EXPRKIND_QUAL);
623632
parse->onConflict->onConflictSet = (List *)
624-
preprocess_expression(root, (Node *) parse->onConflict->onConflictSet,
633+
preprocess_expression(root,
634+
(Node *) parse->onConflict->onConflictSet,
625635
EXPRKIND_TARGET);
626-
627636
parse->onConflict->onConflictWhere =
628-
preprocess_expression(root, (Node *) parse->onConflict->onConflictWhere,
637+
preprocess_expression(root,
638+
parse->onConflict->onConflictWhere,
629639
EXPRKIND_QUAL);
640+
/* exclRelTlist contains only Vars, so no preprocessing needed */
630641
}
631642

632643
root->append_rel_list = (List *)

‎src/backend/optimizer/plan/subselect.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2507,6 +2507,7 @@ finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params,
25072507
&context);
25082508
finalize_primnode((Node *) mtplan->onConflictWhere,
25092509
&context);
2510+
/* exclRelTlist contains only Vars, doesn't need examination */
25102511
foreach(l, mtplan->plans)
25112512
{
25122513
context.paramids =

‎src/backend/optimizer/prep/prepjointree.c

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1039,8 +1039,19 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
10391039
parse->returningList = (List *)
10401040
pullup_replace_vars((Node *) parse->returningList, &rvcontext);
10411041
if (parse->onConflict)
1042+
{
10421043
parse->onConflict->onConflictSet = (List *)
1043-
pullup_replace_vars((Node *) parse->onConflict->onConflictSet, &rvcontext);
1044+
pullup_replace_vars((Node *) parse->onConflict->onConflictSet,
1045+
&rvcontext);
1046+
parse->onConflict->onConflictWhere =
1047+
pullup_replace_vars(parse->onConflict->onConflictWhere,
1048+
&rvcontext);
1049+
1050+
/*
1051+
* We assume ON CONFLICT's arbiterElems, arbiterWhere, exclRelTlist
1052+
* can't contain any references to a subquery
1053+
*/
1054+
}
10441055
replace_vars_in_jointree((Node *) parse->jointree, &rvcontext,
10451056
lowest_nulling_outer_join);
10461057
Assert(parse->setOperations == NULL);
@@ -1633,8 +1644,19 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
16331644
parse->returningList = (List *)
16341645
pullup_replace_vars((Node *) parse->returningList, &rvcontext);
16351646
if (parse->onConflict)
1647+
{
16361648
parse->onConflict->onConflictSet = (List *)
1637-
pullup_replace_vars((Node *) parse->onConflict->onConflictSet, &rvcontext);
1649+
pullup_replace_vars((Node *) parse->onConflict->onConflictSet,
1650+
&rvcontext);
1651+
parse->onConflict->onConflictWhere =
1652+
pullup_replace_vars(parse->onConflict->onConflictWhere,
1653+
&rvcontext);
1654+
1655+
/*
1656+
* We assume ON CONFLICT's arbiterElems, arbiterWhere, exclRelTlist
1657+
* can't contain any references to a subquery
1658+
*/
1659+
}
16381660
replace_vars_in_jointree((Node *) parse->jointree, &rvcontext, NULL);
16391661
Assert(parse->setOperations == NULL);
16401662
parse->havingQual = pullup_replace_vars(parse->havingQual, &rvcontext);

‎src/backend/optimizer/util/plancat.c

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -623,7 +623,6 @@ infer_arbiter_indexes(PlannerInfo *root)
623623
Bitmapset *indexedAttrs = NULL;
624624
List *idxExprs;
625625
List *predExprs;
626-
List *whereExplicit;
627626
AttrNumber natt;
628627
ListCell *el;
629628

@@ -685,6 +684,7 @@ infer_arbiter_indexes(PlannerInfo *root)
685684
{
686685
int attno = idxRel->rd_index->indkey.values[natt];
687686

687+
/* XXX broken */
688688
if (attno < 0)
689689
elog(ERROR, "system column in index");
690690

@@ -745,13 +745,12 @@ infer_arbiter_indexes(PlannerInfo *root)
745745
goto next;
746746

747747
/*
748-
* Any user-supplied ON CONFLICT unique index inference WHERE clause
749-
* need only be implied by the cataloged index definitions predicate.
748+
* If it's a partial index, its predicate must be implied by the ON
749+
* CONFLICT's WHERE clause.
750750
*/
751751
predExprs = RelationGetIndexPredicate(idxRel);
752-
whereExplicit = make_ands_implicit((Expr *) onconflict->arbiterWhere);
753752

754-
if (!predicate_implied_by(predExprs, whereExplicit))
753+
if (!predicate_implied_by(predExprs, (List *) onconflict->arbiterWhere))
755754
goto next;
756755

757756
results = lappend_oid(results, idxForm->indexrelid);

‎src/backend/utils/adt/ruleutils.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5570,12 +5570,15 @@ get_insert_query_def(Query *query, deparse_context *context)
55705570
context->varprefix = save_varprefix;
55715571
}
55725572
}
5573-
else if (confl->constraint != InvalidOid)
5573+
else if (OidIsValid(confl->constraint))
55745574
{
55755575
char *constraint = get_constraint_name(confl->constraint);
55765576

5577+
if (!constraint)
5578+
elog(ERROR, "cache lookup failed for constraint %u",
5579+
confl->constraint);
55775580
appendStringInfo(buf, " ON CONSTRAINT %s",
5578-
quote_qualified_identifier(NULL, constraint));
5581+
quote_identifier(constraint));
55795582
}
55805583

55815584
if (confl->action == ONCONFLICT_NOTHING)

‎src/test/regress/expected/insert_conflict.out

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -454,6 +454,21 @@ ERROR: column excluded.oid does not exist
454454
LINE 1: ...values (1) on conflict (key) do update set data = excluded.o...
455455
^
456456
drop table syscolconflicttest;
457+
--
458+
-- Previous tests all managed to not test any expressions requiring
459+
-- planner preprocessing ...
460+
--
461+
create table insertconflict (a bigint, b bigint);
462+
create unique index insertconflicti1 on insertconflict(coalesce(a, 0));
463+
create unique index insertconflicti2 on insertconflict(b)
464+
where coalesce(a, 1) > 0;
465+
insert into insertconflict values (1, 2)
466+
on conflict (coalesce(a, 0)) do nothing;
467+
insert into insertconflict values (1, 2)
468+
on conflict (b) where coalesce(a, 1) > 0 do nothing;
469+
insert into insertconflict values (1, 2)
470+
on conflict (b) where coalesce(a, 1) > 1 do nothing;
471+
drop table insertconflict;
457472
-- ******************************************************************
458473
-- * *
459474
-- * Test inheritance (example taken from tutorial) *

‎src/test/regress/sql/insert_conflict.sql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,28 @@ insert into syscolconflicttest values (1) on conflict (key) do update set data =
261261
insert into syscolconflicttest values (1) on conflict (key) do update set data = excluded.oid::text;
262262
drop table syscolconflicttest;
263263

264+
--
265+
-- Previous tests all managed to not test any expressions requiring
266+
-- planner preprocessing ...
267+
--
268+
create table insertconflict (a bigint, b bigint);
269+
270+
create unique index insertconflicti1 on insertconflict(coalesce(a, 0));
271+
272+
create unique index insertconflicti2 on insertconflict(b)
273+
where coalesce(a, 1) > 0;
274+
275+
insert into insertconflict values (1, 2)
276+
on conflict (coalesce(a, 0)) do nothing;
277+
278+
insert into insertconflict values (1, 2)
279+
on conflict (b) where coalesce(a, 1) > 0 do nothing;
280+
281+
insert into insertconflict values (1, 2)
282+
on conflict (b) where coalesce(a, 1) > 1 do nothing;
283+
284+
drop table insertconflict;
285+
264286

265287
-- ******************************************************************
266288
-- * *

0 commit comments

Comments
 (0)