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
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) partition by range (a, b);
+create table pt1 (b int not null, v varchar, a int not null) partition by
range (b);
create table pt11 (like pt1);
alter table pt11 drop a;
alter table pt11 add a int;
@@ -2412,18 +2412,19 @@ select table_name, column_name, is_updatable
------------+-------------+--------------
ptv | a | YES
ptv | b | YES
-(2 rows)
+ ptv | v | YES
+(3 rows)
insert into ptv values (1, 2);
select tableoid::regclass, * from pt;
- tableoid | a | b
-----------+---+---
- pt11 | 1 | 2
+ tableoid | a | b | v
+----------+---+---+---
+ pt11 | 1 | 2 |
(1 row)
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).
+DETAIL: Failing row contains (1, 2, null).
drop view ptv, ptv_wco;
drop table pt, pt1, pt11;
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 7666a7d6ca..89e503bff8 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -263,6 +263,13 @@ with ins (a, b, c) as
(insert into mlparted (b, a) select s.a, 1 from generate_series(2, 39) s(a)
returning tableoid::regclass, *)
select a, b, min(c), max(c) from ins group by a, b order by 1;
+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');
+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/sql/updatable_views.sql
b/src/test/regress/sql/updatable_views.sql
index 6a9958d38b..2ede44c02b 100644
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1114,8 +1114,8 @@ 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) partition by range (a, b);
+create table pt1 (b int not null, v varchar, a int not null) partition by
range (b);
create table pt11 (like pt1);
alter table pt11 drop a;
alter table pt11 add a int;
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
