From 3359921fa13977226f8d1a51645181d3ff1f277a Mon Sep 17 00:00:00 2001 From: amit Date: Fri, 22 Jun 2018 17:03:16 +0900 Subject: [PATCH v2] Fix INSERT ON CONFLICT DO UPDATE on updatable views Rewriter forgot to change resnos of the entries in ON CONFLICT related target lists to refer to the attribute numbers of a view's underlying base relation instead of the view's own. Fix that because that's the relation that the executor will try to apply DO UPDATE actions to. --- src/backend/access/common/tupconvert.c | 6 +- src/backend/catalog/partition.c | 3 +- src/backend/catalog/pg_constraint.c | 3 +- src/backend/commands/indexcmds.c | 3 +- src/backend/commands/tablecmds.c | 12 ++- src/backend/executor/execPartition.c | 9 +- src/backend/parser/analyze.c | 121 +++++++++++++---------- src/backend/rewrite/rewriteHandler.c | 136 +++++++++++++++++++++++++- src/include/access/tupconvert.h | 2 +- src/include/parser/analyze.h | 2 + src/test/regress/expected/updatable_views.out | 32 ++++++ src/test/regress/sql/updatable_views.sql | 20 ++++ 12 files changed, 282 insertions(+), 67 deletions(-) diff --git a/src/backend/access/common/tupconvert.c b/src/backend/access/common/tupconvert.c index 3bc67b846d..4498810d2a 100644 --- a/src/backend/access/common/tupconvert.c +++ b/src/backend/access/common/tupconvert.c @@ -218,7 +218,7 @@ convert_tuples_by_name(TupleDesc indesc, bool same; /* Verify compatibility and prepare attribute-number map */ - attrMap = convert_tuples_by_name_map(indesc, outdesc, msg); + attrMap = convert_tuples_by_name_map(indesc, outdesc, msg, false); /* * Check to see if the map is one-to-one, in which case we need not do a @@ -292,7 +292,7 @@ convert_tuples_by_name(TupleDesc indesc, AttrNumber * convert_tuples_by_name_map(TupleDesc indesc, TupleDesc outdesc, - const char *msg) + const char *msg, bool missing_ok) { AttrNumber *attrMap; int outnatts; @@ -355,7 +355,7 @@ convert_tuples_by_name_map(TupleDesc indesc, break; } } - if (attrMap[i] == 0) + if (attrMap[i] == 0 && !missing_ok) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg_internal("%s", _(msg)), diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 558022647c..a8b65bdab9 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -177,7 +177,8 @@ map_partition_varattnos(List *expr, int fromrel_varno, part_attnos = convert_tuples_by_name_map(RelationGetDescr(to_rel), RelationGetDescr(from_rel), - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); expr = (List *) map_variable_attnos((Node *) expr, fromrel_varno, 0, part_attnos, diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 7a6d158f89..d9c4d6a656 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -438,7 +438,8 @@ CloneForeignKeyConstraints(Oid parentId, Oid relationId, List **cloned) */ attmap = convert_tuples_by_name_map(RelationGetDescr(rel), RelationGetDescr(parentRel), - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); ScanKeyInit(&key, Anum_pg_constraint_conrelid, BTEqualStrategyNumber, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index b9dad9672e..b1d0a62cb8 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -914,7 +914,8 @@ DefineIndex(Oid relationId, attmap = convert_tuples_by_name_map(RelationGetDescr(childrel), parentDesc, - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); maplen = parentDesc->natts; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index eb2d33dd86..b169e37e8b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -938,7 +938,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, attmap = convert_tuples_by_name_map(RelationGetDescr(rel), RelationGetDescr(parent), - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); idxstmt = generateClonedIndexStmt(NULL, RelationGetRelid(rel), idxRel, attmap, RelationGetDescr(rel)->natts, @@ -9263,7 +9264,8 @@ ATPrepAlterColumnType(List **wqueue, attmap = convert_tuples_by_name_map(RelationGetDescr(childrel), RelationGetDescr(rel), - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); ((ColumnDef *) cmd->def)->cooked_default = map_variable_attnos(def->cooked_default, 1, 0, @@ -14413,7 +14415,8 @@ AttachPartitionEnsureIndexes(Relation rel, Relation attachrel) info = BuildIndexInfo(idxRel); attmap = convert_tuples_by_name_map(RelationGetDescr(attachrel), RelationGetDescr(rel), - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); constraintOid = get_relation_idx_constraint_oid(RelationGetRelid(rel), idx); /* @@ -14890,7 +14893,8 @@ ATExecAttachPartitionIdx(List **wqueue, Relation parentIdx, RangeVar *name) parentInfo = BuildIndexInfo(parentIdx); attmap = convert_tuples_by_name_map(RelationGetDescr(partTbl), RelationGetDescr(parentTbl), - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); if (!CompareIndexInfo(childInfo, parentInfo, partIdx->rd_indcollation, parentIdx->rd_indcollation, diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index d13be4145f..e875163bac 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -427,7 +427,8 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, part_attnos = convert_tuples_by_name_map(RelationGetDescr(partrel), RelationGetDescr(firstResultRel), - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); wcoList = (List *) map_variable_attnos((Node *) wcoList, firstVarno, 0, @@ -487,7 +488,8 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, part_attnos = convert_tuples_by_name_map(RelationGetDescr(partrel), RelationGetDescr(firstResultRel), - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); returningList = (List *) map_variable_attnos((Node *) returningList, firstVarno, 0, @@ -603,7 +605,8 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, part_attnos = convert_tuples_by_name_map(RelationGetDescr(partrel), RelationGetDescr(firstResultRel), - gettext_noop("could not convert row type")); + gettext_noop("could not convert row type"), + false); onconflset = (List *) map_variable_attnos((Node *) onconflset, INNER_VAR, 0, diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 05f57591e4..8e3ea6ef88 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1022,9 +1022,6 @@ transformOnConflictClause(ParseState *pstate, if (onConflictClause->action == ONCONFLICT_UPDATE) { Relation targetrel = pstate->p_target_relation; - Var *var; - TargetEntry *te; - int attno; /* * All INSERT expressions have been parsed, get ready for potentially @@ -1044,55 +1041,8 @@ transformOnConflictClause(ParseState *pstate, exclRte->relkind = RELKIND_COMPOSITE_TYPE; exclRelIndex = list_length(pstate->p_rtable); - /* - * Build a targetlist representing the columns of the EXCLUDED pseudo - * relation. Have to be careful to use resnos that correspond to - * attnos of the underlying relation. - */ - for (attno = 0; attno < RelationGetNumberOfAttributes(targetrel); attno++) - { - Form_pg_attribute attr = TupleDescAttr(targetrel->rd_att, attno); - char *name; - - if (attr->attisdropped) - { - /* - * can't use atttypid here, but it doesn't really matter what - * type the Const claims to be. - */ - var = (Var *) makeNullConst(INT4OID, -1, InvalidOid); - name = ""; - } - else - { - var = makeVar(exclRelIndex, attno + 1, - attr->atttypid, attr->atttypmod, - attr->attcollation, - 0); - name = pstrdup(NameStr(attr->attname)); - } - - te = makeTargetEntry((Expr *) var, - attno + 1, - name, - false); - - /* don't require select access yet */ - exclRelTlist = lappend(exclRelTlist, te); - } - - /* - * Add a whole-row-Var entry to support references to "EXCLUDED.*". - * Like the other entries in exclRelTlist, its resno must match the - * Var's varattno, else the wrong things happen while resolving - * references in setrefs.c. This is against normal conventions for - * targetlists, but it's okay since we don't use this as a real tlist. - */ - var = makeVar(exclRelIndex, InvalidAttrNumber, - targetrel->rd_rel->reltype, - -1, InvalidOid, 0); - te = makeTargetEntry((Expr *) var, InvalidAttrNumber, NULL, true); - exclRelTlist = lappend(exclRelTlist, te); + exclRelTlist = BuildOnConflictExcludedTargetlist(targetrel, + exclRelIndex); /* * Add EXCLUDED and the target RTE to the namespace, so that they can @@ -1125,6 +1075,73 @@ transformOnConflictClause(ParseState *pstate, return result; } +/* + * CreateOnConflictExcludedTargetlist + * Create the target list of EXCLUDED pseudo-relation of ON CONFLICT + * + * Note: Exported for use in the rewriter. + */ +List * +BuildOnConflictExcludedTargetlist(Relation targetrel, + Index exclRelIndex) +{ + List *result = NIL; + int attno; + Var *var; + TargetEntry *te; + + /* + * Build a targetlist representing the columns of the EXCLUDED pseudo + * relation. Have to be careful to use resnos that correspond to + * attnos of the underlying relation. + */ + for (attno = 0; attno < RelationGetNumberOfAttributes(targetrel); attno++) + { + Form_pg_attribute attr = TupleDescAttr(targetrel->rd_att, attno); + char *name; + + if (attr->attisdropped) + { + /* + * can't use atttypid here, but it doesn't really matter what + * type the Const claims to be. + */ + var = (Var *) makeNullConst(INT4OID, -1, InvalidOid); + name = ""; + } + else + { + var = makeVar(exclRelIndex, attno + 1, + attr->atttypid, attr->atttypmod, + attr->attcollation, + 0); + name = pstrdup(NameStr(attr->attname)); + } + + te = makeTargetEntry((Expr *) var, + attno + 1, + name, + false); + + /* don't require select access yet */ + result = lappend(result, te); + } + + /* + * Add a whole-row-Var entry to support references to "EXCLUDED.*". + * Like the other entries in exclRelTlist, its resno must match the + * Var's varattno, else the wrong things happen while resolving + * references in setrefs.c. This is against normal conventions for + * targetlists, but it's okay since we don't use this as a real tlist. + */ + var = makeVar(exclRelIndex, InvalidAttrNumber, + targetrel->rd_rel->reltype, + -1, InvalidOid, 0); + te = makeTargetEntry((Expr *) var, InvalidAttrNumber, NULL, true); + result = lappend(result, te); + + return result; +} /* * count_rowexpr_columns - diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 5b87c554f5..06db9003cd 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -29,6 +29,7 @@ #include "nodes/nodeFuncs.h" #include "parser/analyze.h" #include "parser/parse_coerce.h" +#include "parser/parse_relation.h" #include "parser/parsetree.h" #include "rewrite/rewriteDefine.h" #include "rewrite/rewriteHandler.h" @@ -80,7 +81,9 @@ static List *matchLocks(CmdType event, RuleLock *rulelocks, static Query *fireRIRrules(Query *parsetree, List *activeRIRs); static bool view_has_instead_trigger(Relation view, CmdType event); static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist); - +static void CheckOnConflictVars(OnConflictExpr *onConflict, + Bitmapset *orig_varnos, + Relation view, Relation base_rel); /* * AcquireRewriteLocks - @@ -3035,6 +3038,86 @@ rewriteTargetView(Query *parsetree, Relation view) } /* + * Modify expressions in OnConflictExpr that would be affected by + * target rel substitution. + */ + if (parsetree->onConflict && + parsetree->onConflict->action == ONCONFLICT_UPDATE) + { + Index old_exclRelIndex, + new_exclRelIndex; + RangeTblEntry *new_exclRte; + AttrNumber *attrMap; + bool found_whole_row; + + /* + * First modify the DO UPDATE SET targetlist like we did above for the + * main targetlist. + */ + foreach(lc, parsetree->onConflict->onConflictSet) + { + TargetEntry *tle = (TargetEntry *) lfirst(lc); + TargetEntry *view_tle; + + if (tle->resjunk) + continue; + + view_tle = get_tle_by_resno(view_targetlist, tle->resno); + if (view_tle != NULL && !view_tle->resjunk && IsA(view_tle->expr, Var)) + tle->resno = ((Var *) view_tle->expr)->varattno; + else + elog(ERROR, "attribute number %d not found in view targetlist", + tle->resno); + } + + /* + * Modify EXCLUDED pseudo-relation to be based on the base relation + * instead of the view relation as it originally would be. + */ + old_exclRelIndex = parsetree->onConflict->exclRelIndex; + + /* + * Create a new RTE for EXCLUDED pseudo-relation based on base_rel. + * This closely mimics the code in transformOnConflictClause. + */ + new_exclRte = addRangeTableEntryForRelation(make_parsestate(NULL), + base_rel, + makeAlias("excluded", + NIL), + false, false); + new_exclRte->relkind = RELKIND_COMPOSITE_TYPE; + parsetree->rtable = lappend(parsetree->rtable, new_exclRte); + new_exclRelIndex = parsetree->onConflict->exclRelIndex = + list_length(parsetree->rtable); + + /* Change exclRelTlist into one containing base_rel's attrs. */ + list_free(parsetree->onConflict->exclRelTlist); + parsetree->onConflict->exclRelTlist = + BuildOnConflictExcludedTargetlist(base_rel, new_exclRelIndex); + + /* Check that none of EXCLUDED references contains view's columns. */ + CheckOnConflictVars(parsetree->onConflict, + bms_make_singleton(old_exclRelIndex), + view, base_rel); + + /* Convert Vars to use the new EXCLUDED pseudo relation. */ + attrMap = convert_tuples_by_name_map(RelationGetDescr(base_rel), + RelationGetDescr(view), + gettext_noop("could not convert row type"), + true); + parsetree->onConflict = (OnConflictExpr *) + map_variable_attnos((Node *) parsetree->onConflict, + old_exclRelIndex, 0, + attrMap, RelationGetDescr(view)->natts, + RelationGetForm(view)->reltype, + &found_whole_row); + ChangeVarNodes((Node *) parsetree->onConflict, + old_exclRelIndex, + new_exclRelIndex, + 0); + } + + /* * For UPDATE/DELETE, pull up any WHERE quals from the view. We know that * any Vars in the quals must reference the one base relation, so we need * only adjust their varnos to reference the new target (just the same as @@ -3164,6 +3247,57 @@ rewriteTargetView(Query *parsetree, Relation view) return parsetree; } +typedef struct +{ + Bitmapset *orig_varnos; + Relation view; + Relation base_rel; +} CheckOnConflictVars_context; + +static bool +CheckOnConflictVars_walker(Node *node, CheckOnConflictVars_context *context) +{ + if (node == NULL) + return false; + + if (IsA(node, Var) && + bms_is_member(((Var *) node)->varno, context->orig_varnos)) + { + AttrNumber attno = ((Var *) node)->varattno; + TupleDesc orig_tupdesc = RelationGetDescr(context->view); + TupleDesc new_tupdesc = RelationGetDescr(context->base_rel); + char *attname = orig_tupdesc->attrs[attno - 1].attname.data; + + for (attno = 1; attno <= new_tupdesc->natts; attno++) + { + Form_pg_attribute att = &new_tupdesc->attrs[attno - 1]; + + if (strcmp(attname, att->attname.data) == 0) + return false; + } + + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot access column \"%s\" of view \"%s\" in this part of query", + attname, RelationGetRelationName(context->view)), + errdetail("Only columns present in base relation can be used in ON CONFLICT specification"))); + } + + return expression_tree_walker(node, CheckOnConflictVars_walker, context); +} + +static void +CheckOnConflictVars(OnConflictExpr *onConflict, Bitmapset *orig_varnos, + Relation view, Relation base_rel) +{ + CheckOnConflictVars_context context; + + context.orig_varnos = orig_varnos; + context.view = view; + context.base_rel = base_rel; + + CheckOnConflictVars_walker((Node *) onConflict, &context); +} /* * RewriteQuery - diff --git a/src/include/access/tupconvert.h b/src/include/access/tupconvert.h index 66c0ed0882..a382a23adc 100644 --- a/src/include/access/tupconvert.h +++ b/src/include/access/tupconvert.h @@ -40,7 +40,7 @@ extern TupleConversionMap *convert_tuples_by_name(TupleDesc indesc, extern AttrNumber *convert_tuples_by_name_map(TupleDesc indesc, TupleDesc outdesc, - const char *msg); + const char *msg, bool missing_ok); extern HeapTuple do_convert_tuple(HeapTuple tuple, TupleConversionMap *map); diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.h index 687ae1b5b7..31b6cd4db6 100644 --- a/src/include/parser/analyze.h +++ b/src/include/parser/analyze.h @@ -42,5 +42,7 @@ extern void CheckSelectLocking(Query *qry, LockClauseStrength strength); extern void applyLockingClause(Query *qry, Index rtindex, LockClauseStrength strength, LockWaitPolicy waitPolicy, bool pushedDown); +extern List *BuildOnConflictExcludedTargetlist(Relation targetrel, + Index exclRelIndex); #endif /* ANALYZE_H */ diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out index b34bab4b29..d128b45e68 100644 --- a/src/test/regress/expected/updatable_views.out +++ b/src/test/regress/expected/updatable_views.out @@ -2578,3 +2578,35 @@ ERROR: new row violates check option for view "wcowrtest_v2" DETAIL: Failing row contains (2, no such row in sometable). drop view wcowrtest_v, wcowrtest_v2; drop table wcowrtest, sometable; +-- Check INSERT .. ON CONFLICT DO UPDATE works correctly when the view's +-- columns may be ordered differently than the underlying table's. +create table uv_iocu_tab (a text unique, b float); +insert into uv_iocu_tab values ('xyxyxy', 1); +-- note the different order of columns in the view +create view uv_iocu_view as select b, b+1 as c, a from uv_iocu_tab; +insert into uv_iocu_view (a, b) values ('xyxyxy', 2) on conflict (a) do update set b = uv_iocu_view.b; +-- error on trying to access view's column that's not present in underlying +-- base relation in the ON CONFLICT portion of the query +insert into uv_iocu_view (a, b) values ('xyxyxy', 3) on conflict (a) do update set b = excluded.b where excluded.c > 0; +ERROR: cannot access column "c" of view "uv_iocu_view" in this part of query +DETAIL: Only columns present in base relation can be used in ON CONFLICT specification +explain (costs off) insert into uv_iocu_view (a, b) values ('xyxyxy', 3) on conflict (a) do update set b = excluded.b where excluded.b > 0; + QUERY PLAN +--------------------------------------------------------- + Insert on uv_iocu_tab + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: uv_iocu_tab_a_key + Conflict Filter: (excluded.b > '0'::double precision) + -> Result +(5 rows) + +insert into uv_iocu_view (a, b) values ('xyxyxy', 3) on conflict (a) do update set b = excluded.b where excluded.b > 0; +-- should display 'xyxyxy, 3' +select * from uv_iocu_tab; + a | b +--------+--- + xyxyxy | 3 +(1 row) + +drop view uv_iocu_view; +drop table uv_iocu_tab; diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql index a7786b26e9..8dc0c6a8cc 100644 --- a/src/test/regress/sql/updatable_views.sql +++ b/src/test/regress/sql/updatable_views.sql @@ -1244,3 +1244,23 @@ insert into wcowrtest_v2 values (2, 'no such row in sometable'); drop view wcowrtest_v, wcowrtest_v2; drop table wcowrtest, sometable; + +-- Check INSERT .. ON CONFLICT DO UPDATE works correctly when the view's +-- columns may be ordered differently than the underlying table's. +create table uv_iocu_tab (a text unique, b float); +insert into uv_iocu_tab values ('xyxyxy', 1); +-- note the different order of columns in the view +create view uv_iocu_view as select b, b+1 as c, a from uv_iocu_tab; +insert into uv_iocu_view (a, b) values ('xyxyxy', 2) on conflict (a) do update set b = uv_iocu_view.b; + +-- error on trying to access view's column that's not present in underlying +-- base relation in the ON CONFLICT portion of the query +insert into uv_iocu_view (a, b) values ('xyxyxy', 3) on conflict (a) do update set b = excluded.b where excluded.c > 0; + +explain (costs off) insert into uv_iocu_view (a, b) values ('xyxyxy', 3) on conflict (a) do update set b = excluded.b where excluded.b > 0; + +insert into uv_iocu_view (a, b) values ('xyxyxy', 3) on conflict (a) do update set b = excluded.b where excluded.b > 0; +-- should display 'xyxyxy, 3' +select * from uv_iocu_tab; +drop view uv_iocu_view; +drop table uv_iocu_tab; -- 2.11.0