On 2017/06/02 10:36, Robert Haas wrote: > On Thu, Jun 1, 2017 at 6:05 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: >> Without having checked the code, I imagine the reason for this is >> that BEFORE triggers are fired after tuple routing occurs. > > Yep. > >> Re-ordering that seems problematic, because what if you have different >> triggers on different partitions? We'd have to forbid that, probably >> by saying that only the parent table's BEFORE ROW triggers are fired, >> but that seems not very nice. > > The parent hasn't got any triggers; that's forbidden. > >> The alternative is to forbid BEFORE triggers on partitions to alter >> the routing result, probably by rechecking the partition constraint >> after trigger firing. > > That's what we need to do. Until we do tuple routing, we don't know > which partition we're addressing, so we don't know which triggers > we're firing. So the only way to prevent this is to recheck. Which I > think is supposed to work already, but apparently doesn't.
That has to do with the assumption written down in the following portion of a comment in InitResultRelInfo(): /* * If partition_root has been specified, that means we are building the * ResultRelInfo for one of its leaf partitions. In that case, we need * *not* initialize the leaf partition's constraint, but rather the * partition_root's (if any). which, as it turns out, is wrong. It completely disregards the fact that BR triggers are executed after tuple-routing and can change the row. 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. Thanks, Amit
From db67c73ea1924dc205ac088eaa63c93e20eb3fa0 Mon Sep 17 00:00:00 2001 From: amit <amitlangot...@gmail.com> Date: Fri, 2 Jun 2017 12:11:17 +0900 Subject: [PATCH] Check the partition constraint even after tuple-routing Partition constraint expressions are not initialized when inserting through the parent because tuple-routing is said to implicitly preserve the constraint. But BR triggers may change a row into one that violates the partition constraint and they are executed after tuple-routing, so any such change must be detected by checking the partition constraint explicitly. So, initialize them regardless of whether inserting through the parent or directly into the partition. --- src/backend/commands/copy.c | 2 +- src/backend/executor/execMain.c | 37 ++++++++-------------------------- src/backend/executor/nodeModifyTable.c | 3 ++- src/test/regress/expected/insert.out | 13 ++++++++++++ src/test/regress/sql/insert.sql | 10 +++++++++ 5 files changed, 34 insertions(+), 31 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 84b1a54cb9..cc2d75d167 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -2642,7 +2642,7 @@ CopyFrom(CopyState cstate) { /* Check the constraints of the tuple */ if (cstate->rel->rd_att->constr || - resultRelInfo->ri_PartitionCheck) + (resultRelInfo->ri_PartitionCheck != NIL)) ExecConstraints(resultRelInfo, slot, estate); if (useHeapMultiInsert) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 4a899f1eb5..72872a2420 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1339,35 +1339,14 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo, resultRelInfo->ri_projectReturning = NULL; /* - * If partition_root has been specified, that means we are building the - * ResultRelInfo for one of its leaf partitions. In that case, we need - * *not* initialize the leaf partition's constraint, but rather the - * partition_root's (if any). We must do that explicitly like this, - * because implicit partition constraints are not inherited like user- - * defined constraints and would fail to be enforced by ExecConstraints() - * after a tuple is routed to a leaf partition. - */ - if (partition_root) - { - /* - * Root table itself may or may not be a partition; partition_check - * would be NIL in the latter case. - */ - partition_check = RelationGetPartitionQual(partition_root); - - /* - * This is not our own partition constraint, but rather an ancestor's. - * So any Vars in it bear the ancestor's attribute numbers. We must - * switch them to our own. (dummy varno = 1) - */ - if (partition_check != NIL) - partition_check = map_partition_varattnos(partition_check, 1, - resultRelationDesc, - partition_root); - } - else - partition_check = RelationGetPartitionQual(resultRelationDesc); - + * Partition constraint. Note that it will be checked even in the case + * of tuple-routing. Although tuple-routing implicitly preserves the + * partition constraint of the target partition for a given row, the + * partition's BR triggers may change the row such that the constraint + * is no longer satisfied, which we must fail for by checking it + * explicitly. + */ + partition_check = RelationGetPartitionQual(resultRelationDesc); resultRelInfo->ri_PartitionCheck = partition_check; resultRelInfo->ri_PartitionRoot = partition_root; } diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index cf555fe78d..d2fee47d2e 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -433,7 +433,8 @@ ExecInsert(ModifyTableState *mtstate, /* * Check the constraints of the tuple */ - if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck) + if (resultRelationDesc->rd_att->constr || + (resultRelInfo->ri_PartitionCheck != NIL)) ExecConstraints(resultRelInfo, slot, estate); if (onconflict != ONCONFLICT_NONE && resultRelInfo->ri_NumIndices > 0) diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index 8b0752a0d2..8b6adb915a 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -495,3 +495,16 @@ select tableoid::regclass::text, * from mcrparted order by 1; -- cleanup drop table mcrparted; +-- check that a BR constraint can't make partition contain violating rows +create table brtrigpartcon (a int, b text) partition by list (a); +create table brtrigpartcon1 partition of brtrigpartcon for values in (1); +create or replace function brtrigpartcon1trigf() returns trigger as $$begin new.a := 2; return new; end$$ language plpgsql; +create trigger brtrigpartcon1trig before insert on brtrigpartcon1 for each row execute procedure brtrigpartcon1trigf(); +insert into brtrigpartcon values (1, 'hi there'); +ERROR: new row for relation "brtrigpartcon1" violates partition constraint +DETAIL: Failing row contains (2, hi there). +insert into brtrigpartcon1 values (1, 'hi there'); +ERROR: new row for relation "brtrigpartcon1" violates partition constraint +DETAIL: Failing row contains (2, hi there). +drop table brtrigpartcon; +drop function brtrigpartcon1trigf(); diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index db8967bad7..83c3ad8f53 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -332,3 +332,13 @@ select tableoid::regclass::text, * from mcrparted order by 1; -- cleanup drop table mcrparted; + +-- check that a BR constraint can't make partition contain violating rows +create table brtrigpartcon (a int, b text) partition by list (a); +create table brtrigpartcon1 partition of brtrigpartcon for values in (1); +create or replace function brtrigpartcon1trigf() returns trigger as $$begin new.a := 2; return new; end$$ language plpgsql; +create trigger brtrigpartcon1trig before insert on brtrigpartcon1 for each row execute procedure brtrigpartcon1trigf(); +insert into brtrigpartcon values (1, 'hi there'); +insert into brtrigpartcon1 values (1, 'hi there'); +drop table brtrigpartcon; +drop function brtrigpartcon1trigf(); -- 2.11.0
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers