Skip to content

Commit 58d8024

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 7516cdb commit 58d8024

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
@@ -1777,6 +1777,15 @@ find_expr_references_walker(Node *node,
17771777
add_object_address(OCLASS_TYPE, cd->resulttype, 0,
17781778
context->addrs);
17791779
}
1780+
else if (IsA(node, OnConflictExpr))
1781+
{
1782+
OnConflictExpr *onconflict = (OnConflictExpr *) node;
1783+
1784+
if (OidIsValid(onconflict->constraint))
1785+
add_object_address(OCLASS_CONSTRAINT, onconflict->constraint, 0,
1786+
context->addrs);
1787+
/* fall through to examine arguments */
1788+
}
17801789
else if (IsA(node, SortGroupClause))
17811790
{
17821791
SortGroupClause *sgc = (SortGroupClause *) node;

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ planner_hook_type planner_hook = NULL;
6565
#define EXPRKIND_APPINFO 7
6666
#define EXPRKIND_PHV 8
6767
#define EXPRKIND_TABLESAMPLE 9
68+
#define EXPRKIND_ARBITER_ELEM 10
6869

6970
/* Passthrough data for standard_qp_callback */
7071
typedef struct
@@ -483,13 +484,23 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
483484

484485
if (parse->onConflict)
485486
{
487+
parse->onConflict->arbiterElems = (List *)
488+
preprocess_expression(root,
489+
(Node *) parse->onConflict->arbiterElems,
490+
EXPRKIND_ARBITER_ELEM);
491+
parse->onConflict->arbiterWhere =
492+
preprocess_expression(root,
493+
parse->onConflict->arbiterWhere,
494+
EXPRKIND_QUAL);
486495
parse->onConflict->onConflictSet = (List *)
487-
preprocess_expression(root, (Node *) parse->onConflict->onConflictSet,
496+
preprocess_expression(root,
497+
(Node *) parse->onConflict->onConflictSet,
488498
EXPRKIND_TARGET);
489-
490499
parse->onConflict->onConflictWhere =
491-
preprocess_expression(root, (Node *) parse->onConflict->onConflictWhere,
500+
preprocess_expression(root,
501+
parse->onConflict->onConflictWhere,
492502
EXPRKIND_QUAL);
503+
/* exclRelTlist contains only Vars, so no preprocessing needed */
493504
}
494505

495506
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
@@ -2426,6 +2426,7 @@ finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params,
24262426
&context);
24272427
finalize_primnode((Node *) mtplan->onConflictWhere,
24282428
&context);
2429+
/* exclRelTlist contains only Vars, doesn't need examination */
24292430
foreach(l, mtplan->plans)
24302431
{
24312432
context.paramids =

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

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,8 +1031,19 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
10311031
parse->returningList = (List *)
10321032
pullup_replace_vars((Node *) parse->returningList, &rvcontext);
10331033
if (parse->onConflict)
1034+
{
10341035
parse->onConflict->onConflictSet = (List *)
1035-
pullup_replace_vars((Node *) parse->onConflict->onConflictSet, &rvcontext);
1036+
pullup_replace_vars((Node *) parse->onConflict->onConflictSet,
1037+
&rvcontext);
1038+
parse->onConflict->onConflictWhere =
1039+
pullup_replace_vars(parse->onConflict->onConflictWhere,
1040+
&rvcontext);
1041+
1042+
/*
1043+
* We assume ON CONFLICT's arbiterElems, arbiterWhere, exclRelTlist
1044+
* can't contain any references to a subquery
1045+
*/
1046+
}
10361047
replace_vars_in_jointree((Node *) parse->jointree, &rvcontext,
10371048
lowest_nulling_outer_join);
10381049
Assert(parse->setOperations == NULL);
@@ -1625,8 +1636,19 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
16251636
parse->returningList = (List *)
16261637
pullup_replace_vars((Node *) parse->returningList, &rvcontext);
16271638
if (parse->onConflict)
1639+
{
16281640
parse->onConflict->onConflictSet = (List *)
1629-
pullup_replace_vars((Node *) parse->onConflict->onConflictSet, &rvcontext);
1641+
pullup_replace_vars((Node *) parse->onConflict->onConflictSet,
1642+
&rvcontext);
1643+
parse->onConflict->onConflictWhere =
1644+
pullup_replace_vars(parse->onConflict->onConflictWhere,
1645+
&rvcontext);
1646+
1647+
/*
1648+
* We assume ON CONFLICT's arbiterElems, arbiterWhere, exclRelTlist
1649+
* can't contain any references to a subquery
1650+
*/
1651+
}
16301652
replace_vars_in_jointree((Node *) parse->jointree, &rvcontext, NULL);
16311653
Assert(parse->setOperations == NULL);
16321654
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
@@ -530,7 +530,6 @@ infer_arbiter_indexes(PlannerInfo *root)
530530
Bitmapset *indexedAttrs = NULL;
531531
List *idxExprs;
532532
List *predExprs;
533-
List *whereExplicit;
534533
AttrNumber natt;
535534
ListCell *el;
536535

@@ -592,6 +591,7 @@ infer_arbiter_indexes(PlannerInfo *root)
592591
{
593592
int attno = idxRel->rd_index->indkey.values[natt];
594593

594+
/* XXX broken */
595595
if (attno < 0)
596596
elog(ERROR, "system column in index");
597597

@@ -652,13 +652,12 @@ infer_arbiter_indexes(PlannerInfo *root)
652652
goto next;
653653

654654
/*
655-
* Any user-supplied ON CONFLICT unique index inference WHERE clause
656-
* need only be implied by the cataloged index definitions predicate.
655+
* If it's a partial index, its predicate must be implied by the ON
656+
* CONFLICT's WHERE clause.
657657
*/
658658
predExprs = RelationGetIndexPredicate(idxRel);
659-
whereExplicit = make_ands_implicit((Expr *) onconflict->arbiterWhere);
660659

661-
if (!predicate_implied_by(predExprs, whereExplicit))
660+
if (!predicate_implied_by(predExprs, (List *) onconflict->arbiterWhere))
662661
goto next;
663662

664663
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
@@ -5543,12 +5543,15 @@ get_insert_query_def(Query *query, deparse_context *context)
55435543
context->varprefix = save_varprefix;
55445544
}
55455545
}
5546-
else if (confl->constraint != InvalidOid)
5546+
else if (OidIsValid(confl->constraint))
55475547
{
55485548
char *constraint = get_constraint_name(confl->constraint);
55495549

5550+
if (!constraint)
5551+
elog(ERROR, "cache lookup failed for constraint %u",
5552+
confl->constraint);
55505553
appendStringInfo(buf, " ON CONSTRAINT %s",
5551-
quote_qualified_identifier(NULL, constraint));
5554+
quote_identifier(constraint));
55525555
}
55535556

55545557
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)