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