From a897e7c25ae62627987a4d8243b02e0b55e012dd Mon Sep 17 00:00:00 2001 From: amit 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 | 19 +++++++++++++++-- src/backend/executor/execMain.c | 37 ++++++++-------------------------- src/backend/executor/nodeModifyTable.c | 21 +++++++++++++++++-- src/test/regress/expected/insert.out | 13 ++++++++++++ src/test/regress/sql/insert.sql | 10 +++++++++ 5 files changed, 67 insertions(+), 33 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 810bae5dad..0a33c40c17 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -2640,9 +2640,24 @@ CopyFrom(CopyState cstate) } else { + /* + * We always check the partition constraint, including when + * the tuple got here via tuple-routing. However we don't + * need to in the latter case if no BR trigger is defined on + * the partition. Note that a BR trigger might modify the + * tuple such that the partition constraint is no longer + * satisfied, so we need to check in that case. + */ + bool check_partition_constr = + (resultRelInfo->ri_PartitionCheck != NIL); + + if (saved_resultRelInfo != NULL && + !(resultRelInfo->ri_TrigDesc && + resultRelInfo->ri_TrigDesc->trig_insert_before_row)) + check_partition_constr = false; + /* Check the constraints of the tuple */ - if (cstate->rel->rd_att->constr || - resultRelInfo->ri_PartitionCheck) + if (cstate->rel->rd_att->constr || check_partition_constr) ExecConstraints(resultRelInfo, slot, estate); if (useHeapMultiInsert) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 4a899f1eb5..fc7580743a 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 in certain cases. 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..bf26488c51 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -415,6 +415,16 @@ ExecInsert(ModifyTableState *mtstate, else { /* + * We always check the partition constraint, including when the tuple + * got here via tuple-routing. However we don't need to in the latter + * case if no BR trigger is defined on the partition. Note that a BR + * trigger might modify the tuple such that the partition constraint + * is no longer satisfied, so we need to check in that case. + */ + bool check_partition_constr = + (resultRelInfo->ri_PartitionCheck != NIL); + + /* * Constraints might reference the tableoid column, so initialize * t_tableOid before evaluating them. */ @@ -431,9 +441,16 @@ ExecInsert(ModifyTableState *mtstate, resultRelInfo, slot, estate); /* - * Check the constraints of the tuple + * No need though if the tuple has been routed, and a BR trigger + * doesn't exist. */ - if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck) + if (saved_resultRelInfo != NULL && + !(resultRelInfo->ri_TrigDesc && + resultRelInfo->ri_TrigDesc->trig_insert_before_row)) + check_partition_constr = false; + + /* Check the constraints of the tuple */ + if (resultRelationDesc->rd_att->constr || check_partition_constr) 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