Thread: [HACKERS] Rules on table partitions

Currently we allow rules to be defined on table partitions, but these
rules only fire when the partition is accessed directly, not when it
is accessed via the parent:

CREATE TABLE t1(a int, b int) PARTITION BY RANGE(a);
CREATE TABLE t1_p PARTITION OF t1 FOR VALUES FROM (1) TO (10);
INSERT INTO t1 VALUES (1,101), (2,201);

CREATE TABLE t1_p_log(a int, b int, d date);
CREATE RULE t1_p_log_rule AS ON UPDATE TO t1_p DO ALSO INSERT INTO t1_p_log VALUES(old.a, old.b, now());

UPDATE t1 SET b=b+1 WHERE a=1;
UPDATE t1_p SET b=b+1 WHERE a=2;

SELECT * FROM t1_p_log;
a |  b  |     d
---+-----+------------2 | 201 | 2017-06-19
(1 row)


I'd regard that as a bug, especially since this kind of thing would
have worked with old-style user-defined partitioning. Perhaps we
should explicitly forbid this for now -- i.e., raise a "not supported"
error when attempting to add a rule to a partition, or attach a table
with rules to a partitioned table.

Personally, I wouldn't regard adding proper support for rules on
partitions as a high priority, so I'd be OK with it remaining
unsupported unless someone cares enough to implement it, but that
seems preferable to leaving it partially working in this way.

Also, as things stand, it is possible to do the following:

CREATE TABLE t2(a int, b int) PARTITION BY RANGE(a);
CREATE TABLE t2_p PARTITION OF t2 FOR VALUES FROM (1) TO (10);
CREATE RULE "_RETURN" AS ON SELECT TO t2_p DO INSTEAD SELECT * FROM t2;

which results in the partition becoming a view that selects from the
parent, which surely ought to be forbidden.

Regards,
Dean

On 6/19/17 07:19, Dean Rasheed wrote:
> Currently we allow rules to be defined on table partitions, but these
> rules only fire when the partition is accessed directly, not when it
> is accessed via the parent

That's what I would have expected, but I realize that there are always
multiple ways to interpret things in this area.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Dean,

On 2017/06/19 20:19, Dean Rasheed wrote:
> Currently we allow rules to be defined on table partitions, but these
> rules only fire when the partition is accessed directly, not when it
> is accessed via the parent:

Yeah, the same thing as will happen with an inheritance setup, but I guess
you are asking whether that's what we want.

> CREATE TABLE t1(a int, b int) PARTITION BY RANGE(a);
> CREATE TABLE t1_p PARTITION OF t1 FOR VALUES FROM (1) TO (10);
> INSERT INTO t1 VALUES (1,101), (2,201);
> 
> CREATE TABLE t1_p_log(a int, b int, d date);
> CREATE RULE t1_p_log_rule AS ON UPDATE TO t1_p
>   DO ALSO INSERT INTO t1_p_log VALUES(old.a, old.b, now());
> 
> UPDATE t1 SET b=b+1 WHERE a=1;
> UPDATE t1_p SET b=b+1 WHERE a=2;
> 
> SELECT * FROM t1_p_log;
> 
>  a |  b  |     d
> ---+-----+------------
>  2 | 201 | 2017-06-19
> (1 row)
> 
> 
> I'd regard that as a bug, especially since this kind of thing would
> have worked with old-style user-defined partitioning.

With old-style inheritance based setup, you will see exactly the exact
same result:

create table p (a int, b int);
create table c () inherits (p);
create table c_log (a int, b int, c date);
create rule c_p_log_rule as on update to c
  do also insert into c_log values (old.a, old.b, now());
insert into p values (1, 100);
insert into c values (2, 200)

update p set b = b + 1 where a = 1;
select tableoid::regclass, * from p;
 tableoid | a |  b
----------+---+-----
 p        | 1 | 101
 c        | 2 | 200
(2 rows)

select * from c_log;
 a | b | c
---+---+---
(0 rows)

update p set b = b + 1 where a = 2;
select tableoid::regclass, * from p;
 tableoid | a |  b
----------+---+-----
 p        | 1 | 101
 c        | 2 | 201
(2 rows)

select * from c_log;
 a | b | c
---+---+---
(0 rows)

update c set b = b + 1 where a = 2;
select tableoid::regclass, * from p;
 tableoid | a |  b
----------+---+-----
 p        | 1 | 101
 c        | 2 | 202
(2 rows)

select * from c_log;
 a |  b  |     c
---+-----+------------
 2 | 201 | 2017-06-20
(1 row)

Note that inheritance is expanded in the planner, not the rewriter, so the
rules of partitions (that are added to the query later than the rewriter)
won't fire, AFAICS.  Same will apply to partitions.

> Perhaps we
> should explicitly forbid this for now -- i.e., raise a "not supported"
> error when attempting to add a rule to a partition, or attach a table
> with rules to a partitioned table.

We could do that, but an oft-raised question is how different we should
make new partitions from the old-style inheritance child tables?

Although a slightly different territory, you will also notice that
statement triggers of partitions won't be fired unless they are explicitly
named in the query, which is what happens for inheritance in general and
hence also for partitions.

> Personally, I wouldn't regard adding proper support for rules on
> partitions as a high priority, so I'd be OK with it remaining
> unsupported unless someone cares enough to implement it, but that
> seems preferable to leaving it partially working in this way.

Sure, if consensus turns out to be that we prohibit rules, statement
triggers, etc. that depend on the relation being explicitly named in the
query to be defined on partitions, I could draft up a patch for v10.

> Also, as things stand, it is possible to do the following:
> 
> CREATE TABLE t2(a int, b int) PARTITION BY RANGE(a);
> CREATE TABLE t2_p PARTITION OF t2 FOR VALUES FROM (1) TO (10);
> CREATE RULE "_RETURN" AS ON SELECT TO t2_p DO INSTEAD SELECT * FROM t2;
> 
> which results in the partition becoming a view that selects from the
> parent, which surely ought to be forbidden.

Hmm, yes.  The following exercise convinced me.

create table r (a int) partition by range (a);
create table r1 partition of r for values from (1) to (10);
create rule "_RETURN" as on select to r1 do instead select * from r;

insert into r values (1);
ERROR:  cannot insert into view "r1"
HINT:  To enable inserting into the view, provide an INSTEAD OF INSERT
trigger or an unconditional ON INSERT DO INSTEAD rule.

The error is emitted by CheckValidResultRel() that is called on individual
leaf partitions when setting up tuple-routing in ExecInitModifyTable.

I agree that we should forbid this case, so please find attached a 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 20 June 2017 at 03:01, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/06/19 20:19, Dean Rasheed wrote:
>> Currently we allow rules to be defined on table partitions, but these
>> rules only fire when the partition is accessed directly, not when it
>> is accessed via the parent:
>
> Yeah, the same thing as will happen with an inheritance setup, but I guess
> you are asking whether that's what we want.
>
> With old-style inheritance based setup, you will see exactly the exact
> same result:
>
> Note that inheritance is expanded in the planner, not the rewriter, so the
> rules of partitions (that are added to the query later than the rewriter)
> won't fire, AFAICS.  Same will apply to partitions.
>

Ah yes, you're right. I was misremembering how that worked. In an
old-style inheritance-based setup you would have to define triggers on
the parent table to handle INSERT, and since they would insert
directly into the relevant child, any child INSERT rules would be
fired, but child UPDATE and DELETE rules would not. At least with
built-in partitioning it's consistent in not firing rules on the
partitions for any command.


>> Perhaps we
>> should explicitly forbid this for now -- i.e., raise a "not supported"
>> error when attempting to add a rule to a partition, or attach a table
>> with rules to a partitioned table.
>
> We could do that, but an oft-raised question is how different we should
> make new partitions from the old-style inheritance child tables?
>
> Although a slightly different territory, you will also notice that
> statement triggers of partitions won't be fired unless they are explicitly
> named in the query, which is what happens for inheritance in general and
> hence also for partitions.
>
>> Personally, I wouldn't regard adding proper support for rules on
>> partitions as a high priority, so I'd be OK with it remaining
>> unsupported unless someone cares enough to implement it, but that
>> seems preferable to leaving it partially working in this way.
>
> Sure, if consensus turns out to be that we prohibit rules, statement
> triggers, etc. that depend on the relation being explicitly named in the
> query to be defined on partitions, I could draft up a patch for v10.
>

Hmm, perhaps it's OK as-is. The user can always define the
rules/triggers on both the parent and the children, if that's what
they want.


>> Also, as things stand, it is possible to do the following:
>>
>> CREATE TABLE t2(a int, b int) PARTITION BY RANGE(a);
>> CREATE TABLE t2_p PARTITION OF t2 FOR VALUES FROM (1) TO (10);
>> CREATE RULE "_RETURN" AS ON SELECT TO t2_p DO INSTEAD SELECT * FROM t2;
>>
>> which results in the partition becoming a view that selects from the
>> parent, which surely ought to be forbidden.
>
> Hmm, yes.  The following exercise convinced me.
>
> create table r (a int) partition by range (a);
> create table r1 partition of r for values from (1) to (10);
> create rule "_RETURN" as on select to r1 do instead select * from r;
>
> insert into r values (1);
> ERROR:  cannot insert into view "r1"
> HINT:  To enable inserting into the view, provide an INSTEAD OF INSERT
> trigger or an unconditional ON INSERT DO INSTEAD rule.
>
> The error is emitted by CheckValidResultRel() that is called on individual
> leaf partitions when setting up tuple-routing in ExecInitModifyTable.
>
> I agree that we should forbid this case,

Yeah. Also it would be possible to define the rule to select from a
non-empty table, leading to rows appearing in the partition, but not
the parent. Since we normally explicitly forbid the use of a view as a
table partition, it seems worth closing the loophole in this case.

> so please find attached a patch.

Thanks, I'll take a look at it later today.

Regards,
Dean

On 2017/06/20 17:51, Dean Rasheed wrote:
> On 20 June 2017 at 03:01, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/06/19 20:19, Dean Rasheed wrote:
>>> Perhaps we
>>> should explicitly forbid this for now -- i.e., raise a "not supported"
>>> error when attempting to add a rule to a partition, or attach a table
>>> with rules to a partitioned table.
>>
>> We could do that, but an oft-raised question is how different we should
>> make new partitions from the old-style inheritance child tables?
>>
>> Although a slightly different territory, you will also notice that
>> statement triggers of partitions won't be fired unless they are explicitly
>> named in the query, which is what happens for inheritance in general and
>> hence also for partitions.
>>
>>> Personally, I wouldn't regard adding proper support for rules on
>>> partitions as a high priority, so I'd be OK with it remaining
>>> unsupported unless someone cares enough to implement it, but that
>>> seems preferable to leaving it partially working in this way.
>>
>> Sure, if consensus turns out to be that we prohibit rules, statement
>> triggers, etc. that depend on the relation being explicitly named in the
>> query to be defined on partitions, I could draft up a patch for v10.
>>
> 
> Hmm, perhaps it's OK as-is. The user can always define the
> rules/triggers on both the parent and the children, if that's what
> they want.

I feel that the documentation in this area could clarify some of these
things a bit more (at least any differences that exist between the
old-style inheritance and new partitioning).  I will try to see if I can
come up with a sensible patch for that.

>>> Also, as things stand, it is possible to do the following:
>>>
>>> CREATE TABLE t2(a int, b int) PARTITION BY RANGE(a);
>>> CREATE TABLE t2_p PARTITION OF t2 FOR VALUES FROM (1) TO (10);
>>> CREATE RULE "_RETURN" AS ON SELECT TO t2_p DO INSTEAD SELECT * FROM t2;
>>>
>>> which results in the partition becoming a view that selects from the
>>> parent, which surely ought to be forbidden.
>>
>> Hmm, yes.  The following exercise convinced me.
>>
>> create table r (a int) partition by range (a);
>> create table r1 partition of r for values from (1) to (10);
>> create rule "_RETURN" as on select to r1 do instead select * from r;
>>
>> insert into r values (1);
>> ERROR:  cannot insert into view "r1"
>> HINT:  To enable inserting into the view, provide an INSTEAD OF INSERT
>> trigger or an unconditional ON INSERT DO INSTEAD rule.
>>
>> The error is emitted by CheckValidResultRel() that is called on individual
>> leaf partitions when setting up tuple-routing in ExecInitModifyTable.
>>
>> I agree that we should forbid this case,
> 
> Yeah. Also it would be possible to define the rule to select from a
> non-empty table, leading to rows appearing in the partition, but not
> the parent. Since we normally explicitly forbid the use of a view as a
> table partition, it seems worth closing the loophole in this case.

Oh, that's even worse. :-(

>> so please find attached a patch.
> 
> Thanks, I'll take a look at it later today.

Thanks.

Regards,
Amit

On 20 June 2017 at 03:01, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Hmm, yes.  The following exercise convinced me.
>
> create table r (a int) partition by range (a);
> create table r1 partition of r for values from (1) to (10);
> create rule "_RETURN" as on select to r1 do instead select * from r;
>
> insert into r values (1);
> ERROR:  cannot insert into view "r1"
> HINT:  To enable inserting into the view, provide an INSTEAD OF INSERT
> trigger or an unconditional ON INSERT DO INSTEAD rule.
>
> The error is emitted by CheckValidResultRel() that is called on individual
> leaf partitions when setting up tuple-routing in ExecInitModifyTable.
>
> I agree that we should forbid this case, so please find attached a patch.
>

Committed.

Regards,
Dean

On 2017/06/21 18:49, Dean Rasheed wrote:
> On 20 June 2017 at 03:01, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Hmm, yes.  The following exercise convinced me.
>>
>> create table r (a int) partition by range (a);
>> create table r1 partition of r for values from (1) to (10);
>> create rule "_RETURN" as on select to r1 do instead select * from r;
>>
>> insert into r values (1);
>> ERROR:  cannot insert into view "r1"
>> HINT:  To enable inserting into the view, provide an INSTEAD OF INSERT
>> trigger or an unconditional ON INSERT DO INSTEAD rule.
>>
>> The error is emitted by CheckValidResultRel() that is called on individual
>> leaf partitions when setting up tuple-routing in ExecInitModifyTable.
>>
>> I agree that we should forbid this case, so please find attached a patch.
>>
> 
> Committed.

Thanks, Dean.

Regards,
Amit