On 2017/06/03 1:56, Robert Haas wrote:
> On Fri, Jun 2, 2017 at 12:51 AM, Amit Langote
> <langote_amit...@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.
Thanks,
Amit
From a897e7c25ae62627987a4d8243b02e0b55e012dd 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 | 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
