Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Robert Haas]

On Mon, Jul 24, 2017 at 6:21 AM, Amit Langote
 wrote:
> Yes, we need that there too.
>
> Done that in the attached v3 (including the test where
> ExecPartitionCheck() would have crashed without the patch).

Committed.  Thanks to all of you.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Langote]

On 2017/07/24 17:30, Etsuro Fujita wrote:
> On 2017/07/24 16:16, Amit Khandekar wrote:
>> On 24 July 2017 at 12:11, Amit Langote 
>> wrote:
>>> Attached is the updated version of your patch.
> 
> Good catch, Amit K. and Amit L.!
> 
>> Now that this is done, any particular reason it is not done in
>> ExecPartitionCheck() ? I see that there is a do_convert_tuple() in
>> that function, again without changing the slot descriptor.
> 
> Yeah, I think we would need that in ExecPartitionCheck() as well.

Yes, we need that there too.

Done that in the attached v3 (including the test where
ExecPartitionCheck() would have crashed without the patch).

Thanks,
Amit
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index b22de78516..78cbcd1a32 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1879,6 +1879,7 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, 
TupleTableSlot *slot,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, map);
+   ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, slot, InvalidBuffer, 
false);
}
}
@@ -1956,6 +1957,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, 
map);
+   ExecSetSlotDescriptor(slot, 
tupdesc);
ExecStoreTuple(tuple, slot, 
InvalidBuffer, false);
}
}
@@ -2003,6 +2005,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, map);
+   ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, slot, 
InvalidBuffer, false);
}
}
@@ -2112,6 +2115,7 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo 
*resultRelInfo,
if (map != NULL)
{
tuple = 
do_convert_tuple(tuple, map);
+   
ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, 
slot, InvalidBuffer, false);
}
}
diff --git a/src/test/regress/expected/insert.out 
b/src/test/regress/expected/insert.out
index c608ce377f..0dcc86fef4 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -410,6 +410,21 @@ with ins (a, b, c) as
  mlparted4  | 1 |  30 |  39
 (5 rows)
 
+alter table mlparted add c text;
+create table mlparted5 (c text, a int not null, b int not null) partition by 
list (c);
+create table mlparted5a (a int not null, c text, b int not null);
+alter table mlparted5 attach partition mlparted5a for values in ('a');
+alter table mlparted attach partition mlparted5 for values from (1, 40) to (1, 
50);
+alter table mlparted add constraint check_b check (a = 1 and b < 45);
+insert into mlparted values (1, 45, 'a');
+ERROR:  new row for relation "mlparted5a" violates check constraint "check_b"
+DETAIL:  Failing row contains (1, 45, a).
+create function mlparted5abrtrig_func() returns trigger as $$ begin new.c = 
'b'; return new; end; $$ language plpgsql;
+create trigger mlparted5abrtrig before insert on mlparted5a for each row 
execute procedure mlparted5abrtrig_func();
+insert into mlparted5 (a, b, c) values (1, 40, 'a');
+ERROR:  new row for relation "mlparted5a" violates partition constraint
+DETAIL:  Failing row contains (b, 1, 40).
+drop table mlparted5;
 -- check that message shown after failure to find a partition shows the
 -- appropriate key description (or none) in various situations
 create table key_desc (a int, b int) partition by list ((a+0));
diff --git a/src/test/regress/expected/updatable_views.out 
b/src/test/regress/expected/updatable_views.out
index caca81a70b..eab5c0334c 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2368,8 +2368,8 @@ DETAIL:  Failing row contains (-1, invalid).
 DROP VIEW v1;
 DROP TABLE t1;
 -- check that an auto-updatable view on a partitioned table works correctly
-create table pt (a int, b int) partition by range (a, b);
-create table pt1 (b int not null, a int not null) partition by range (b);
+create table pt (a int, b int, v varchar) 

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Etsuro Fujita]

On 2017/07/24 16:16, Amit Khandekar wrote:

On 24 July 2017 at 12:11, Amit Langote  wrote:

Attached is the updated version of your patch.


Good catch, Amit K. and Amit L.!


Now that this is done, any particular reason it is not done in
ExecPartitionCheck() ? I see that there is a do_convert_tuple() in
that function, again without changing the slot descriptor.


Yeah, I think we would need that in ExecPartitionCheck() as well.

Best regards,
Etsuro Fujita



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Khandekar]

On 24 July 2017 at 12:11, Amit Langote  wrote:
> Hi Amit,
>
> On 2017/07/24 14:09, Amit Khandekar wrote:
 On 2017/07/10 14:15, Etsuro Fujita wrote:
 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?
>>
>> + if (map != NULL)
>> + {
>> + tuple = do_convert_tuple(tuple, map);
>> + ExecStoreTuple(tuple, slot, InvalidBuffer, false);
>> + }
>>
>> Above, the tuple descriptor also needs to be set, since the parent and
>> child partitions can have different column ordering.
>>
>> Due to this, the following testcase crashes :
>>
>> CREATE TABLE range_parted (a text,b int, c int) partition by range (b);
>> CREATE VIEW upview AS SELECT * FROM range_parted WHERE (select c >
>> 120) WITH CHECK OPTION;
>> create table part_a_1_a_10(b int, c int, a text);
>> alter table range_parted attach partition part_a_1_a_10 for values
>> from (1) to (10);
>> insert into upview values ('a', 2, 100);
>
> Good catch.  Thanks for creating the patch.
>
> There are other places with similar code viz. those in ExecConstraints()
> that would need fixing.

Ah ok. I should have noticed that. Thanks.

> Attached is the updated version of your patch.
Now that this is done, any particular reason it is not done in
ExecPartitionCheck() ? I see that there is a do_convert_tuple() in
that function, again without changing the slot descriptor.


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Langote]

Hi Amit,

On 2017/07/24 14:09, Amit Khandekar wrote:
>>> On 2017/07/10 14:15, Etsuro Fujita wrote:
>>> 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?
> 
> + if (map != NULL)
> + {
> + tuple = do_convert_tuple(tuple, map);
> + ExecStoreTuple(tuple, slot, InvalidBuffer, false);
> + }
> 
> Above, the tuple descriptor also needs to be set, since the parent and
> child partitions can have different column ordering.
> 
> Due to this, the following testcase crashes :
> 
> CREATE TABLE range_parted (a text,b int, c int) partition by range (b);
> CREATE VIEW upview AS SELECT * FROM range_parted WHERE (select c >
> 120) WITH CHECK OPTION;
> create table part_a_1_a_10(b int, c int, a text);
> alter table range_parted attach partition part_a_1_a_10 for values
> from (1) to (10);
> insert into upview values ('a', 2, 100);

Good catch.  Thanks for creating the patch.

There are other places with similar code viz. those in ExecConstraints()
that would need fixing.  Without the same, the following causes a crash
(building on your example):

alter table range_parted add constraint check_c check (c > 120);
insert into range_parted values ('a', 2, 100);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Attached is the updated version of your patch.  A test is also added in
insert.sql on lines of the above example.

Will add this to the PG 10 open items list.

Thanks,
Amit
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index b22de78516..040e9a916a 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1956,6 +1956,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, 
map);
+   ExecSetSlotDescriptor(slot, 
tupdesc);
ExecStoreTuple(tuple, slot, 
InvalidBuffer, false);
}
}
@@ -2003,6 +2004,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, map);
+   ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, slot, 
InvalidBuffer, false);
}
}
@@ -2112,6 +2114,7 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo 
*resultRelInfo,
if (map != NULL)
{
tuple = 
do_convert_tuple(tuple, map);
+   
ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, 
slot, InvalidBuffer, false);
}
}
diff --git a/src/test/regress/expected/insert.out 
b/src/test/regress/expected/insert.out
index c608ce377f..0eaa47fb2b 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -410,6 +410,14 @@ with ins (a, b, c) as
  mlparted4  | 1 |  30 |  39
 (5 rows)
 
+alter table mlparted add c text;
+create table mlparted5 (c text, a int not null, b int not null);
+alter table mlparted attach partition mlparted5 for values from (1, 40) to (1, 
50);
+alter table mlparted add constraint check_b check (a = 1 and b < 45);
+insert into mlparted values (1, 45, 'bah');
+ERROR:  new row for relation "mlparted5" violates check constraint "check_b"
+DETAIL:  Failing row contains (1, 45, bah).
+drop table mlparted5;
 -- check that message shown after failure to find a partition shows the
 -- appropriate key description (or none) in various situations
 create table key_desc (a int, b int) partition by list ((a+0));
diff --git a/src/test/regress/expected/updatable_views.out 

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Khandekar]

>> On 2017/07/10 14:15, Etsuro Fujita wrote:
>> 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?

+ if (map != NULL)
+ {
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }

Above, the tuple descriptor also needs to be set, since the parent and
child partitions can have different column ordering.

Due to this, the following testcase crashes :

CREATE TABLE range_parted (a text,b int, c int) partition by range (b);
CREATE VIEW upview AS SELECT * FROM range_parted WHERE (select c >
120) WITH CHECK OPTION;
create table part_a_1_a_10(b int, c int, a text);
alter table range_parted attach partition part_a_1_a_10 for values
from (1) to (10);
insert into upview values ('a', 2, 100);

Attached is a patch that sets the tuple descriptor.
Also in the patch, in test updatable_view.sql, I have added a varchar
column in one of the partitioned tables used in updatable_views.sql,
so as to cover this scenario. Without setting the tuple descriptor,
the output of the patched updatable_views.sql  shows junk value in one
of the columns of the row in the error message emitted for the
WithCheckOption violation.

Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


set_slot_descriptor.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Langote]

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 
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,

   _dispatch_info,

   ,

   _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 @@ 

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Etsuro Fujita]

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.


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.


You are right.  Thank you for pointing that out.


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.


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?


Best regards,
Etsuro Fujita
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 2097,2102  ExecWithCheckOptions(WCOKind kind, ResultRelInfo 
*resultRelInfo,
--- 2097,2121 
 * 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);
*** a/src/test/regress/expected/updatable_views.out
--- b/src/test/regress/expected/updatable_views.out
***
*** 2424,2429  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).
  drop view ptv, ptv_wco;
  drop table pt, pt1, pt11;
--- 2424,2429 
  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 (1, 2).
  drop view ptv, ptv_wco;
  drop table pt, pt1, pt11;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Langote]

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,

   _dispatch_info,

   ,

   _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
@@ 

[HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Etsuro Fujita]

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 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.  I think this should be fixed.  Attached is a patch for that.


Best regards,
Etsuro Fujita
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 92,97  static bool ExecCheckRTEPermsModified(Oid relOid, Oid userid,
--- 92,99 
  Bitmapset *modifiedCols,
  AclMode requiredPerms);
  static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
+ static ResultRelInfo *ExecFindResultRelInfo(EState *estate, Oid relid,
+   
bool missing_ok);
  static char *ExecBuildSlotValueDescription(Oid reloid,
  TupleTableSlot *slot,
  TupleDesc tupdesc,
***
*** 1360,1365  InitResultRelInfo(ResultRelInfo *resultRelInfo,
--- 1362,1392 
  }
  
  /*
+  * ExecFindResultRelInfo -- find the ResultRelInfo struct for given relation 
OID
+  *
+  * If no such struct, either return NULL or throw error depending on 
missing_ok
+  */
+ static ResultRelInfo *
+ ExecFindResultRelInfo(EState *estate, Oid relid, bool missing_ok)
+ {
+   ResultRelInfo *rInfo;
+   int nr;
+ 
+   rInfo = estate->es_result_relations;
+   nr = estate->es_num_result_relations;
+   while (nr > 0)
+   {
+   if (RelationGetRelid(rInfo->ri_RelationDesc) == relid)
+   return rInfo;
+   rInfo++;
+   nr--;
+   }
+   if (!missing_ok)
+   elog(ERROR, "failed to find ResultRelInfo for relation %u", 
relid);
+   return NULL;
+ }
+ 
+ /*
   *ExecGetTriggerResultRel
   *
   * Get a ResultRelInfo for a trigger target relation.  Most of the time,
***
*** 1379,1399  ResultRelInfo *
  ExecGetTriggerResultRel(EState *estate, Oid relid)
  {
ResultRelInfo *rInfo;
-   int nr;
ListCell   *l;
Relationrel;
MemoryContext oldcontext;
  
/* First, search through the query result relations */
!   rInfo = estate->es_result_relations;
!   nr = estate->es_num_result_relations;
!   while (nr > 0)
!   {
!   if (RelationGetRelid(rInfo->ri_RelationDesc) == relid)
!   return rInfo;
!   rInfo++;
!   nr--;
!   }
/* Nope, but maybe we already made an extra ResultRelInfo for it */
foreach(l, estate->es_trig_target_relations)
{
--- 1406,1419 
  ExecGetTriggerResultRel(EState *estate, Oid relid)
  {
ResultRelInfo *rInfo;
ListCell   *l;
Relationrel;
MemoryContext oldcontext;
  
/* First, search through the query result relations */
!   rInfo = ExecFindResultRelInfo(estate, relid, true);
!   if (rInfo)
!   return rInfo;
/* Nope, but maybe we already made an extra ResultRelInfo for it */
foreach(l, estate->es_trig_target_relations)
{
***
*** 1828,1833  ExecPartitionCheck(ResultRelInfo *resultRelInfo, 
TupleTableSlot *slot,
--- 1848,1854 
   EState *estate)
  {
Relationrel =