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
