On 2018/04/10 11:56, Amit Langote wrote:
> On 2018/03/27 13:27, Amit Langote wrote:
>> On 2018/03/26 23:20, Alvaro Herrera wrote:
>>> The one thing I wasn't terribly in love with is the four calls to
>>> map_partition_varattnos(), creating the attribute map four times ... but
>>> we already have it in the TupleConversionMap, no? Looks like we could
>>> save a bunch of work there.
>>
>> Hmm, actually we can't use that map, assuming you're talking about the
>> following map:
>>
>> TupleConversionMap *map = proute->parent_child_tupconv_maps[partidx];
>>
>> We can only use that to tell if we need converting expressions (as we
>> currently do), but it cannot be used to actually convert the expressions.
>> The map in question is for use by do_convert_tuple(), not to map varattnos
>> in Vars using map_variable_attnos().
>>
>> But it's definitely a bit undesirable to have various
>> map_partition_varattnos() calls within ExecInitPartitionInfo() to
>> initialize the same information (the map) multiple times.
>
> I will try to think of doing something about this later this week.
The solution I came up with is to call map_variable_attnos() directly,
instead of going through map_partition_varattnos() every time, after first
creating the attribute map ourselves.
>>> And a final item is: can we have a whole-row expression in the clauses?
>>> We currently don't handle those either, not even to throw an error.
>>> [figures a test case] ... and now that I test it, it does crash!
>>>
>>> create table part (a int primary key, b text) partition by range (a);
>>> create table part1 (b text, a int not null);
>>> alter table part attach partition part1 for values from (1) to (1000);
>>> insert into part values (1, 'two') on conflict (a)
>>> do update set b = format('%s (was %s)', excluded.b, part.b)
>>> where part.* *<> (1, text 'two');
>>>
>>> I think this means we should definitely handle found_whole_row. (If you
>>> create part1 in the normal way, it works as you'd expect.)
>>
>> I agree. That means we simply remove the Assert after the
>> map_partition_varattnos call.
>>
>>> I'm going to close a few other things first, then come back to this.
>>
>> Attached find a patch to fix the whole-row expression issue. I added your
>> test to insert_conflict.sql.
Combined the above patch into the attached patch.
Thanks,
Amit
From a90decd69a42bebdb6e07c8268686c0500f8c48e Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Mon, 16 Apr 2018 17:31:42 +0900
Subject: [PATCH v2] Couple of fixes for ExecInitPartitionInfo
First, avoid repeated calling of map_partition_varattnos. To do that,
generate the rootrel -> partrel attribute conversion map ourselves
just once and call map_variable_attnos() directly with it.
Second, support conversion of whole-row variables that appear in
ON CONFLICT expressions. Add relevant test.
---
src/backend/executor/execPartition.c | 88 ++++++++++++++++++++-------
src/test/regress/expected/insert_conflict.out | 16 +++++
src/test/regress/sql/insert_conflict.sql | 14 +++++
3 files changed, 97 insertions(+), 21 deletions(-)
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index 218645d43b..1727e111bb 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"
@@ -309,6 +310,8 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
partrel;
ResultRelInfo *leaf_part_rri;
MemoryContext oldContext;
+ AttrNumber *part_attnos = NULL;
+ bool found_whole_row;
/*
* We locked all the partitions in ExecSetupPartitionTupleRouting
@@ -397,8 +400,19 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
/*
* Convert Vars in it to contain this partition's attribute
numbers.
*/
- wcoList = map_partition_varattnos(wcoList, firstVarno,
-
partrel, firstResultRel, NULL);
+ 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));
@@ -446,9 +460,20 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
/*
* Convert Vars in it to contain this partition's attribute
numbers.
*/
- returningList = map_partition_varattnos(returningList,
firstVarno,
-
partrel, firstResultRel,
-
NULL);
+ 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;
/*
@@ -549,14 +574,27 @@ 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);
+ 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);
@@ -587,14 +625,22 @@ 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);
+ 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..66ca1839bc 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, 'cuarenta', 1);
+insert into parted_conflict values (50, 'cuarenta', 2)
+ on conflict (a, b) do update set (a, b, c) = row(excluded.*)
+ where parted_conflict = (50, text 'cuarenta', 1) and
+ excluded = (50, text 'cuarenta', 2);
+-- should see (50, 'cuarenta', 2)
+select * from parted_conflict order by a;
+ a | b | c
+----+----------+---
+ 50 | cuarenta | 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..fb30530a54 100644
--- a/src/test/regress/sql/insert_conflict.sql
+++ b/src/test/regress/sql/insert_conflict.sql
@@ -558,4 +558,18 @@ create table parted_conflict_1_1 partition of
parted_conflict_1 for values from
insert into parted_conflict values (40, 'forty');
insert into parted_conflict_1 values (40, 'cuarenta')
on conflict (a) do update set b = excluded.b;
+
+-- 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, 'cuarenta', 1);
+insert into parted_conflict values (50, 'cuarenta', 2)
+ on conflict (a, b) do update set (a, b, c) = row(excluded.*)
+ where parted_conflict = (50, text 'cuarenta', 1) and
+ excluded = (50, text 'cuarenta', 2);
+
+-- should see (50, 'cuarenta', 2)
+select * from parted_conflict order by a;
+
drop table parted_conflict;
--
2.11.0
