From 158b7bc6d77948d2f474dc9f2777c87f81d1365a Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Mon, 16 Apr 2018 15:50:57 -0300 Subject: [PATCH] Ignore whole-rows in INSERT/CONFLICT with partitioned tables MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit We had an Assert() preventing whole-row expressions from being used in the SET clause of INSERT ON CONFLICT, but it seems unnecessary, given some tests, so remove it. Add a new test to exercise the case. Still at ExecInitPartitionInfo, we used map_partition_varattnos (which constructs an attribute map, then calls map_variable_attnos) using the same two relations many times in different expressions and with different parameters. Constructing the map over and over is a waste. To avoid this repeated work, construct the map once, and use map_variable_attnos() directly instead. Author: Amit Langote, per comments by me (Álvaro) Discussion: https://postgr.es/m/20180326142016.m4st5e34chrzrknk@alvherre.pgsql --- src/backend/executor/execPartition.c | 121 +++++++++++++----- src/test/regress/expected/insert_conflict.out | 16 +++ src/test/regress/sql/insert_conflict.sql | 18 +++ 3 files changed, 125 insertions(+), 30 deletions(-) diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 218645d43b..23a74bc3d9 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -24,6 +24,7 @@ #include "nodes/makefuncs.h" #include "partitioning/partbounds.h" #include "partitioning/partprune.h" +#include "rewrite/rewriteManip.h" #include "utils/lsyscache.h" #include "utils/partcache.h" #include "utils/rel.h" @@ -307,8 +308,12 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, ModifyTable *node = (ModifyTable *) mtstate->ps.plan; Relation rootrel = resultRelInfo->ri_RelationDesc, partrel; + Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc; ResultRelInfo *leaf_part_rri; MemoryContext oldContext; + AttrNumber *part_attnos = NULL; + bool found_whole_row; + bool equalTupdescs; /* * We locked all the partitions in ExecSetupPartitionTupleRouting @@ -356,6 +361,10 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, (node != NULL && node->onConflictAction != ONCONFLICT_NONE)); + /* if tuple descs are identical, we don't need to map the attrs */ + equalTupdescs = equalTupleDescs(RelationGetDescr(partrel), + RelationGetDescr(firstResultRel)); + /* * Build WITH CHECK OPTION constraints for the partition. Note that we * didn't build the withCheckOptionList for partitions within the planner, @@ -369,7 +378,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, List *wcoExprs = NIL; ListCell *ll; int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex; - Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc; /* * In the case of INSERT on a partitioned table, there is only one @@ -397,8 +405,22 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, /* * Convert Vars in it to contain this partition's attribute numbers. */ - wcoList = map_partition_varattnos(wcoList, firstVarno, - partrel, firstResultRel, NULL); + if (!equalTupdescs) + { + part_attnos = + convert_tuples_by_name_map(RelationGetDescr(partrel), + RelationGetDescr(firstResultRel), + gettext_noop("could not convert row type")); + wcoList = (List *) + map_variable_attnos((Node *) wcoList, + firstVarno, 0, + part_attnos, + RelationGetDescr(firstResultRel)->natts, + RelationGetForm(partrel)->reltype, + &found_whole_row); + /* We ignore the value of found_whole_row. */ + } + foreach(ll, wcoList) { WithCheckOption *wco = castNode(WithCheckOption, lfirst(ll)); @@ -425,7 +447,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, ExprContext *econtext; List *returningList; int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex; - Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc; /* See the comment above for WCO lists. */ Assert((node->operation == CMD_INSERT && @@ -443,12 +464,26 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, */ returningList = linitial(node->returningLists); - /* - * Convert Vars in it to contain this partition's attribute numbers. - */ - returningList = map_partition_varattnos(returningList, firstVarno, - partrel, firstResultRel, - NULL); + if (!equalTupdescs) + { + /* + * Convert Vars in it to contain this partition's attribute numbers. + */ + if (part_attnos == NULL) + part_attnos = + convert_tuples_by_name_map(RelationGetDescr(partrel), + RelationGetDescr(firstResultRel), + gettext_noop("could not convert row type")); + returningList = (List *) + map_variable_attnos((Node *) returningList, + firstVarno, 0, + part_attnos, + RelationGetDescr(firstResultRel)->natts, + RelationGetForm(partrel)->reltype, + &found_whole_row); + /* We ignore the value of found_whole_row. */ + } + leaf_part_rri->ri_returningList = returningList; /* @@ -473,7 +508,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, { TupleConversionMap *map = proute->parent_child_tupconv_maps[partidx]; int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex; - Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc; TupleDesc partrelDesc = RelationGetDescr(partrel); ExprContext *econtext = mtstate->ps.ps_ExprContext; ListCell *lc; @@ -549,17 +583,33 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, * target relation (firstVarno). */ onconflset = (List *) copyObject((Node *) node->onConflictSet); - onconflset = - map_partition_varattnos(onconflset, INNER_VAR, partrel, - firstResultRel, &found_whole_row); - Assert(!found_whole_row); - onconflset = - map_partition_varattnos(onconflset, firstVarno, partrel, - firstResultRel, &found_whole_row); - Assert(!found_whole_row); - - /* Finally, adjust this tlist to match the partition. */ - onconflset = adjust_partition_tlist(onconflset, map); + if (!equalTupdescs) + { + if (part_attnos == NULL) + part_attnos = + convert_tuples_by_name_map(RelationGetDescr(partrel), + RelationGetDescr(firstResultRel), + gettext_noop("could not convert row type")); + onconflset = (List *) + map_variable_attnos((Node *) onconflset, + INNER_VAR, 0, + part_attnos, + RelationGetDescr(firstResultRel)->natts, + RelationGetForm(partrel)->reltype, + &found_whole_row); + /* We ignore the value of found_whole_row. */ + onconflset = (List *) + map_variable_attnos((Node *) onconflset, + firstVarno, 0, + part_attnos, + RelationGetDescr(firstResultRel)->natts, + RelationGetForm(partrel)->reltype, + &found_whole_row); + /* We ignore the value of found_whole_row. */ + + /* Finally, adjust this tlist to match the partition. */ + onconflset = adjust_partition_tlist(onconflset, map); + } /* * Build UPDATE SET's projection info. The user of this @@ -587,14 +637,25 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, List *clause; clause = copyObject((List *) node->onConflictWhere); - clause = map_partition_varattnos(clause, INNER_VAR, - partrel, firstResultRel, - &found_whole_row); - Assert(!found_whole_row); - clause = map_partition_varattnos(clause, firstVarno, - partrel, firstResultRel, - &found_whole_row); - Assert(!found_whole_row); + if (!equalTupdescs) + { + clause = (List *) + map_variable_attnos((Node *) clause, + INNER_VAR, 0, + part_attnos, + RelationGetDescr(firstResultRel)->natts, + RelationGetForm(partrel)->reltype, + &found_whole_row); + /* We ignore the value of found_whole_row. */ + clause = (List *) + map_variable_attnos((Node *) clause, + firstVarno, 0, + part_attnos, + RelationGetDescr(firstResultRel)->natts, + RelationGetForm(partrel)->reltype, + &found_whole_row); + /* We ignore the value of found_whole_row. */ + } leaf_part_rri->ri_onConflict->oc_WhereClause = ExecInitQual((List *) clause, &mtstate->ps); } diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out index 2d7061fa1b..b27ad25d37 100644 --- a/src/test/regress/expected/insert_conflict.out +++ b/src/test/regress/expected/insert_conflict.out @@ -884,4 +884,20 @@ insert into parted_conflict values (40, 'forty'); insert into parted_conflict_1 values (40, 'cuarenta') on conflict (a) do update set b = excluded.b; ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification +-- test whole-row Vars in ON CONFLICT expressions +create unique index on parted_conflict (a, b); +alter table parted_conflict add c int; +truncate parted_conflict; +insert into parted_conflict values (50, 'cincuenta', 1); +insert into parted_conflict values (50, 'cincuenta', 2) + on conflict (a, b) do update set (a, b, c) = row(excluded.*) + where parted_conflict = (50, text 'cincuenta', 1) and + excluded = (50, text 'cincuenta', 2); +-- should see (50, 'cincuenta', 2) +select * from parted_conflict order by a; + a | b | c +----+-----------+--- + 50 | cincuenta | 2 +(1 row) + drop table parted_conflict; diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql index 6c50fd61eb..c677d70fb7 100644 --- a/src/test/regress/sql/insert_conflict.sql +++ b/src/test/regress/sql/insert_conflict.sql @@ -559,3 +559,21 @@ insert into parted_conflict values (40, 'forty'); insert into parted_conflict_1 values (40, 'cuarenta') on conflict (a) do update set b = excluded.b; drop table parted_conflict; + +-- test whole-row Vars in ON CONFLICT expressions +create table parted_conflict (a int, b text, c int) partition by range (a); +create table parted_conflict_1 (drp text, c int, a int, b text); +alter table parted_conflict_1 drop column drp; +create unique index on parted_conflict (a, b); +alter table parted_conflict attach partition parted_conflict_1 for values from (0) to (1000); +truncate parted_conflict; +insert into parted_conflict values (50, 'cincuenta', 1); +insert into parted_conflict values (50, 'cincuenta', 2) + on conflict (a, b) do update set (a, b, c) = row(excluded.*) + where parted_conflict = (50, text 'cincuenta', 1) and + excluded = (50, text 'cincuenta', 2); + +-- should see (50, 'cincuenta', 2) +select * from parted_conflict order by a; + +drop table parted_conflict; -- 2.40.0