Fujita-san,
On 2017/07/10 14:15, Etsuro Fujita wrote:
> On 2017/07/07 18:47, Amit Langote wrote:
>> On 2017/07/06 16:06, Etsuro Fujita wrote:
>>> I think this should be fixed. Attached is a patch for that.
>
>> How about setting ri_RangeTableIndex of the partition ResultRelInfo
>> correctly in the first place? If you look at the way InitResultRelInfo()
>> is called in ExecSetupPartitionTupleRouting(), a dummy RT index of 1 is
>> passed. We could instead pass the correct one. I think
>> ModifyTable.nominalRelation is that (at least in the INSERT case.
>>
>> The attached patch implements that. It modifies
>> ExecSetupPartitionTupleRouting to accept one more argument resultRTindex
>> and passes along the same to InitResultRelInfo(). Later when
>> ExecConstraints() wants to build the modifiedCols set, it looks up the
>> correct RTE using the partition ResultRelInfo, which has the appropriate
>> ri_RangeTableIndex set and uses the same.
>
> Looks good to me.
Thanks for the review.
>> The patch keeps tests that you added in your patch. Added this to the
>> open items list.
>
> Another thing I noticed is the error handling in ExecWithCheckOptions; it
> doesn't take any care for partition tables, so the column description in
> the error message for WCO_VIEW_CHECK is built using the partition table's
> rowtype, not the root table's. But I think it'd be better to build that
> using the root table's rowtype, like ExecConstraints, because that would
> make the column description easier to understand since the parent view(s)
> (from which WithCheckOptions evaluated there are created) would have been
> defined on the root table. This seems independent from the above issue,
> so I created a separate patch, which I'm attaching. What do you think
> about that?
Good catch. The patch looks spot on to me. To keep both the patches
together, I'm attaching them here as 0001 which fixes the original issue
you reported on this thread and 0002 which is your patch to fix error
reporting in ExecWithCheckOptions.
Thanks,
Amit
From f9f4cc93d962ff433fe98b36a84953ba4048a725 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 7 Jul 2017 17:24:44 +0900
Subject: [PATCH 1/2] Properly set ri_RangeTableIndex of partition result rels
Previously it was set to a "dummy" value of 1, which would cause
certain code, such as ExecConstraint()'s error handling code, to
look at the unintended range table entry. Instead set it to the
index of the RT entry corresponding to root partitioned table.
---
src/backend/commands/copy.c | 1 +
src/backend/executor/execMain.c | 3 ++-
src/backend/executor/nodeModifyTable.c | 1 +
src/include/executor/executor.h | 1 +
src/test/regress/expected/insert.out | 18 ++++++++++++++++++
src/test/regress/sql/insert.sql | 18 ++++++++++++++++++
6 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f391828e74..51693791cc 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1433,6 +1433,7 @@ BeginCopy(ParseState *pstate,
num_partitions;
ExecSetupPartitionTupleRouting(rel,
+
1,
&partition_dispatch_info,
&partitions,
&partition_tupconv_maps,
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 0f08283f81..c36b5b7392 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -3213,6 +3213,7 @@ EvalPlanQualEnd(EPQState *epqstate)
*/
void
ExecSetupPartitionTupleRouting(Relation rel,
+ Index resultRTindex,
PartitionDispatch
**pd,
ResultRelInfo
**partitions,
TupleConversionMap
***tup_conv_maps,
@@ -3271,7 +3272,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
InitResultRelInfo(leaf_part_rri,
partrel,
- 1, /* dummy */
+ resultRTindex, /*
dummy */
rel,
0);
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index 8d17425abe..77ba15dd90 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1914,6 +1914,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
num_partitions;
ExecSetupPartitionTupleRouting(rel,
+
node->nominalRelation,
&partition_dispatch_info,
&partitions,
&partition_tupconv_maps,
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index e25cfa3aba..59c28b709e 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -207,6 +207,7 @@ extern void EvalPlanQualSetTuple(EPQState *epqstate, Index
rti,
HeapTuple tuple);
extern HeapTuple EvalPlanQualGetTuple(EPQState *epqstate, Index rti);
extern void ExecSetupPartitionTupleRouting(Relation rel,
+ Index resultRTindex,
PartitionDispatch
**pd,
ResultRelInfo
**partitions,
TupleConversionMap
***tup_conv_maps,
diff --git a/src/test/regress/expected/insert.out
b/src/test/regress/expected/insert.out
index d1153f410b..dd5dddb20c 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -506,5 +506,23 @@ DETAIL: Failing row contains (2, hi there).
insert into brtrigpartcon1 values (1, 'hi there');
ERROR: new row for relation "brtrigpartcon1" violates partition constraint
DETAIL: Failing row contains (2, hi there).
+-- check that the message shows the appropriate column description in a
+-- situation where the partitioned table is not the primary ModifyTable node
+create table inserttest3 (f1 text default 'foo', f2 text default 'bar', f3
int);
+create role regress_coldesc_role;
+grant insert on inserttest3 to regress_coldesc_role;
+grant insert on brtrigpartcon to regress_coldesc_role;
+revoke select on brtrigpartcon from regress_coldesc_role;
+set role regress_coldesc_role;
+with result as (insert into brtrigpartcon values (1, 'hi there') returning 1)
+ insert into inserttest3 (f3) select * from result;
+ERROR: new row for relation "brtrigpartcon1" violates partition constraint
+DETAIL: Failing row contains (a, b) = (2, hi there).
+reset role;
+-- cleanup
+revoke all on inserttest3 from regress_coldesc_role;
+revoke all on brtrigpartcon from regress_coldesc_role;
+drop role regress_coldesc_role;
+drop table inserttest3;
drop table brtrigpartcon;
drop function brtrigpartcon1trigf();
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 83c3ad8f53..fe63020768 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -340,5 +340,23 @@ create or replace function brtrigpartcon1trigf() returns
trigger as $$begin new.
create trigger brtrigpartcon1trig before insert on brtrigpartcon1 for each row
execute procedure brtrigpartcon1trigf();
insert into brtrigpartcon values (1, 'hi there');
insert into brtrigpartcon1 values (1, 'hi there');
+
+-- check that the message shows the appropriate column description in a
+-- situation where the partitioned table is not the primary ModifyTable node
+create table inserttest3 (f1 text default 'foo', f2 text default 'bar', f3
int);
+create role regress_coldesc_role;
+grant insert on inserttest3 to regress_coldesc_role;
+grant insert on brtrigpartcon to regress_coldesc_role;
+revoke select on brtrigpartcon from regress_coldesc_role;
+set role regress_coldesc_role;
+with result as (insert into brtrigpartcon values (1, 'hi there') returning 1)
+ insert into inserttest3 (f3) select * from result;
+reset role;
+
+-- cleanup
+revoke all on inserttest3 from regress_coldesc_role;
+revoke all on brtrigpartcon from regress_coldesc_role;
+drop role regress_coldesc_role;
+drop table inserttest3;
drop table brtrigpartcon;
drop function brtrigpartcon1trigf();
--
2.11.0
From cb919ba074f7cf20019aeef17f381d7f0cfe4a6a Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 10 Jul 2017 15:26:58 +0900
Subject: [PATCH 2/2] Correctly format the row shown in WITH CHECK OPTION error
message
Author: Etsuro Fujita
---
src/backend/executor/execMain.c | 19 +++++++++++++++++++
src/test/regress/expected/updatable_views.out | 2 +-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c36b5b7392..d7bfb939b1 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -2097,6 +2097,25 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo
*resultRelInfo,
* USING policy.
*/
case WCO_VIEW_CHECK:
+ /* See the comment in
ExecConstraints(). */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ HeapTuple tuple =
ExecFetchSlotTuple(slot);
+ TupleDesc old_tupdesc =
RelationGetDescr(rel);
+ TupleConversionMap *map;
+
+ rel =
resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ /* a reverse map */
+ map =
convert_tuples_by_name(old_tupdesc, tupdesc,
+
gettext_noop("could not convert row type"));
+ if (map != NULL)
+ {
+ tuple =
do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple,
slot, InvalidBuffer, false);
+ }
+ }
+
insertedCols =
GetInsertedColumns(resultRelInfo, estate);
updatedCols =
GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols,
updatedCols);
diff --git a/src/test/regress/expected/updatable_views.out
b/src/test/regress/expected/updatable_views.out
index 971dddd01c..caca81a70b 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2424,6 +2424,6 @@ select tableoid::regclass, * from pt;
create view ptv_wco as select * from pt where a = 0 with check option;
insert into ptv_wco values (1, 2);
ERROR: new row violates check option for view "ptv_wco"
-DETAIL: Failing row contains (2, 1).
+DETAIL: Failing row contains (1, 2).
drop view ptv, ptv_wco;
drop table pt, pt1, pt11;
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
