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

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