Fujita-san,
On 2017/07/06 16:06, Etsuro Fujita wrote:
> Here is an example:
>
> postgres=# create table col_desc (a int, b int) partition by list (a);
> postgres=# create table col_desc_1 partition of col_desc for values in (1);
> postgres=# alter table col_desc_1 add check (b > 0);
> postgres=# create role col_desc_user;
> postgres=# grant insert on col_desc to col_desc_user;
> postgres=# revoke select on col_desc from col_desc_user;
> postgres=# set role col_desc_user;
> postgres=> insert into col_desc values (1, -1) returning 1;
> ERROR: new row for relation "col_desc_1" violates check constraint
> "col_desc_1_b_check"
> DETAIL: Failing row contains (a, b) = (1, -1).
>
> Looks good, but
>
> postgres=> reset role;
> postgres=# create table result (f1 text default 'foo', f2 text default
> 'bar', f3 int);
> postgres=# grant insert on result to col_desc_user;
> postgres=# set role col_desc_user;
> postgres=> with t as (insert into col_desc values (1, -1) returning 1)
> insert into result (f3) select * from t;
> ERROR: new row for relation "col_desc_1" violates check constraint
> "col_desc_1_b_check"
>
> Looks odd to me because the error message doesn't show any DETAIL info;
> since the CTE query, which produces the message, is the same as the above
> query, the message should also be the same as the one for the above
> query.
I agree that the DETAIL should be shown.
> I think the reason for that is: ExecConstraints looks at an
> incorrect resultRelInfo to build the message for a violating tuple that
> has been routed *in the case where the partitioned table isn't the primary
> ModifyTable node*, which leads to deriving an incorrect modifiedCols and
> then invoking ExecBuildSlotValueDescription with the wrong bitmap.
That's true. Choosing the appropriate ResultRelInfo will lead us to
looking at the correct RangeTblEntry (via ri_RangeTableIndex) and hence
get the desired modifiedCols bitmap. But...
> I think this should be fixed. Attached is a patch for that.
Looking up the ResultRelInfo using the proposed method in
ExecFindResultRelInfo() can be unreliable sometimes, that is, the method
of using the root table OID to pin down the RangeTableEntry to get the the
modified columns set. If the same table is a target in the main query
with different, the corresponding RTE will appear earlier in the
es_range_table list and will get returned. For example (slightly altered
version of the example in your email):
alter table col_desc add c int;
set session authorization col_desc_user ;
with t (a) as (insert into col_desc values (1, -1) returning 1)
insert into col_desc (c) select * from t;
ERROR: new row for relation "col_desc_1" violates check constraint
"col_desc_1_b_check"
DETAIL: Failing row contains (c) = (null).
Note that the patched code ends up matching the main query RTE of col_desc
instead of that in the CTE query. And the columns shown in the DETAIL
part reflects which RTE got used to compute the modifiedCols set.
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. With the patch:
with t (a) as (insert into col_desc values (1, -1) returning 1)
insert into col_desc (c) select * from t;
ERROR: new row for relation "col_desc_1" violates check constraint
"col_desc_1_b_check"
DETAIL: Failing row contains (a, b) = (1, -1).
The patch keeps tests that you added in your patch. Added this to the
open items list.
Thoughts?
Thanks,
Amit
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..df9302896c 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,
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();
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
