On Tue, May 12, 2015 at 3:16 PM, Andres Freund <and...@anarazel.de> wrote:
> Could you rebase and adjust your patch? I'd rather have the inference
> structure refactoring separate.
I realized that I didn't split out the patch as requested.
Here is a cumulative version of what was previously posted.
Thanks
--
Peter Geoghegan
From b089633e7db8a778449ea609fad012bec317f02c Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <peter.geoghega...@gmail.com>
Date: Mon, 18 May 2015 12:49:13 -0700
Subject: [PATCH 2/2] Further fixes to minor ON CONFLICT issues
Deparsing with an inference clause is now correctly supported.
Finally, fix a few typos, and rename a variable -- "candidates" seemed
like a misnomer for the return value of infer_arbiter_indexes().
---
src/backend/executor/nodeModifyTable.c | 2 +-
src/backend/optimizer/util/plancat.c | 14 ++---
src/backend/parser/parse_clause.c | 2 +-
src/backend/utils/adt/ruleutils.c | 75 ++++++++++++++++++++++++++-
src/test/regress/expected/insert_conflict.out | 2 +-
src/test/regress/expected/rules.out | 38 ++++++++------
src/test/regress/sql/rules.sql | 7 ++-
7 files changed, 109 insertions(+), 31 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 89f1f57..72bbd62 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -414,7 +414,7 @@ ExecInsert(ModifyTableState *mtstate,
estate, true, &specConflict,
arbiterIndexes);
- /* adjust the tuple's state accordingly */
+ /* adjust the tuple's state */
if (!specConflict)
heap_finish_speculative(resultRelationDesc, tuple);
else
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index cbb4776..a857ba3 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -438,8 +438,8 @@ infer_arbiter_indexes(PlannerInfo *root)
Bitmapset *inferAttrs = NULL;
List *inferElems = NIL;
- /* Result */
- List *candidates = NIL;
+ /* Results */
+ List *results = NIL;
/*
* Quickly return NIL for ON CONFLICT DO NOTHING without an inference
@@ -565,11 +565,11 @@ infer_arbiter_indexes(PlannerInfo *root)
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("ON CONFLICT DO UPDATE not supported with exclusion constraints")));
- candidates = lappend_oid(candidates, idxForm->indexrelid);
+ results = lappend_oid(results, idxForm->indexrelid);
list_free(indexList);
index_close(idxRel, NoLock);
heap_close(relation, NoLock);
- return candidates;
+ return results;
}
else if (indexOidFromConstraint != InvalidOid)
{
@@ -660,7 +660,7 @@ infer_arbiter_indexes(PlannerInfo *root)
if (!predicate_implied_by(predExprs, whereExplicit))
goto next;
- candidates = lappend_oid(candidates, idxForm->indexrelid);
+ results = lappend_oid(results, idxForm->indexrelid);
next:
index_close(idxRel, NoLock);
}
@@ -668,12 +668,12 @@ next:
list_free(indexList);
heap_close(relation, NoLock);
- if (candidates == NIL)
+ if (results == NIL)
ereport(ERROR,
(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
errmsg("there is no unique or exclusion constraint matching the ON CONFLICT specification")));
- return candidates;
+ return results;
}
/*
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index c8af5ab..f8eebfe 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -2765,7 +2765,7 @@ transformOnConflictArbiter(ParseState *pstate,
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("ON CONFLICT DO UPDATE requires inference specification or constraint name"),
- errhint("For example, ON CONFLICT ON CONFLICT (<column>)."),
+ errhint("For example, ON CONFLICT (<column>)."),
parser_errposition(pstate,
exprLocation((Node *) onConflictClause))));
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 0a77400..b54d508 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -5479,13 +5479,38 @@ get_insert_query_def(Query *query, deparse_context *context)
{
OnConflictExpr *confl = query->onConflict;
+ appendStringInfo(buf, " ON CONFLICT");
+
+ if (confl->arbiterElems)
+ {
+ /* Add the single-VALUES expression list */
+ appendStringInfoChar(buf, '(');
+ get_rule_expr((Node *) confl->arbiterElems, context, false);
+ appendStringInfoChar(buf, ')');
+
+ /* Add a WHERE clause (for partial indexes) if given */
+ if (confl->arbiterWhere != NULL)
+ {
+ appendContextKeyword(context, " WHERE ",
+ -PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
+ get_rule_expr(confl->arbiterWhere, context, false);
+ }
+ }
+ else
+ {
+ char *constraint = get_constraint_name(confl->constraint);
+
+ appendStringInfo(buf, " ON CONSTRAINT %s",
+ quote_qualified_identifier(NULL, constraint));
+ }
+
if (confl->action == ONCONFLICT_NOTHING)
{
- appendStringInfoString(buf, " ON CONFLICT DO NOTHING");
+ appendStringInfoString(buf, " DO NOTHING");
}
else
{
- appendStringInfoString(buf, " ON CONFLICT DO UPDATE SET ");
+ appendStringInfoString(buf, " DO UPDATE SET ");
/* Deparse targetlist */
get_update_query_targetlist_def(query, confl->onConflictSet,
context, rte);
@@ -7886,6 +7911,52 @@ get_rule_expr(Node *node, deparse_context *context,
}
break;
+ case T_InferenceElem:
+ {
+ InferenceElem *iexpr = (InferenceElem *) node;
+ bool varprefix = context->varprefix;
+ bool need_parens;
+
+ /*
+ * InfereneElem can only refer to target relation, so prefix is
+ * never useful
+ */
+ context->varprefix = false;
+
+ /*
+ * Parenthesize the element unless it's a simple Var or a bare
+ * function call. Follows pg_get_indexdef_worker().
+ */
+ need_parens = !IsA(iexpr->expr, Var);
+ if (IsA(iexpr->expr, FuncExpr) &&
+ ((FuncExpr *) iexpr->expr)->funcformat ==
+ COERCE_EXPLICIT_CALL)
+ need_parens = false;
+
+ if (need_parens)
+ appendStringInfoChar(buf, '(');
+ get_rule_expr((Node *) iexpr->expr,
+ context, false);
+ if (need_parens)
+ appendStringInfoChar(buf, ')');
+
+ context->varprefix = varprefix;
+
+ if (iexpr->infercollid)
+ appendStringInfo(buf, " COLLATE %s",
+ generate_collation_name(iexpr->infercollid));
+
+ /* Add the operator class name, if not default */
+ if (iexpr->inferopclass)
+ {
+ Oid inferopclass = iexpr->inferopclass;
+ Oid inferopcinputtype = get_opclass_input_type(iexpr->inferopclass);
+
+ get_opclass_name(inferopclass, inferopcinputtype, buf);
+ }
+ }
+ break;
+
case T_List:
{
char *sep;
diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out
index 3273d98..a6abd8b 100644
--- a/src/test/regress/expected/insert_conflict.out
+++ b/src/test/regress/expected/insert_conflict.out
@@ -192,7 +192,7 @@ insert into insertconflicttest values (1, 'Apple') on conflict do update set fru
ERROR: ON CONFLICT DO UPDATE requires inference specification or constraint name
LINE 1: ...nsert into insertconflicttest values (1, 'Apple') on conflic...
^
-HINT: For example, ON CONFLICT ON CONFLICT (<column>).
+HINT: For example, ON CONFLICT (<column>).
-- inference succeeds:
insert into insertconflicttest values (1, 'Apple') on conflict (key) do update set fruit = excluded.fruit;
insert into insertconflicttest values (2, 'Orange') on conflict (key, key, key) do update set fruit = excluded.fruit;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 6eb2e8c..3005761 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2778,16 +2778,19 @@ CREATE TABLE hats (
hat_color char(10) -- hat color
);
CREATE TABLE hat_data (
- hat_name char(10) primary key,
+ hat_name char(10),
hat_color char(10) -- hat color
);
+create unique index hat_data_unique_idx
+ on hat_data (hat_name COLLATE "C" bpchar_pattern_ops);
-- okay
CREATE RULE hat_nosert AS ON INSERT TO hats
DO INSTEAD
INSERT INTO hat_data VALUES (
NEW.hat_name,
NEW.hat_color)
- ON CONFLICT (hat_name) DO NOTHING RETURNING *;
+ ON CONFLICT (hat_name COLLATE "C" bpchar_pattern_ops) WHERE hat_color = 'green'
+ DO NOTHING RETURNING *;
-- Works (projects row)
INSERT INTO hats VALUES ('h7', 'black') RETURNING *;
hat_name | hat_color
@@ -2803,12 +2806,13 @@ INSERT INTO hats VALUES ('h7', 'black') RETURNING *;
SELECT tablename, rulename, definition FROM pg_rules
WHERE tablename = 'hats';
- tablename | rulename | definition
------------+------------+------------------------------------------------------------------------------
- hats | hat_nosert | CREATE RULE hat_nosert AS +
- | | ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color)+
- | | VALUES (new.hat_name, new.hat_color) ON CONFLICT DO NOTHING +
- | | RETURNING hat_data.hat_name, +
+ tablename | rulename | definition
+-----------+------------+---------------------------------------------------------------------------------------------
+ hats | hat_nosert | CREATE RULE hat_nosert AS +
+ | | ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) +
+ | | VALUES (new.hat_name, new.hat_color) ON CONFLICT(hat_name COLLATE "C" bpchar_pattern_ops)+
+ | | WHERE (hat_data.hat_color = 'green'::bpchar) DO NOTHING +
+ | | RETURNING hat_data.hat_name, +
| | hat_data.hat_color;
(1 row)
@@ -2861,13 +2865,13 @@ SELECT * FROM hat_data WHERE hat_name = 'h8';
SELECT tablename, rulename, definition FROM pg_rules
WHERE tablename = 'hats';
- tablename | rulename | definition
------------+------------+-------------------------------------------------------------------------------------------------------------------------------
- hats | hat_upsert | CREATE RULE hat_upsert AS +
- | | ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) +
- | | VALUES (new.hat_name, new.hat_color) ON CONFLICT DO UPDATE SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color+
- | | WHERE (excluded.hat_color <> 'forbidden'::bpchar) +
- | | RETURNING hat_data.hat_name, +
+ tablename | rulename | definition
+-----------+------------+-----------------------------------------------------------------------------------------------------------------------------------------
+ hats | hat_upsert | CREATE RULE hat_upsert AS +
+ | | ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) +
+ | | VALUES (new.hat_name, new.hat_color) ON CONFLICT(hat_name) DO UPDATE SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color+
+ | | WHERE (excluded.hat_color <> 'forbidden'::bpchar) +
+ | | RETURNING hat_data.hat_name, +
| | hat_data.hat_color;
(1 row)
@@ -2877,7 +2881,7 @@ explain (costs off) INSERT INTO hats VALUES ('h8', 'forbidden') RETURNING *;
----------------------------------------------------------------
Insert on hat_data
Conflict Resolution: UPDATE
- Conflict Arbiter Indexes: hat_data_pkey
+ Conflict Arbiter Indexes: hat_data_unique_idx
Conflict Filter: (excluded.hat_color <> 'forbidden'::bpchar)
-> Result
(5 rows)
@@ -2909,7 +2913,7 @@ RETURNING *;
----------------------------------------------------------------
Insert on hat_data
Conflict Resolution: UPDATE
- Conflict Arbiter Indexes: hat_data_pkey
+ Conflict Arbiter Indexes: hat_data_unique_idx
Conflict Filter: (excluded.hat_color <> 'forbidden'::bpchar)
CTE data
-> Values Scan on "*VALUES*"
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 1a81155..621adc0 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1049,9 +1049,11 @@ CREATE TABLE hats (
);
CREATE TABLE hat_data (
- hat_name char(10) primary key,
+ hat_name char(10),
hat_color char(10) -- hat color
);
+create unique index hat_data_unique_idx
+ on hat_data (hat_name COLLATE "C" bpchar_pattern_ops);
-- okay
CREATE RULE hat_nosert AS ON INSERT TO hats
@@ -1059,7 +1061,8 @@ CREATE RULE hat_nosert AS ON INSERT TO hats
INSERT INTO hat_data VALUES (
NEW.hat_name,
NEW.hat_color)
- ON CONFLICT (hat_name) DO NOTHING RETURNING *;
+ ON CONFLICT (hat_name COLLATE "C" bpchar_pattern_ops) WHERE hat_color = 'green'
+ DO NOTHING RETURNING *;
-- Works (projects row)
INSERT INTO hats VALUES ('h7', 'black') RETURNING *;
--
1.9.1
From c142dc590431da977757c7402a6443c9afd3548c Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <peter.geoghega...@gmail.com>
Date: Mon, 11 May 2015 15:37:54 -0700
Subject: [PATCH 1/2] Refactor parse analysis of ON CONFLICT inference
Defer lookup of the opfamily and input type of a user specified opclass
until the optimizer selects among available unique indexes.
---
contrib/pg_stat_statements/pg_stat_statements.c | 3 +--
src/backend/nodes/copyfuncs.c | 3 +--
src/backend/nodes/equalfuncs.c | 3 +--
src/backend/nodes/outfuncs.c | 3 +--
src/backend/nodes/readfuncs.c | 3 +--
src/backend/optimizer/util/plancat.c | 20 +++++++++++++++-----
src/backend/parser/parse_clause.c | 14 +++-----------
src/include/nodes/primnodes.h | 3 +--
8 files changed, 24 insertions(+), 28 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index c4d3ee5..3cc687b 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2645,8 +2645,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
InferenceElem *ie = (InferenceElem *) node;
APP_JUMB(ie->infercollid);
- APP_JUMB(ie->inferopfamily);
- APP_JUMB(ie->inferopcinputtype);
+ APP_JUMB(ie->inferopclass);
JumbleExpr(jstate, ie->expr);
}
break;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index fa7d286..94f79ef 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1838,8 +1838,7 @@ _copyInferenceElem(const InferenceElem *from)
COPY_NODE_FIELD(expr);
COPY_SCALAR_FIELD(infercollid);
- COPY_SCALAR_FIELD(inferopfamily);
- COPY_SCALAR_FIELD(inferopcinputtype);
+ COPY_SCALAR_FIELD(inferopclass);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index d7928a9..f19251e 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -702,8 +702,7 @@ _equalInferenceElem(const InferenceElem *a, const InferenceElem *b)
{
COMPARE_NODE_FIELD(expr);
COMPARE_SCALAR_FIELD(infercollid);
- COMPARE_SCALAR_FIELD(inferopfamily);
- COMPARE_SCALAR_FIELD(inferopcinputtype);
+ COMPARE_SCALAR_FIELD(inferopclass);
return true;
}
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 66fee3e..c47a637 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1473,8 +1473,7 @@ _outInferenceElem(StringInfo str, const InferenceElem *node)
WRITE_NODE_FIELD(expr);
WRITE_OID_FIELD(infercollid);
- WRITE_OID_FIELD(inferopfamily);
- WRITE_OID_FIELD(inferopcinputtype);
+ WRITE_OID_FIELD(inferopclass);
}
static void
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 6fd9d46..f5a40fb 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1214,8 +1214,7 @@ _readInferenceElem(void)
READ_NODE_FIELD(expr);
READ_OID_FIELD(infercollid);
- READ_OID_FIELD(inferopfamily);
- READ_OID_FIELD(inferopcinputtype);
+ READ_OID_FIELD(inferopclass);
READ_DONE();
}
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index b425680..cbb4776 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -633,7 +633,7 @@ infer_arbiter_indexes(PlannerInfo *root)
* index definition.
*/
if (elem->infercollid != InvalidOid ||
- elem->inferopfamily != InvalidOid ||
+ elem->inferopclass != InvalidOid ||
list_member(idxExprs, elem->expr))
continue;
@@ -709,23 +709,33 @@ infer_collation_opclass_match(InferenceElem *elem, Relation idxRel,
Bitmapset *inferAttrs, List *idxExprs)
{
AttrNumber natt;
+ Oid inferopfamily = InvalidOid; /* OID of att opfamily */
+ Oid inferopcinputtype = InvalidOid; /* OID of att opfamily */
/*
* If inference specification element lacks collation/opclass, then no
* need to check for exact match.
*/
- if (elem->infercollid == InvalidOid && elem->inferopfamily == InvalidOid)
+ if (elem->infercollid == InvalidOid && elem->inferopclass == InvalidOid)
return true;
+ /*
+ * Lookup opfamily and input type, for matching indexes
+ */
+ if (elem->inferopclass)
+ {
+ inferopfamily = get_opclass_family(elem->inferopclass);
+ inferopcinputtype = get_opclass_input_type(elem->inferopclass);
+ }
+
for (natt = 1; natt <= idxRel->rd_att->natts; natt++)
{
Oid opfamily = idxRel->rd_opfamily[natt - 1];
Oid opcinputtype = idxRel->rd_opcintype[natt - 1];
Oid collation = idxRel->rd_indcollation[natt - 1];
- if (elem->inferopfamily != InvalidOid &&
- (elem->inferopfamily != opfamily ||
- elem->inferopcinputtype != opcinputtype))
+ if (elem->inferopclass != InvalidOid &&
+ (inferopfamily != opfamily || inferopcinputtype != opcinputtype))
{
/* Attribute needed to match opclass, but didn't */
continue;
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index a90bcf4..c8af5ab 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -2730,18 +2730,10 @@ resolve_unique_index_expr(ParseState *pstate, InferClause *infer,
exprLocation(pInfer->expr));
if (!ielem->opclass)
- {
- pInfer->inferopfamily = InvalidOid;
- pInfer->inferopcinputtype = InvalidOid;
- }
+ pInfer->inferopclass = InvalidOid;
else
- {
- Oid opclass = get_opclass_oid(BTREE_AM_OID, ielem->opclass,
- false);
-
- pInfer->inferopfamily = get_opclass_family(opclass);
- pInfer->inferopcinputtype = get_opclass_input_type(opclass);
- }
+ pInfer->inferopclass = get_opclass_oid(BTREE_AM_OID,
+ ielem->opclass, false);
result = lappend(result, pInfer);
}
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a5467c5..9f3a726 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1198,8 +1198,7 @@ typedef struct InferenceElem
Expr xpr;
Node *expr; /* expression to infer from, or NULL */
Oid infercollid; /* OID of collation, or InvalidOid */
- Oid inferopfamily; /* OID of att opfamily, or InvalidOid */
- Oid inferopcinputtype; /* OID of att input type, or InvalidOid */
+ Oid inferopclass; /* OID of att opclass, or InvalidOid */
} InferenceElem;
/*--------------------
--
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
