Thread: [HACKERS] Oddity in error handling of constraint violation in ExecConstraintsfor partitioned tables

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

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

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

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

On Fri, Jul 07, 2017 at 06:47:26PM +0900, Amit Langote wrote:
> On 2017/07/06 16:06, Etsuro Fujita wrote:
> > 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.

> The patch keeps tests that you added in your patch.  Added this to the
> open items list.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

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

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

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

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

On Sun, Jul 9, 2017 at 3:06 AM, Noah Misch <noah@leadboat.com> wrote:
> On Fri, Jul 07, 2017 at 06:47:26PM +0900, Amit Langote wrote:
>> On 2017/07/06 16:06, Etsuro Fujita wrote:
>> > 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.
>
>> The patch keeps tests that you added in your patch.  Added this to the
>> open items list.
>
> [Action required within three days.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 10 open item.  Robert,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

The posted patches look OK to me.  Barring developments, I will commit
them on 2017-07-17, or send another update by then.

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

On Mon, Jul 10, 2017 at 5:44 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> The posted patches look OK to me.  Barring developments, I will commit
> them on 2017-07-17, or send another update by then.

Committed them.

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

On 2017/07/18 11:03, Robert Haas wrote:
> On Mon, Jul 10, 2017 at 5:44 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> The posted patches look OK to me.  Barring developments, I will commit
>> them on 2017-07-17, or send another update by then.
> 
> Committed them.

Thank you!

Best regards,
Etsuro Fujita

On 2017/07/18 16:20, Etsuro Fujita wrote:
> On 2017/07/18 11:03, Robert Haas wrote:
>> On Mon, Jul 10, 2017 at 5:44 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> The posted patches look OK to me.  Barring developments, I will commit
>>> them on 2017-07-17, or send another update by then.
>>
>> Committed them.
> 
> Thank you!

Thank you both.

Regards,
Amit

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

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

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

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

On 24 July 2017 at 12:11, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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

On 2017/07/24 16:16, Amit Khandekar wrote:
> On 24 July 2017 at 12:11, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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

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 <Langote_Amit_f8@lab.ntt.co.jp>
>> 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

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

On Mon, Jul 24, 2017 at 6:21 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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