On 2017/12/01 11:01, Amit Langote wrote:
> On 2017/12/01 1:02, Robert Haas wrote:
>> Second, this would be the first place where the second argument to
>> ExecOpenIndices() is passed simply as true. The only other caller
>> that doesn't pass constant false is in nodeModifyTable.c and looks
>> like this:
>>
>> ExecOpenIndices(resultRelInfo, mtstate->mt_onconflict != ONCONFLICT_NONE);
>>
>> The intention here appears to be to avoid the overhead of doing
>> BuildSpeculativeIndexInfo() when it isn't needed. I think we should
>> try to do the same thing here. As written, the patch will do that
>> work even when the query has no ON CONFLICT clause, which doesn't seem
>> like a good idea.
>
> Agreed. One thing though is that ExecSetupPartitionTupleRouting doesn't
> receive the mtstate today. I've a patch pending that will re-design that
> interface (for another project) that I will post in the near future, but
> meanwhile let's just add a new parameter mtstate so that this patch can
> move forward with its business. The future patch I'm talking about will
> keep the parameter intact, so it should be OK now to add the same.
>
> Attached two patches:
>
> 0001: refactoring to add the mtstate parameter
> 0002: the original patch updated to address the above comment
I forgot to consider the fact that mtstate could be NULL in
ExecSetupPartitionTupleRouting(), so would result in dereferencing NULL
pointer when called from CopyFrom(), which fixed in the attached updated
patch.
Thanks,
Amit
From 986676c8614d2d5954ea5d505cf729dc6dc487bd Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 1 Dec 2017 10:44:37 +0900
Subject: [PATCH 1/2] Pass mtstate to ExecSetupPartitionTupleRouting
---
src/backend/commands/copy.c | 3 ++-
src/backend/executor/execPartition.c | 3 ++-
src/backend/executor/nodeModifyTable.c | 3 ++-
src/include/executor/execPartition.h | 3 ++-
4 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 13eb9e34ba..bace390470 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2478,7 +2478,8 @@ CopyFrom(CopyState cstate)
int num_parted,
num_partitions;
- ExecSetupPartitionTupleRouting(cstate->rel,
+ ExecSetupPartitionTupleRouting(NULL,
+
cstate->rel,
1,
estate,
&partition_dispatch_info,
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index 59a0ca4597..f6108a156b 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -63,7 +63,8 @@ static char *ExecBuildSlotPartitionKeyDescription(Relation
rel,
* RowExclusiveLock mode upon return from this function.
*/
void
-ExecSetupPartitionTupleRouting(Relation rel,
+ExecSetupPartitionTupleRouting(ModifyTableState *mtstate,
+ Relation rel,
Index resultRTindex,
EState *estate,
PartitionDispatch
**pd,
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index 1e3ece9b34..afb83ed3ae 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1953,7 +1953,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
int num_parted,
num_partitions;
- ExecSetupPartitionTupleRouting(rel,
+ ExecSetupPartitionTupleRouting(mtstate,
+ rel,
node->nominalRelation,
estate,
&partition_dispatch_info,
diff --git a/src/include/executor/execPartition.h
b/src/include/executor/execPartition.h
index 43ca9908aa..86a199d169 100644
--- a/src/include/executor/execPartition.h
+++ b/src/include/executor/execPartition.h
@@ -49,7 +49,8 @@ typedef struct PartitionDispatchData
typedef struct PartitionDispatchData *PartitionDispatch;
-extern void ExecSetupPartitionTupleRouting(Relation rel,
+extern void ExecSetupPartitionTupleRouting(ModifyTableState *mtstate,
+ Relation rel,
Index resultRTindex,
EState *estate,
PartitionDispatch
**pd,
--
2.11.0
From d933513cb7cf14ddb766c7afcc0792b7841b0852 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 3 Apr 2017 19:13:38 +0900
Subject: [PATCH 2/2] Allow ON CONFLICT DO NOTHING form on partitioned table
ON CONFLICT .. DO UPDATE still doesn't work, because it requires
specifying the conflict target. DO NOTHING doesn't require it,
but the executor will check for conflicts within only a given
leaf partitions, if relevant constraints exist.
Specifying the conflict target makes the planner look for the
required indexes on the parent table, which are not allowed, so an
error will always be reported in that case.
---
doc/src/sgml/ddl.sgml | 12 +++++++++---
src/backend/executor/execPartition.c | 13 +++++++++----
src/backend/parser/analyze.c | 8 --------
src/test/regress/expected/insert_conflict.out | 13 +++++++++++++
src/test/regress/sql/insert_conflict.sql | 13 +++++++++++++
5 files changed, 44 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 9f583266de..d109ee3fe1 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3288,9 +3288,15 @@ ALTER TABLE measurement ATTACH PARTITION
measurement_y2008m02
<listitem>
<para>
Using the <literal>ON CONFLICT</literal> clause with partitioned tables
- will cause an error, because unique or exclusion constraints can only be
- created on individual partitions. There is no support for enforcing
- uniqueness (or an exclusion constraint) across an entire partitioning
+ will cause an error if the conflict target is specified (see
+ <xref linkend="sql-on-conflict"> for more details on how the clause
+ works). That means it's not possible to specify
+ <literal>DO UPDATE</literal> as the alternative action, because
+ specifying the conflict target is mandatory in that case. On the other
+ hand, specifying <literal>DO NOTHING</literal> as the alternative action
+ works fine provided the conflict target is not specified. In that case,
+ unique constraints (or exclusion constraints) of the individual leaf
+ partitions are considered, not those across the whole partitioning
hierarchy.
</para>
</listitem>
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index f6108a156b..2c712cd266 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -134,13 +134,18 @@ ExecSetupPartitionTupleRouting(ModifyTableState *mtstate,
CheckValidResultRel(leaf_part_rri, CMD_INSERT);
/*
- * Open partition indices (remember we do not support ON
CONFLICT in
- * case of partitioned tables, so we do not need support
information
- * for speculative insertion)
+ * Open partition indices. The user may have asked to check for
+ * conflicts within this leaf partition and do "nothing"
instead of
+ * throwing an error. Be prepared in that case by initializing
the
+ * index information needed by ExecInsert() to perform
speculative
+ * insertions, but only if it would be needed at all.
*/
if (leaf_part_rri->ri_RelationDesc->rd_rel->relhasindex &&
leaf_part_rri->ri_IndexRelationDescs == NULL)
- ExecOpenIndices(leaf_part_rri, false);
+ ExecOpenIndices(leaf_part_rri,
+ mtstate != NULL
+ ?
mtstate->mt_onconflict != ONCONFLICT_NONE
+ : false);
estate->es_leaf_result_relations =
lappend(estate->es_leaf_result_relations,
leaf_part_rri);
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 757a4a8fd1..d680d2285c 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -847,16 +847,8 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
/* Process ON CONFLICT, if any. */
if (stmt->onConflictClause)
- {
- /* Bail out if target relation is partitioned table */
- if (pstate->p_target_rangetblentry->relkind ==
RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("ON CONFLICT clause is not
supported with partitioned tables")));
-
qry->onConflict = transformOnConflictClause(pstate,
stmt->onConflictClause);
- }
/*
* If we have a RETURNING clause, we need to add the target relation to
diff --git a/src/test/regress/expected/insert_conflict.out
b/src/test/regress/expected/insert_conflict.out
index 8d005fddd4..8fd2027d6a 100644
--- a/src/test/regress/expected/insert_conflict.out
+++ b/src/test/regress/expected/insert_conflict.out
@@ -786,3 +786,16 @@ select * from selfconflict;
(3 rows)
drop table selfconflict;
+-- check that the following works:
+-- insert into partitioned_table on conflict do nothing
+create table parted_conflict_test (a int, b char) partition by list (a);
+create table parted_conflict_test_1 partition of parted_conflict_test (b
unique) for values in (1);
+insert into parted_conflict_test values (1, 'a') on conflict do nothing;
+insert into parted_conflict_test values (1, 'a') on conflict do nothing;
+-- however, on conflict do update is not supported yet
+insert into parted_conflict_test values (1) on conflict (b) do update set a =
excluded.a;
+ERROR: there is no unique or exclusion constraint matching the ON CONFLICT
specification
+-- but it works OK if we target the partition directly
+insert into parted_conflict_test_1 values (1) on conflict (b) do
+update set a = excluded.a;
+drop table parted_conflict_test;
diff --git a/src/test/regress/sql/insert_conflict.sql
b/src/test/regress/sql/insert_conflict.sql
index df3a9b59b5..32c647e3f8 100644
--- a/src/test/regress/sql/insert_conflict.sql
+++ b/src/test/regress/sql/insert_conflict.sql
@@ -471,3 +471,16 @@ commit;
select * from selfconflict;
drop table selfconflict;
+
+-- check that the following works:
+-- insert into partitioned_table on conflict do nothing
+create table parted_conflict_test (a int, b char) partition by list (a);
+create table parted_conflict_test_1 partition of parted_conflict_test (b
unique) for values in (1);
+insert into parted_conflict_test values (1, 'a') on conflict do nothing;
+insert into parted_conflict_test values (1, 'a') on conflict do nothing;
+-- however, on conflict do update is not supported yet
+insert into parted_conflict_test values (1) on conflict (b) do update set a =
excluded.a;
+-- but it works OK if we target the partition directly
+insert into parted_conflict_test_1 values (1) on conflict (b) do
+update set a = excluded.a;
+drop table parted_conflict_test;
--
2.11.0
