Обсуждение: Unique indexes & constraints on partitioned tables

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services