Re: Unique indexes & constraints on partitioned tables

[Alvaro Herrera]

Thanks for the patch.

Amit Langote wrote:

> I mentioned this case at [1] and had a WIP patch to address that.  Please
> find it attached here.  It is to be applied on top of both of your patches.

In this bit:

> + /*
> +  * When specific arbiter indexes requested, only examine them.  
> If
> +  * this is a partition (after a tuple is routed to it from the
> +  * parent into which the original tuple has been inserted), we 
> must
> +  * check the parent index id, instead of our own id, because 
> that's
> +  * the one that appears in the arbiterIndexes list.
> +  */
>   if (arbiterIndexes != NIL &&
> - !list_member_oid(arbiterIndexes,
> -  
> indexRelation->rd_index->indexrelid))
> + !(list_member_oid(arbiterIndexes,
> +   
> indexRelation->rd_index->indexrelid) ||
> +   list_member_oid(arbiterIndexes,
> +   
> indexRelation->rd_index->indparentidx)))

I think this would fail if there is two-level partitioning (or more),
because the index mentioned in the arbiter indexes list would be the
grand-parent index and would not appear in indparentidx.  Maybe what we
need is to map the parent index ids to partition indexes, all the way up
in ExecInsert before calling ExecCheckIndexConstraints, which looks
pretty annoying.

Another I'm mildly worried about is the use of ModifyState->nominalRelation,
which is said to be "for the use of EXPLAIN"; in this new code, we're
extending its charter so that we're actually relying on it for
execution.  Maybe this is not a problem and we just need to update the
comments (if we believe that we maintain it reliably enough, which is
probably true), but I'm not certain.

I also wonder about short-circuiting the build of the on-conflict stuff
for the parent table in the partitioned case, because surely we don't
need it.  But that seems fairly minor.

Thanks again,

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Unique indexes & constraints on partitioned tables

[Alvaro Herrera]

Amit Langote wrote:

> Have you considered what happens when ON CONFLICT code tries to depend on
> such an index (a partitioned unique index)?

Not yet, but it was on my list of things to fix.  Thanks for working on
it -- I'll be reviewing this soon.

> +create table parted_conflict_test_2 partition of parted_conflict_test for 
> values in (2);
> +create unique index on parted_conflict_test (a);
> +insert into parted_conflict_test values (1, 'a') on conflict (a) do nothing;
> +insert into parted_conflict_test values (1, 'b') on conflict (a) do update 
> set b = excluded.b;
> +insert into parted_conflict_test values (1, 'b') on conflict (a) do update 
> set b = excluded.b where parted_conflict_test.b = 'a';
> +select * from parted_conflict_test;
> + a | b 
> +---+---
> + 1 | b
> +(1 row)

sweet.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Unique indexes & constraints on partitioned tables

[Amit Langote]

Hi Alvaro,

On 2017/12/23 6:29, Alvaro Herrera wrote:
> Hello,
> 
> I'm giving this patch its own thread for mental sanity, but this is
> essentially what already posted in [1], plus some doc fixes.  This patch
> depends on the main "local partitioned indexes" in that thread, last
> version of which is at [2].

Thanks for working on this.

Have you considered what happens when ON CONFLICT code tries to depend on
such an index (a partitioned unique index)?  It seems we'll need some new
code in the executor for the same.  I tried the following after applying
your patch:

create table p (a char) partition by list (a);
create table pa partition of p for values in ('a');;
create table pb partition of p for values in ('b');
create unique index on p (a);

insert into p values ('a', 1);
INSERT 0 1

insert into p values ('a', 1);
ERROR:  duplicate key value violates unique constraint "pa_a_idx"
DETAIL:  Key (a)=(a) already exists.

insert into p values ('a', 1) on conflict do nothing;
INSERT 0 0

Fine so far... but

insert into p values ('a', 1) on conflict (a) do nothing;
ERROR:  unexpected failure to find arbiter index

or

insert into p values ('a', 1) on conflict (a) do update set b = excluded.b;
ERROR:  unexpected failure to find arbiter index

I mentioned this case at [1] and had a WIP patch to address that.  Please
find it attached here.  It is to be applied on top of both of your patches.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/a26f7823-6c7d-3f41-c5fb-7d50dd2f4848%40lab.ntt.co.jp
>From 490a8302b71cbe78e7028879b0dffa2c43d03f33 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 25 Dec 2017 18:45:33 +0900
Subject: [PATCH 3/3] Teach executor to handle ON CONFLICT (key) on partitioned
 tables

---
 src/backend/executor/execIndexing.c   | 14 +---
 src/backend/executor/nodeModifyTable.c| 32 +++
 src/test/regress/expected/insert_conflict.out | 27 +-
 src/test/regress/sql/insert_conflict.sql  | 18 ++-
 4 files changed, 77 insertions(+), 14 deletions(-)

diff --git a/src/backend/executor/execIndexing.c 
b/src/backend/executor/execIndexing.c
index 89e189fa71..f76a2ede76 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -531,10 +531,18 @@ ExecCheckIndexConstraints(TupleTableSlot *slot,
if (!indexInfo->ii_ReadyForInserts)
continue;
 
-   /* When specific arbiter indexes requested, only examine them */
+   /*
+* When specific arbiter indexes requested, only examine them.  
If
+* this is a partition (after a tuple is routed to it from the
+* parent into which the original tuple has been inserted), we 
must
+* check the parent index id, instead of our own id, because 
that's
+* the one that appears in the arbiterIndexes list.
+*/
if (arbiterIndexes != NIL &&
-   !list_member_oid(arbiterIndexes,
-
indexRelation->rd_index->indexrelid))
+   !(list_member_oid(arbiterIndexes,
+ 
indexRelation->rd_index->indexrelid) ||
+ list_member_oid(arbiterIndexes,
+ 
indexRelation->rd_index->indparentidx)))
continue;
 
if (!indexRelation->rd_index->indimmediate)
diff --git a/src/backend/executor/nodeModifyTable.c 
b/src/backend/executor/nodeModifyTable.c
index afb83ed3ae..94f819006a 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2186,6 +2186,38 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, 
int eflags)
 
resultRelInfo->ri_onConflictSetWhere = qualexpr;
}
+
+   /* Build the above information for each leaf partition rel */
+   for (i = 0; i < mtstate->mt_num_partitions; i++)
+   {
+   Relationpartrel;
+   List   *leaf_oc_set;
+
+   resultRelInfo = mtstate->mt_partitions[i];
+   partrel = resultRelInfo->ri_RelationDesc;
+
+   /* varno = node->nominalRelation */
+   leaf_oc_set = 
map_partition_varattnos(node->onConflictSet,
+   
node->nominalRelation,
+   
partrel, rel, NULL);
+   resultRelInfo->ri_onConflictSetProj =
+   ExecBuildProjectionInfo(leaf_oc_set, 
econtext,
+   

Unique indexes & constraints on partitioned tables

[Alvaro Herrera]

Hello,

I'm giving this patch its own thread for mental sanity, but this is
essentially what already posted in [1], plus some doc fixes.  This patch
depends on the main "local partitioned indexes" in that thread, last
version of which is at [2].

I also added a mechanism to set the constraints in partitions as
dependent on the constraint in the parent partitioned table, so deletion
is sensible: the PK in partitions goes away when the PK in the parent
table is dropped; and you can't drop the PK in partitions on their own.

In order to implement that I dress Rube Goldberg as Santa: the
constraint OID of the parent is needed by index_constraint_create when
creating the child; but it's the previous index_constraint_create itself
that generates the OID when creating the parent, and it's DefineIndex
that does the recursion.  So index_constraint_create returns the value
to index_create who returns it to DefineIndex, so that the recursive
step can pass it down to index_create to give it to
index_constraint_create.  It seems crazy, but it's correct.

As far as I can tell, pg_dump works correctly without any additional
changes.

[1] https://postgr.es/m/20171220194937.pldcecyx7yrwmgkg@alvherre.pgsql
[2] https://postgr.es/m/20171220212503.aamhlrs425flg47f@alvherre.pgsql

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 311cdeeae542a3f4cc20bf2308488756b8d3da80 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Mon, 6 Nov 2017 17:04:55 +0100
Subject: [PATCH] allow indexes on partitioned tables to be unique

---
 doc/src/sgml/ref/alter_table.sgml  |   9 +-
 doc/src/sgml/ref/create_table.sgml |  16 +++-
 src/backend/bootstrap/bootparse.y  |   2 +
 src/backend/catalog/index.c|  28 +-
 src/backend/catalog/toasting.c |   4 +-
 src/backend/commands/indexcmds.c   |  80 ++--
 src/backend/commands/tablecmds.c   |  12 ++-
 src/backend/parser/parse_utilcmd.c |  31 +--
 src/backend/tcop/utility.c |   1 +
 src/include/catalog/index.h|   5 +-
 src/include/commands/defrem.h  |   1 +
 src/include/parser/parse_utilcmd.h |   3 +-
 src/test/regress/expected/alter_table.out  |   8 --
 src/test/regress/expected/create_table.out |  12 ---
 src/test/regress/expected/indexing.out | 142 -
 src/test/regress/sql/alter_table.sql   |   2 -
 src/test/regress/sql/create_table.sql  |   8 --
 src/test/regress/sql/indexing.sql  |  73 ++-
 18 files changed, 355 insertions(+), 82 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml 
b/doc/src/sgml/ref/alter_table.sgml
index 0a2f3e3646..ee6a45c9ad 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -782,8 +782,9 @@ ALTER TABLE [ IF EXISTS ] name
   This form attaches an existing table (which might itself be partitioned)
   as a partition of the target table. The table can be attached
   as a partition for specific values using FOR VALUES
-   or as a default partition by using DEFAULT
-  .  For each index in the target table, a corresponding
+   or as a default partition by using
+  DEFAULT.
+  For each index in the target table, a corresponding
   one will be created in the attached table; or, if an equivalent
   index already exists, will be attached to the target table's index,
   as if ALTER INDEX ATTACH had been executed.
@@ -798,8 +799,10 @@ ALTER TABLE [ IF EXISTS ] name
   as the target table and no more; moreover, the column types must also
   match.  Also, it must have all the NOT NULL and
   CHECK constraints of the target table.  Currently
-  UNIQUE, PRIMARY KEY, and
   FOREIGN KEY constraints are not considered.
+  UNIQUE and PRIMARY KEY constraints
+  from the parent table will be created in the partition, if they don't
+  already exist.
   If any of the CHECK constraints of the table being
   attached is marked NO INHERIT, the command will fail;
   such a constraint must be recreated without the NO 
INHERIT
diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index a0c9a6d257..98ab39473b 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -546,8 +546,8 @@ WITH ( MODULUS numeric_literal, REM
  
 
  
-  Partitioned tables do not support UNIQUE,
-  PRIMARY KEY, EXCLUDE, or
+  Partitioned tables do not support
+  EXCLUDE, or
   FOREIGN KEY constraints; however, you can define
   these constraints on individual partitions.
  
@@ -786,6 +786,11 @@ WITH ( MODULUS numeric_literal, REM
   primary key constraint defined for the table.  (Otherwise it
   would just be the same constraint listed twice.)
  
+
+ 
+  When used on