From 23f33298485ecff80f01feb0dbd349cca2b38032 Mon Sep 17 00:00:00 2001 From: Marti Raudsepp Date: Tue, 18 Oct 2011 21:31:42 +0300 Subject: [PATCH] Don't allow join removal for deferrable unique constraints This used to be allowed, but could return incorrect results if the deferrable constraint is invalid in the middle of a transaction. --- src/backend/catalog/pg_constraint.c | 4 +- src/backend/optimizer/path/indxpath.c | 101 ++++++++++++++++++++++++++++++++- src/test/regress/expected/join.out | 39 +++++++++++++ src/test/regress/sql/join.sql | 24 ++++++++ 4 files changed, 164 insertions(+), 4 deletions(-) diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 6997994..7dbaca0 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -842,8 +842,8 @@ check_functional_grouping(Oid relid, /* Only PK constraints are of interest for now, see comment above */ if (con->contype != CONSTRAINT_PRIMARY) continue; - /* Constraint must be non-deferrable */ - if (con->condeferrable) + /* Constraint must be non-deferrable and valid */ + if (con->condeferrable || !con->convalidated) continue; /* Extract the conkey array, ie, attnums of PK's columns */ diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c index 940efb3..6d46e75 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -17,10 +17,14 @@ #include +#include "access/genam.h" +#include "access/heapam.h" #include "access/skey.h" #include "access/sysattr.h" +#include "catalog/indexing.h" #include "catalog/pg_am.h" #include "catalog/pg_collation.h" +#include "catalog/pg_constraint.h" #include "catalog/pg_operator.h" #include "catalog/pg_opfamily.h" #include "catalog/pg_type.h" @@ -34,9 +38,12 @@ #include "optimizer/var.h" #include "utils/builtins.h" #include "utils/bytea.h" +#include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/pg_locale.h" #include "utils/selfuncs.h" +#include "utils/tqual.h" +#include "storage/lock.h" #define IsBooleanOpfamily(opfamily) \ @@ -116,6 +123,8 @@ static bool matches_any_index(RestrictInfo *rinfo, RelOptInfo *rel, Relids outer_relids); static List *find_clauses_for_join(PlannerInfo *root, RelOptInfo *rel, Relids outer_relids, bool isouterjoin); +static bool unique_index_is_consistent(Oid reloid, Oid indexoid, + Oid *conoid); static bool match_boolean_index_clause(Node *clause, int indexcol, IndexOptInfo *index); static bool match_special_index_operator(Expr *clause, @@ -2248,6 +2257,71 @@ find_clauses_for_join(PlannerInfo *root, RelOptInfo *rel, } /* + * unique_constraint_is_consistent + * Make sure that a unique index's respective constraint is validated and + * not deferrable. Sets *conoid to the found constraint OID, or InvalidOid + * if not found. + * + * This is expensive; there is on index on pg_constraint.conindid, so we have + * to scan all constraints on the relation. + */ +static bool +unique_index_is_consistent(Oid reloid, Oid indexoid, + Oid *conoid) +{ + /* If no constraint is found, then the index cannot be invalid/deferrable */ + bool result = true; + Relation pg_constraint; + HeapTuple tuple; + SysScanDesc scan; + ScanKeyData skey[1]; + + *conoid = InvalidOid; + + /* Scan pg_constraint for constraints of the target rel */ + pg_constraint = heap_open(ConstraintRelationId, AccessShareLock); + + ScanKeyInit(&skey[0], + Anum_pg_constraint_conrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(reloid)); + + scan = systable_beginscan(pg_constraint, ConstraintRelidIndexId, true, + SnapshotNow, 1, skey); + + while (HeapTupleIsValid(tuple = systable_getnext(scan))) + { + Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(tuple); + + if (con->conindid == indexoid) + { + if (con->condeferrable) + result = false; + + /* + * Currently unique constraints are always valid, but that could + * change in the future. This check costs us nothing. + */ + if (!con->convalidated) + result = false; + + *conoid = HeapTupleGetOid(tuple); + + /* + * Stop searching since we found a constraint. Assumes that + * conindid is unique. + */ + break; + } + } + + systable_endscan(scan); + heap_close(pg_constraint, AccessShareLock); + + return result; +} + +/* * relation_has_unique_index_for * Determine whether the relation provably has at most one row satisfying * a set of equality conditions, because the conditions constrain all @@ -2274,6 +2348,8 @@ relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel, foreach(ic, rel->indexlist) { IndexOptInfo *ind = (IndexOptInfo *) lfirst(ic); + RangeTblEntry *rte; + Oid conoid = InvalidOid; int c; /* @@ -2326,8 +2402,29 @@ relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel, } /* Matched all columns of this index? */ - if (c == ind->ncolumns) - return true; + if (c != ind->ncolumns) + continue; + + /* + * Deferrable or invalid constraints don't qualify for removal. This + * check is last since it's the most expensive -- requires a catalog + * lookup. + * + * It's tempting to peek into the queue of deferred unique checks + * and see if it's empty, but there's no mechanism to invalidate the + * plan when that queue changes. + */ + rte = root->simple_rte_array[rel->relid]; + Assert(rte->rtekind == RTE_RELATION); + + if (!unique_index_is_consistent(rte->relid, ind->indexoid, &conoid)) + continue; + + /* Query plan now relies on this constraint */ + if (conoid != InvalidOid) + root->parse->constraintDeps = lappend_oid(root->parse->constraintDeps, conoid); + + return true; } return false; diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index a54000b..9217714 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -2755,3 +2755,42 @@ SELECT * FROM (5 rows) rollback; +-- and just one more bug, deferrable unique constraints can't be relied on +create temp table uniq (i int); +create unique index uniq_i_idx on uniq (i); +begin; +prepare join_removal as + select a.* from uniq as a left join uniq as b using (i); +explain (costs off) execute join_removal; + QUERY PLAN +-------------------- + Seq Scan on uniq a +(1 row) + +-- query must be replanned after a deferrable constraint is added +alter table uniq add constraint uniq_i_idx + unique using index uniq_i_idx deferrable initially deferred; +explain (costs off) execute join_removal; + QUERY PLAN +-------------------------------- + Hash Left Join + Hash Cond: (a.i = b.i) + -> Seq Scan on uniq a + -> Hash + -> Seq Scan on uniq b +(5 rows) + +-- test actual results +insert into uniq values(1),(1); +execute join_removal; + i +--- + 1 + 1 + 1 + 1 +(4 rows) + +rollback; +deallocate join_removal; +drop table uniq; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 1be80b8..ca46bae 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -736,3 +736,27 @@ SELECT * FROM ON true; rollback; + +-- and just one more bug, deferrable unique constraints can't be relied on +create temp table uniq (i int); +create unique index uniq_i_idx on uniq (i); + +begin; +prepare join_removal as + select a.* from uniq as a left join uniq as b using (i); + +explain (costs off) execute join_removal; + +-- query must be replanned after a deferrable constraint is added +alter table uniq add constraint uniq_i_idx + unique using index uniq_i_idx deferrable initially deferred; + +explain (costs off) execute join_removal; + +-- test actual results +insert into uniq values(1),(1); +execute join_removal; + +rollback; +deallocate join_removal; +drop table uniq; -- 1.7.7