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

Reply via email to