unique indexes on partitioned tables

This new version fixes markup mistakes in the docs, and nothing else.
I'm not posting the ON CONFLICT DO UPDATE patch from Amit, since I
haven't fixed it; the 0003 patch has been squashed on 0002 instead, with
regression tests adapted. I'll see about the ON CONFLICT stuff as I
have time.

The 0001 patch is the same I posted last time in the other thread.

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

On 29 December 2017 at 23:06, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

This is the patch series for UNIQUE / PRIMARY KEY indexes on partitioned
tables. This is on top of the patch in
/messages/by-id/20171229175930.3aew7lzwd5w6m2x6@alvherre.pgsql
but I included it here as 0001 for simplicity. (Don't review that patch
in this thread please). This is essentially the same patch I posted
elsewhere in that thread.

This is a very simple patch, so not much to object to. Most of the
code is about passing the details thru APIs.

Looks good to go.

The comments are slightly better than the explanation in the docs.

I included Amit's support for ON CONFLICT DO UPDATE, but as I mentioned
in the other thread, it has a small bug. In principle we could push
0002 together with 0003, but I'd rather fix 0004 first and push it all
as one commit.

I agree we want 0004 also, but it would be simpler to just push 0002
and 0003 now and come back later for 0004. That would also avoid any
confusion over patch credits.

This serves as basis to build foreign keys on top; I'll post that
separately.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera wrote:

This is the patch series for UNIQUE / PRIMARY KEY indexes on partitioned
tables.

Rebased.

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

Version 4 of this patch, rebased on today's master.

The main change is in dependency handling for the constraints: you now
can't drop a constraint from a partition, if it's attached to a
constraint in the parent (you can't drop indexes from under the
constraints either, but that was true in previous versions too). Also
some error message rewording. I added a bunch of additional tests.

I implemented the dependencies using pg_depend entries. However,
pg_constraint has the notion of "coninhcount" and "conislocal", so I
update those values for the partition's pg_constraint row, because the
error messages are nicer that way. We could remove those lines from the
patch and the mechanics should be pretty much identical.

I'll review the doc additions, per Simon upthread.

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

Alvaro Herrera wrote:

Version 4 of this patch, rebased on today's master.

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

Hi Alvaro,

On 01/22/2018 05:55 PM, Alvaro Herrera wrote:

Alvaro Herrera wrote:

Version 4 of this patch, rebased on today's master.

Passes make check-world.

Maybe add a test case to indexing.sql that highlights that hash indexes
doesn't support UNIQUE; although not unique to partitioned indexes.

Thanks for working on this !

Best regards,
Jesper

On 1/22/18 17:55, Alvaro Herrera wrote:

Alvaro Herrera wrote:

Version 4 of this patch, rebased on today's master.

+           if (key->partattrs[i] == 0)
+               ereport(ERROR,
+                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                        errmsg("unsupported %s constraint with
partition key definition",
+                               constraint_type),
+                        errmsg("%s constraints cannot be used when
partition keys include expressions.",
+                               constraint_type)));

Double errmsg(). (Maybe an Assert somewhere should help catch this?)

+alter table idxpart add primary key (a); -- not an incomplete one tho

"though"?

I would like to see some tests that the unique constraints are actually
enforced. That is, insert some duplicate values and see it fail. Throw
some null values in, to check PK behavior as well. It should be
trivial, but seems kind of useful.

Other than that, this looks pretty good to me. A logical extension of
the previous partitioned index patch.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Alvaro.

On 2018/01/23 7:55, Alvaro Herrera wrote:

Alvaro Herrera wrote:

Version 4 of this patch, rebased on today's master.

With the latest patch, I noticed what I think is an unintended behavior.

create table p (a int, b int) partition by list (a);
create table p1 partition of p for values in (1) partition by range (b);
create table p11 partition of p1 for values from (1) to (10);
create table p2 partition of p for values in (2);

create unique index on p (a);
ERROR: insufficient columns in UNIQUE constraint definition
DETAIL: UNIQUE constraint on table "p1" lacks column "b" which is part of
the partition key.

It seems that after recursing to p1 which is itself partitioned,
DefineIndex() mistakenly looks for column b (which is in the p1's
partition key) in the unique key. I think that's unnecessary.
DefineIndex() should check that only once, that is, before recursing.

Please find attached a fix, a delta patch which applies on top of your v4
patch. With it:

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

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

drop index p_a_idx;
create unique index on p (a, b);
insert into p values (1, 1);
insert into p values (1, 1);
ERROR: duplicate key value violates unique constraint "p11_a_b_idx"
DETAIL: Key (a, b)=(1, 1) already exists.

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

Am I missing something?

Thanks,
Amit

On 2018/01/29 16:28, Amit Langote wrote:

create table p (a int, b int) partition by list (a);
create table p1 partition of p for values in (1) partition by range (b);
create table p11 partition of p1 for values from (1) to (10);
create table p2 partition of p for values in (2);

create unique index on p (a);
ERROR: insufficient columns in UNIQUE constraint definition
DETAIL: UNIQUE constraint on table "p1" lacks column "b" which is part of
the partition key.

It seems that after recursing to p1 which is itself partitioned,
DefineIndex() mistakenly looks for column b (which is in the p1's
partition key) in the unique key. I think that's unnecessary.
DefineIndex() should check that only once, that is, before recursing.

Hmm, scratch that...

Am I missing something?

Yes, I am.

create table p (a int, b int) partition by list (a);
create table p1 partition of p for values in (1) partition by range (b);
create table p11 partition of p1 for values from (1) to (10);
create table p12 partition of p1 for values from (10) to (20);
create table p2 partition of p for values in (2);

-- after applying my delta patch
create unique index on p (a);

insert into p values (1, 1); -- unique index p11 (a) says all fine
insert into p values (1, 10); -- unique index p12 (a) says all fine

That can't be right, because p (a) is no longer unique.

So, a unique key on a partitioned table must include the partition key
columns of *all* downstream partitioned tables, as your patch correctly
enforces. Sorry about the noise.

That said, I think that it might be a good idea to include the above
detail in the documentation of CREATE INDEX and ALTER TABLE ADD UNIQUE.

Thanks,
Amit

On 1/26/18 13:42, Peter Eisentraut wrote:

Other than that, this looks pretty good to me. A logical extension of
the previous partitioned index patch.

Moved to next CF.

Seems close to ready.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services