On Thu, May 28, 2015 at 2:37 PM, Peter Geoghegan <p...@heroku.com> wrote:
> A second attached patch fixes another, largely independent bug. I
> noticed array assignment with ON CONFLICT DO UPDATE was broken. See
> commit message for full details.
Finally, here is a third patch, fixing the final bug that I discussed
with you privately. There are now fixes for all bugs that I'm
currently aware of.
This concerns a thinko in unique index inference. See the commit
message for full details.
--
Peter Geoghegan
From 904f46f4d9e7758c588254e2c2bb3ef3ef93f3c9 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <peter.geoghega...@gmail.com>
Date: Thu, 28 May 2015 15:45:48 -0700
Subject: [PATCH 3/3] Fix bug in unique index inference
ON CONFLICT unique index inference had a thinko that could affect cases
where the user-supplied inference clause required that an attribute
match a particular (user named) collation and/or opclass.
Firstly, infer_collation_opclass_match() matched on opclass and/or
collation. Secondly, the attribute had to be in the list of attributes
or expressions known to be in the definition of the index under
consideration. The second step wasn't correct though, because having
some match doesn't necessarily mean that the second step found the same
index attribute as the (collation/opclass wise) match from the first
step.
To fix, make infer_collation_opclass_match() more discriminating in its
second step.
---
src/backend/optimizer/util/plancat.c | 45 +++++++++++++++++----------
src/test/regress/expected/insert_conflict.out | 18 +++++++++++
src/test/regress/sql/insert_conflict.sql | 12 +++++++
3 files changed, 59 insertions(+), 16 deletions(-)
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index b04dc2e..1b81f4c 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -52,7 +52,7 @@ get_relation_info_hook_type get_relation_info_hook = NULL;
static bool infer_collation_opclass_match(InferenceElem *elem, Relation idxRel,
- Bitmapset *inferAttrs, List *idxExprs);
+ List *idxExprs);
static int32 get_rel_data_width(Relation rel, int32 *attr_widths);
static List *get_relation_constraints(PlannerInfo *root,
Oid relationObjectId, RelOptInfo *rel,
@@ -615,8 +615,7 @@ infer_arbiter_indexes(PlannerInfo *root)
* this for both expressions and ordinary (non-expression)
* attributes appearing as inference elements.
*/
- if (!infer_collation_opclass_match(elem, idxRel, inferAttrs,
- idxExprs))
+ if (!infer_collation_opclass_match(elem, idxRel, idxExprs))
goto next;
/*
@@ -681,11 +680,10 @@ next:
* infer_collation_opclass_match - ensure infer element opclass/collation match
*
* Given unique index inference element from inference specification, if
- * collation was specified, or if opclass (represented here as opfamily +
- * opcintype) was specified, verify that there is at least one matching
- * indexed attribute (occasionally, there may be more). Skip this in the
- * common case where inference specification does not include collation or
- * opclass (instead matching everything, regardless of cataloged
+ * collation was specified, or if opclass was specified, verify that there is
+ * at least one matching indexed attribute (occasionally, there may be more).
+ * Skip this in the common case where inference specification does not include
+ * collation or opclass (instead matching everything, regardless of cataloged
* collation/opclass of indexed attribute).
*
* At least historically, Postgres has not offered collations or opclasses
@@ -707,11 +705,12 @@ next:
*/
static bool
infer_collation_opclass_match(InferenceElem *elem, Relation idxRel,
- Bitmapset *inferAttrs, List *idxExprs)
+ List *idxExprs)
{
AttrNumber natt;
- Oid inferopfamily = InvalidOid; /* OID of att opfamily */
- Oid inferopcinputtype = InvalidOid; /* OID of att opfamily */
+ Oid inferopfamily = InvalidOid; /* OID of opclass opfamily */
+ Oid inferopcinputtype = InvalidOid; /* OID of opclass input type */
+ int nplain = 0; /* # plain attrs observed */
/*
* If inference specification element lacks collation/opclass, then no
@@ -734,6 +733,10 @@ infer_collation_opclass_match(InferenceElem *elem, Relation idxRel,
Oid opfamily = idxRel->rd_opfamily[natt - 1];
Oid opcinputtype = idxRel->rd_opcintype[natt - 1];
Oid collation = idxRel->rd_indcollation[natt - 1];
+ int attno = idxRel->rd_index->indkey.values[natt - 1];
+
+ if (attno != 0)
+ nplain++;
if (elem->inferopclass != InvalidOid &&
(inferopfamily != opfamily || inferopcinputtype != opcinputtype))
@@ -749,12 +752,22 @@ infer_collation_opclass_match(InferenceElem *elem, Relation idxRel,
continue;
}
- if ((IsA(elem->expr, Var) &&
- bms_is_member(((Var *) elem->expr)->varattno, inferAttrs)) ||
- list_member(idxExprs, elem->expr))
+ /* If one matching index att found, good enough -- return true */
+ if (IsA(elem->expr, Var))
{
- /* Found one match - good enough */
- return true;
+ if (((Var *) elem->expr)->varattno == attno)
+ return true;
+ }
+ else
+ {
+ Node *nattExpr = list_nth(idxExprs, (natt - 1) - nplain);
+
+ /*
+ * Note that unlike routines like match_index_to_operand(), we're
+ * unconcerned about RelabelType. An exact match is required.
+ */
+ if (equal(elem->expr, nattExpr))
+ return true;
}
}
diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out
index 442d9fc..8aa0918 100644
--- a/src/test/regress/expected/insert_conflict.out
+++ b/src/test/regress/expected/insert_conflict.out
@@ -141,6 +141,24 @@ drop index collation_index_key;
drop index both_index_key;
drop index both_index_expr_key;
--
+-- Make sure that cross matching of attribute opclass/collation does not occur
+--
+create unique index cross_match on insertconflicttest(lower(fruit) collate "C", upper(fruit) text_pattern_ops);
+-- fails:
+explain (costs off) insert into insertconflicttest values(0, 'Crowberry') on conflict (lower(fruit) text_pattern_ops, upper(fruit) collate "C") do nothing;
+ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification
+-- works:
+explain (costs off) insert into insertconflicttest values(0, 'Crowberry') on conflict (lower(fruit) collate "C", upper(fruit) text_pattern_ops) do nothing;
+ QUERY PLAN
+-----------------------------------------
+ Insert on insertconflicttest
+ Conflict Resolution: NOTHING
+ Conflict Arbiter Indexes: cross_match
+ -> Result
+(4 rows)
+
+drop index cross_match;
+--
-- Single key tests
--
create unique index key_index on insertconflicttest(key);
diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql
index a2b6aba..72440af 100644
--- a/src/test/regress/sql/insert_conflict.sql
+++ b/src/test/regress/sql/insert_conflict.sql
@@ -58,6 +58,18 @@ drop index both_index_key;
drop index both_index_expr_key;
--
+-- Make sure that cross matching of attribute opclass/collation does not occur
+--
+create unique index cross_match on insertconflicttest(lower(fruit) collate "C", upper(fruit) text_pattern_ops);
+
+-- fails:
+explain (costs off) insert into insertconflicttest values(0, 'Crowberry') on conflict (lower(fruit) text_pattern_ops, upper(fruit) collate "C") do nothing;
+-- works:
+explain (costs off) insert into insertconflicttest values(0, 'Crowberry') on conflict (lower(fruit) collate "C", upper(fruit) text_pattern_ops) do nothing;
+
+drop index cross_match;
+
+--
-- Single key tests
--
create unique index key_index on insertconflicttest(key);
--
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
