On Tue, May 12, 2015 at 3:18 PM, Peter Geoghegan <p...@heroku.com> wrote:
> 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.
>
> Sure.
Rebased version of patch is attached.
--
Peter Geoghegan
From cc2fcd8862e01880b1559f315aa3da23017edfe6 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] Further fixes to minor ON CONFLICT issues
Deparsing with an inference clause is now correctly supported. In
passing, re-factor InferenceElem representation of opclass, to defer
opfamily lookup for Relation index matching until index inference proper
(i.e., within the optimizer). This is done for the convenience of the
new ruleutils.c code, but independently make senses.
Finally, fix a few typos, and rename a variable -- "candidates" seemed
like a misnomer for the return value of infer_arbiter_indexes().
---
contrib/pg_stat_statements/pg_stat_statements.c | 3 +-
src/backend/executor/nodeModifyTable.c | 2 +-
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 | 34 +++++++----
src/backend/parser/parse_clause.c | 16 ++----
src/backend/utils/adt/ruleutils.c | 75 ++++++++++++++++++++++++-
src/include/nodes/primnodes.h | 3 +-
src/test/regress/expected/insert_conflict.out | 2 +-
src/test/regress/expected/rules.out | 38 +++++++------
src/test/regress/sql/rules.sql | 7 ++-
13 files changed, 133 insertions(+), 59 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6abe3f0..f97cc2c 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2637,8 +2637,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/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/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 76b63af..957c2bc 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1803,8 +1803,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 e032142..f26626e 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -687,8 +687,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 fe868b8..454d9ec 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1450,8 +1450,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 8136306..81b6243 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1141,8 +1141,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..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)
{
@@ -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;
@@ -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;
}
/*
@@ -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 73c505e..77a7a43 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -2258,18 +2258,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);
}
@@ -2301,7 +2293,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 903e80a..87fe580 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -5319,13 +5319,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);
@@ -7716,6 +7741,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/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index f10ae4e..51a65bd 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1159,8 +1159,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;
/*--------------------
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 cb18bb9..056dd8e 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2777,16 +2777,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
@@ -2802,12 +2805,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)
@@ -2860,13 +2864,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)
@@ -2876,7 +2880,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)
@@ -2908,7 +2912,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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
