Re: [HACKERS] BEFORE trigger can cause undetected partitionconstraint violation

On 2017/06/07 1:19, Robert Haas wrote:
> On Mon, Jun 5, 2017 at 1:02 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/06/03 1:56, Robert Haas wrote:
>>> On Fri, Jun 2, 2017 at 12:51 AM, Amit Langote
>>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>> Attached patch makes InitResultRelInfo() *always* initialize the
>>>> partition's constraint, that is, regardless of whether insert/copy is
>>>> through the parent or directly on the partition.  I'm wondering if
>>>> ExecInsert() and CopyFrom() should check if it actually needs to execute
>>>> the constraint?  I mean it's needed if there exists BR insert triggers
>>>> which may change the row, but not otherwise.  The patch currently does not
>>>> implement that check.
>>>
>>> I think it should.  I mean, otherwise we're leaving a
>>> probably-material amount of performance on the table.
>>
>> I agree.  Updated the patch to implement the check.
> 
> OK, so this isn't quite right.  Consider the following test case:
> 
> rhaas=# create table x (a int, b int, c int) partition by list (a);
> CREATE TABLE
> rhaas=# create table y partition of x for values in (1) partition by list (b);
> CREATE TABLE
> rhaas=# create table z partition of y for values in (1);
> CREATE TABLE
> rhaas=# insert into y values (2,1,1);
> INSERT 0 1

Gah!

> The absence of the before-trigger is treated as evidence that z need
> not revalidate the partition constraint, but actually it (or
> something) still needs to enforce the part of it that originates from
> y's ancestors.  In short, this patch would reintroduce the bug that
> was fixed in 39162b2030fb0a35a6bb28dc636b5a71b8df8d1c, and would do so
> by removing the exact same code that was added (by you!) in that
> patch.

I think we will have to go for the "or something" here, which is the way I
should have originally proposed it (I mean the patch that went in as
39162b2030fb0a35a6bb28dc636b5a71b8df8d1c). :-(

How about we export ExecPartitionCheck() out of execMain.c and call it
just before ExecFindPartition() using the root table's ResultRelInfo?  If
the root table is a partition, its ResultRelInfo.ri_PartitionCheck must
have been initialized, which ExecPartitionCheck will check.  Since there
cannot be any BR triggers on the root table (because partitioned), we can
safely do that.  After tuple-routing, if the target leaf partition has BR
triggers, any violating changes they might make will be checked by
ExecConstraints() using the leaf partition's ResultRelInfo, whose
ri_PartitionCheck consists of its own partition constraints plus that of
any of its ancestors that are partitions.  If the leaf partition does not
have any BR triggers we need not check any partition constraints just as
the patch does.  (Remember that we already checked the root table's
partition constraint before we began routing the tuple, as the proposed
updated patch will do, and we don't need to worry about any of
intermediate ancestors, because if the tuple does not satisfy the
constraint of any one of those, tuple-routing will fail anyway.)

I proposed a similar thing in the hash partitioning thread [1], where
Dilip was complaining about name of the table that appears in the
"violates partition constraint" error message.  Even if the tuple failed
an ancestor table's partition constraint, since the ResultRelInfo passed
to ExecConstraints() is the leaf partition's, the name shown is also leaf
partition's.  ISTM, showing the leaf partition's name is fine as long as
it's a NOT NULL or a CHECK constraint failing, because they are explicitly
attached to the leaf table, but maybe not fine when an implicitly
inherited internal partition constraint fails.

Attached updated patch that implements the change described above, in
addition to fixing the originally reported bug.  With the patch:

create table x (a int, b int, c int) partition by list (a);
create table y partition of x for values in (1) partition by list (b);
create table z partition of y for values in (1);

insert into y values (2,1,1);
ERROR:  new row for relation "y" violates partition constraint
DETAIL:  Failing row contains (2, 1, 1).

-- whereas on HEAD, it shows the leaf partition's name
insert into y values (2,1,1);
ERROR:  new row for relation "z" violates partition constraint
DETAIL:  Failing row contains (2, 1, 1).

Thoughts?

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/0ded7a50-0b35-1805-232b-f8edcf4cbadb%40lab.ntt.co.jp

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