From f2703383d3ddff50b4325f5686011b20a486aa02 Mon Sep 17 00:00:00 2001 From: amit Date: Mon, 31 Jul 2017 17:48:07 +0900 Subject: [PATCH 2/2] Teach map_variable_attnos_mutator to convert whole-row Vars Partitioning code that uses it requires it. --- src/backend/catalog/partition.c | 5 ++- src/backend/commands/tablecmds.c | 4 +-- src/backend/executor/nodeModifyTable.c | 10 ++---- src/backend/parser/parse_utilcmd.c | 6 ++-- src/backend/rewrite/rewriteManip.c | 50 ++++++++++++++++++++------- src/include/rewrite/rewriteManip.h | 2 +- src/include/utils/rel.h | 7 ++++ src/test/regress/expected/insert.out | 10 ++++++ src/test/regress/expected/updatable_views.out | 20 +++++++++-- src/test/regress/sql/insert.sql | 6 ++++ src/test/regress/sql/updatable_views.sql | 22 ++++++++++-- 11 files changed, 111 insertions(+), 31 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 5891af9cf2..889d9635bc 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -909,6 +909,7 @@ map_partition_varattnos(List *expr, int target_varno, { AttrNumber *part_attnos; bool my_found_whole_row; + Oid to_rowtype; if (expr == NIL) return NIL; @@ -916,11 +917,13 @@ map_partition_varattnos(List *expr, int target_varno, part_attnos = convert_tuples_by_name_map(RelationGetDescr(partrel), RelationGetDescr(parent), gettext_noop("could not convert row type")); + + to_rowtype = RelationGetRelType(partrel); expr = (List *) map_variable_attnos((Node *) expr, target_varno, 0, part_attnos, RelationGetDescr(parent)->natts, - &my_found_whole_row); + &my_found_whole_row, to_rowtype); if (found_whole_row) *found_whole_row = my_found_whole_row; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index cc5d3d6faf..86cefe4696 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -1989,7 +1989,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence, expr = map_variable_attnos(stringToNode(check[i].ccbin), 1, 0, newattno, tupleDesc->natts, - &found_whole_row); + &found_whole_row, InvalidOid); /* * For the moment we have to reject whole-row variables. We @@ -8874,7 +8874,7 @@ ATPrepAlterColumnType(List **wqueue, map_variable_attnos(def->cooked_default, 1, 0, attmap, RelationGetDescr(rel)->natts, - &found_whole_row); + &found_whole_row, InvalidOid); if (found_whole_row) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 361241a6ce..435aed3b8b 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1993,10 +1993,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) List *wcoExprs = NIL; ListCell *ll; - /* - * We are passing node->nominalRelation as the varno. wcoList - * might contain whole-row vars which is legal. - */ + /* varno = node->nominalRelation */ mapped_wcoList = map_partition_varattnos(wcoList, node->nominalRelation, partrel, rel, NULL); @@ -2069,10 +2066,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) Relation partrel = resultRelInfo->ri_RelationDesc; List *rlist; - /* - * We are passing node->nominalRelation as the varno. - * returningList might contain wholerow vars which is legal. - */ + /* varno = node->nominalRelation */ rlist = map_partition_varattnos(returningList, node->nominalRelation, partrel, rel, NULL); diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 9f37f1b920..f2f0ea540b 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -1107,7 +1107,7 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla ccbin_node = map_variable_attnos(stringToNode(ccbin), 1, 0, attmap, tupleDesc->natts, - &found_whole_row); + &found_whole_row, InvalidOid); /* * We reject whole-row variables because the whole point of LIKE @@ -1463,7 +1463,7 @@ generateClonedIndexStmt(CreateStmtContext *cxt, Relation source_idx, indexkey = map_variable_attnos(indexkey, 1, 0, attmap, attmap_length, - &found_whole_row); + &found_whole_row, InvalidOid); /* As in transformTableLikeClause, reject whole-row variables */ if (found_whole_row) @@ -1539,7 +1539,7 @@ generateClonedIndexStmt(CreateStmtContext *cxt, Relation source_idx, pred_tree = map_variable_attnos(pred_tree, 1, 0, attmap, attmap_length, - &found_whole_row); + &found_whole_row, InvalidOid); /* As in transformTableLikeClause, reject whole-row variables */ if (found_whole_row) diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index b89b435da0..bd3129b7cd 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -1202,15 +1202,18 @@ replace_rte_variables_mutator(Node *node, * A zero in the mapping array represents a dropped column, which should not * appear in the expression. * - * If the expression tree contains a whole-row Var for the target RTE, - * the Var is not changed but *found_whole_row is returned as TRUE. - * For most callers this is an error condition, but we leave it to the caller - * to report the error so that useful context can be provided. (In some - * usages it would be appropriate to modify the Var's vartype and insert a - * ConvertRowtypeExpr node to map back to the original vartype. We might - * someday extend this function's API to support that. For now, the only - * concession to that future need is that this function is a tree mutator - * not just a walker.) + * Depending on the caller, whole-row Vars in the expression tree are + * converted if the necessary information is provided (see below.) If the + * caller cannot provide the information, it might mean that finding a + * whole-row Var in the first place is an error. In that case, we let the + * caller know that a whole-row Var was found by returning *found_whole_row + * as TRUE, which the caller then can report as an error by providing useful + * context information. + * + * When the information necessary to convert whole-row Vars is present, we + * substitute the Var's vartype (replace by to_rowtype) and add a + * ConvertRowtypeExpr node to map back to the original vartype. Currently, + * only map_partition_varattnos() requests conversion of whole-row Vars. * * This could be built using replace_rte_variables and a callback function, * but since we don't ever need to insert sublinks, replace_rte_variables is @@ -1224,6 +1227,8 @@ typedef struct const AttrNumber *attno_map; /* map array for user attnos */ int map_length; /* number of entries in attno_map[] */ bool *found_whole_row; /* output flag */ + /* Target type when converting whole-row vars */ + Oid to_rowtype; } map_variable_attnos_context; static Node * @@ -1240,10 +1245,9 @@ map_variable_attnos_mutator(Node *node, var->varlevelsup == context->sublevels_up) { /* Found a matching variable, make the substitution */ - Var *newvar = (Var *) palloc(sizeof(Var)); + Var *newvar = copyObject(var); int attno = var->varattno; - *newvar = *var; if (attno > 0) { /* user-defined column, replace attno */ @@ -1257,6 +1261,27 @@ map_variable_attnos_mutator(Node *node, { /* whole-row variable, warn caller */ *(context->found_whole_row) = true; + + /* If the callers expects us to convert the same, do so. */ + if (OidIsValid(context->to_rowtype)) + { + ConvertRowtypeExpr *r; + + /* Var itself is converted to the requested rowtype. */ + newvar->vartype = context->to_rowtype; + + /* + * And a conversion step on top to convert back to the + * original type. + */ + r = makeNode(ConvertRowtypeExpr); + r->arg = (Expr *) newvar; + r->resulttype = var->vartype; + r->convertformat = COERCE_IMPLICIT_CAST; + r->location = -1; + + return (Node *) r; + } } return (Node *) newvar; } @@ -1283,7 +1308,7 @@ Node * map_variable_attnos(Node *node, int target_varno, int sublevels_up, const AttrNumber *attno_map, int map_length, - bool *found_whole_row) + bool *found_whole_row, Oid to_rowtype) { map_variable_attnos_context context; @@ -1291,6 +1316,7 @@ map_variable_attnos(Node *node, context.sublevels_up = sublevels_up; context.attno_map = attno_map; context.map_length = map_length; + context.to_rowtype = to_rowtype; context.found_whole_row = found_whole_row; *found_whole_row = false; diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h index 282ff9967f..56f9bb8013 100644 --- a/src/include/rewrite/rewriteManip.h +++ b/src/include/rewrite/rewriteManip.h @@ -72,7 +72,7 @@ extern Node *replace_rte_variables_mutator(Node *node, extern Node *map_variable_attnos(Node *node, int target_varno, int sublevels_up, const AttrNumber *attno_map, int map_length, - bool *found_whole_row); + bool *found_whole_row, Oid to_rowtype); extern Node *ReplaceVarsFromTargetList(Node *node, int target_varno, int sublevels_up, diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 4bc61e5380..bd5428f72f 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -444,6 +444,13 @@ typedef struct ViewOptions ((relation)->rd_rel->relnamespace) /* + * RelationGetRelType + * Returns the rel's pg_type OID. + */ +#define RelationGetRelType(relation) \ + ((relation)->rd_rel->reltype) + +/* * RelationIsMapped * True if the relation uses the relfilenode map. * diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index b642de39d5..38b364d10f 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -668,4 +668,14 @@ insert into returningwrtest values (1) returning returningwrtest; (1) (1 row) +alter table returningwrtest add b text; +create table returningwrtest2 (b text, c int, a int); +alter table returningwrtest2 drop c; +alter table returningwrtest attach partition returningwrtest2 for values in (2); +insert into returningwrtest values (2, 'foo') returning returningwrtest; + returningwrtest +----------------- + (2,foo) +(1 row) + drop table returningwrtest; diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out index 51a21f10c2..2090a411fe 100644 --- a/src/test/regress/expected/updatable_views.out +++ b/src/test/regress/expected/updatable_views.out @@ -2436,5 +2436,21 @@ create view wcowrtest_v as select * from wcowrtest where wcowrtest = '(2)'::wcow insert into wcowrtest_v values (1); ERROR: new row violates check option for view "wcowrtest_v" DETAIL: Failing row contains (1). -drop view wcowrtest_v; -drop table wcowrtest; +alter table wcowrtest add b text; +create table wcowrtest2 (b text, c int, a int); +alter table wcowrtest2 drop c; +alter table wcowrtest attach partition wcowrtest2 for values in (2); +create table sometable (a int, b text); +insert into sometable values (1, 'a'), (2, 'b'); +create view wcowrtest_v2 as + select * + from wcowrtest r + where r in (select s from sometable s where r.a = s.a) +with check option; +-- WITH CHECK qual will be processed with wcowrtest2's +-- rowtype after tuple-routing +insert into wcowrtest_v2 values (2, 'no such row in sometable'); +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; diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index 2eff832e59..f51d72484d 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -404,4 +404,10 @@ drop table mcrparted; create table returningwrtest (a int) partition by list (a); create table returningwrtest1 partition of returningwrtest for values in (1); insert into returningwrtest values (1) returning returningwrtest; + +alter table returningwrtest add b text; +create table returningwrtest2 (b text, c int, a int); +alter table returningwrtest2 drop c; +alter table returningwrtest attach partition returningwrtest2 for values in (2); +insert into returningwrtest values (2, 'foo') returning returningwrtest; drop table returningwrtest; diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql index af8499a019..a6ba5aad9e 100644 --- a/src/test/regress/sql/updatable_views.sql +++ b/src/test/regress/sql/updatable_views.sql @@ -1148,5 +1148,23 @@ create table wcowrtest (a int) partition by list (a); create table wcowrtest1 partition of wcowrtest for values in (1); create view wcowrtest_v as select * from wcowrtest where wcowrtest = '(2)'::wcowrtest with check option; insert into wcowrtest_v values (1); -drop view wcowrtest_v; -drop table wcowrtest; + +alter table wcowrtest add b text; +create table wcowrtest2 (b text, c int, a int); +alter table wcowrtest2 drop c; +alter table wcowrtest attach partition wcowrtest2 for values in (2); + +create table sometable (a int, b text); +insert into sometable values (1, 'a'), (2, 'b'); +create view wcowrtest_v2 as + select * + from wcowrtest r + where r in (select s from sometable s where r.a = s.a) +with check option; + +-- WITH CHECK qual will be processed with wcowrtest2's +-- rowtype after tuple-routing +insert into wcowrtest_v2 values (2, 'no such row in sometable'); + +drop view wcowrtest_v, wcowrtest_v2; +drop table wcowrtest, sometable; -- 2.11.0