Thread: unique indexes on partitioned tables
This is the patch series for UNIQUE / PRIMARY KEY indexes on partitioned tables. This is on top of the patch in https://postgr.es/m/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. 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. This serves as basis to build foreign keys on top; I'll post that separately. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
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
Attachment
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 > https://postgr.es/m/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
Attachment
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
Attachment
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
Attachment
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
Hello,
Thanks, Peter, Jesper, Amit, for reviewing the patch. Replying to
all review comments at once:
Jesper Pedersen wrote:
> Maybe add a test case to indexing.sql that highlights that hash indexes
> doesn't support UNIQUE; although not unique to partitioned indexes.
I'm not sure about this. If one day unique is supported by hash, why
would this test have to be modified? Other than to add a few relevant
test cases, that is! Lack of support is already tested elsewhere (one
hopes).
Peter Eisentraut wrote:
> + 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?)
Ooh, great catch! Fixed.
> +alter table idxpart add primary key (a); -- not an incomplete one tho
>
> "though"?
Fixed :-)
> 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.
Added.
Amit Langote said:
> 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.
Added some text there.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Here is a mini-patch on top of yours to fix a few cosmetic things. I don't understand the variable name "third". I don't see a "first" or "second" nearby. I find some of the columns in pg_constraint confusing. For a primary key on a partitioned table, for the PK on the partition I get conislocal = false, coninhcount = 1, connoinherit = true The last part is confusing to me. I don't know if that's really on this patch. But perhaps it could be documented better. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 12 February 2018 at 15:26, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Hello, > > Thanks, Peter, Jesper, Amit, for reviewing the patch. Replying to > all review comments at once: > [... v5 of patch attached ...] Hi Álvaro, attached a tiny patch (on top of yours) that silence two "variables uninitilized" warnings. also noted that if you: """ create table t1(i int) partition by hash (i); create table t1_0 partition of t1 for values with (modulus 2, remainder 0); create table t1_1 partition of t1 for values with (modulus 2, remainder 1); create unique index on t1(i); alter table t1 add primary key using index t1_i_idx ; """ the ALTER TABLE ADD PK does not recurse to partitions, which maybe is perfectly fine because i'm using USING INDEX but it feels like an oversight to me -- Jaime Casanova www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Jaime Casanova wrote: > Hi Álvaro, > > attached a tiny patch (on top of yours) that silence two "variables > uninitilized" warnings. Thanks! Applied. > also noted that if you: > > """ > create table t1(i int) partition by hash (i); > create table t1_0 partition of t1 for values with (modulus 2, remainder 0); > create table t1_1 partition of t1 for values with (modulus 2, remainder 1); > create unique index on t1(i); > alter table t1 add primary key using index t1_i_idx ; > """ > > the ALTER TABLE ADD PK does not recurse to partitions, which maybe is > perfectly fine because i'm using USING INDEX but it feels like an > oversight to me Ouch. Yeah, this is a bug. I'll try to come up with something. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
I pushed this now, with fixes for the last few comments there were. Peter Eisentraut wrote: > I don't understand the variable name "third". I don't see a "first" or > "second" nearby. Hah. That was referring to variables "myself" and "referenced". I changed the variable name to "parentConstr". > I find some of the columns in pg_constraint confusing. For a primary > key on a partitioned table, for the PK on the partition I get > > conislocal = false, coninhcount = 1, connoinherit = true > > The last part is confusing to me. Yeah, I think it was patently wrong. I changed it so that connoinherit becomes true in this case. Alvaro Herrera wrote: > Jaime Casanova wrote: > > also noted that if you: > > > > """ > > create table t1(i int) partition by hash (i); > > create table t1_0 partition of t1 for values with (modulus 2, remainder 0); > > create table t1_1 partition of t1 for values with (modulus 2, remainder 1); > > create unique index on t1(i); > > alter table t1 add primary key using index t1_i_idx ; > > """ > > > > the ALTER TABLE ADD PK does not recurse to partitions, which maybe is > > perfectly fine because i'm using USING INDEX but it feels like an > > oversight to me > > Ouch. Yeah, this is a bug. I'll try to come up with something. After looking at it for a few minutes I determined that adding this feature requires some more work: you need to iterate on all partitions, obtain the corresponding index, cons up a few fake parse nodes, then recurse to create the PK in the children. I think this should be doable with a couple dozen lines of code, but it's a refinement that can be added on top. Care to submit a patch? In the meantime, I added an ereport(ERROR) to avoid leaving the system in an inconsistent state (that probably would not even be reproduced correctly by pg_dump). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi. On 2018/02/20 5:45, Alvaro Herrera wrote: > I pushed this now, with fixes for the last few comments there were. I noticed with the commit that, while ON CONFLICT (conflict_target) DO UPDATE gives a less surprising error message by catching it in the parser, ON CONFLICT (conflict_target) DO NOTHING will go into the executor without the necessary code to handle the case. Example: create table p (a int primary key, b text) partition by list (a); create table p12 partition of p for values in (1, 2); create table p3 partition of p (a unique) for values in (3); insert into p values (1, 'a') on conflict (a) do nothing; ERROR: unexpected failure to find arbiter index Attached is a patch to fix that. Actually, there are two -- one that adjusts the partitioned table tests in insert_conflict.sql to have a partitioned unique index and another that fixes the code. I suppose we'd need to apply this temporarily until we fix the ON CONFLICT (conflict_target) case to be able to use partitioned indexes. Thanks, Amit
Attachment
Hi.
I tried this feature with the latest snapshot. When I executed the following SQL statement, multiple primary keys were
createdon the partition.
Is this the intended behavior?
-- test
postgres=> CREATE TABLE part1(c1 INT PRIMARY KEY, c2 INT, c3 VARCHAR(10)) PARTITION BY RANGE(c1) ;
CREATE TABLE
postgres=> CREATE TABLE part1v1 (LIKE part1) ;
CREATE TABLE
postgres=> ALTER TABLE part1v1 ADD CONSTRAINT pk_part1v1 PRIMARY KEY (c1, c2) ;
ALTER TABLE
postgres=> ALTER TABLE part1 ATTACH PARTITION part1v1 FOR VALUES FROM (100) TO (200) ;
ALTER TABLE
postgres=> \d part1v1
Table "public.part1v1"
Column | Type | Collation | Nullable | Default
--------+-----------------------+-----------+----------+---------
c1 | integer | | not null |
c2 | integer | | not null |
c3 | character varying(10) | | |
Partition of: part1 FOR VALUES FROM (100) TO (200)
Indexes:
"part1v1_pkey" PRIMARY KEY, btree (c1)
"pk_part1v1" PRIMARY KEY, btree (c1, c2)
Regards,
Noriyoshi Shinoda
-----Original Message-----
From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]
Sent: Tuesday, February 20, 2018 6:24 PM
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Jaime Casanova
<jaime.casanova@2ndquadrant.com>
Cc: Jesper Pedersen <jesper.pedersen@redhat.com>; Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: unique indexes on partitioned tables
Hi.
On 2018/02/20 5:45, Alvaro Herrera wrote:
> I pushed this now, with fixes for the last few comments there were.
I noticed with the commit that, while ON CONFLICT (conflict_target) DO UPDATE gives a less surprising error message by
catchingit in the parser, ON CONFLICT (conflict_target) DO NOTHING will go into the executor without the necessary code
tohandle the case. Example:
create table p (a int primary key, b text) partition by list (a); create table p12 partition of p for values in (1, 2);
createtable p3 partition of p (a unique) for values in (3);
insert into p values (1, 'a') on conflict (a) do nothing;
ERROR: unexpected failure to find arbiter index
Attached is a patch to fix that. Actually, there are two -- one that adjusts the partitioned table tests in
insert_conflict.sqlto have a partitioned unique index and another that fixes the code.
I suppose we'd need to apply this temporarily until we fix the ON CONFLICT
(conflict_target) case to be able to use partitioned indexes.
Thanks,
Amit
Hi, Shinoda, Noriyoshi wrote: > I tried this feature with the latest snapshot. When I executed the > following SQL statement, multiple primary keys were created on the > partition. Is this the intended behavior? Hahah. Is that a serious question? Of course it isn't. I'll fix it: > -- test > postgres=> CREATE TABLE part1(c1 INT PRIMARY KEY, c2 INT, c3 VARCHAR(10)) PARTITION BY RANGE(c1) ; > CREATE TABLE > postgres=> CREATE TABLE part1v1 (LIKE part1) ; > CREATE TABLE > postgres=> ALTER TABLE part1v1 ADD CONSTRAINT pk_part1v1 PRIMARY KEY (c1, c2) ; > ALTER TABLE > postgres=> ALTER TABLE part1 ATTACH PARTITION part1v1 FOR VALUES FROM (100) TO (200) ; > ALTER TABLE I think the correct behavior here is to error out the ATTACH PARTITION indicating that a primary key already exist. Thanks for pointing this out. Please keep testing, -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Shinoda, Noriyoshi wrote: Hi, > I tried this feature with the latest snapshot. When I executed the > following SQL statement, multiple primary keys were created on the > partition. > Is this the intended behavior? It turns out that the error check for duplicate PKs is only invoked if you tell this code that it's being invoked by ALTER TABLE, and my original patch wasn't. I changed it and now everything seems to behave as expected. I added a test case pretty much like yours, which now works correctly. I also added another one where the bogus PK is two levels down rather than one. This is because I had originally developed a different fix -- which fixed the problem for your test case, until I realized that since this code is recursive, we could cause trouble at a distance. Thanks for reporting the problem -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Álvaro, Thank you for your developing the new patch. I will continue testing. -----Original Message----- From: Alvaro Herrera [mailto:alvherre@2ndquadrant.com] Sent: Tuesday, March 13, 2018 7:51 AM To: Shinoda, Noriyoshi <noriyoshi.shinoda@hpe.com> Cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Jaime Casanova <jaime.casanova@2ndquadrant.com>;Jesper Pedersen <jesper.pedersen@redhat.com>; Pg Hackers <pgsql-hackers@postgresql.org> Subject: Re: unique indexes on partitioned tables Shinoda, Noriyoshi wrote: Hi, > I tried this feature with the latest snapshot. When I executed the > following SQL statement, multiple primary keys were created on the > partition. > Is this the intended behavior? It turns out that the error check for duplicate PKs is only invoked if you tell this code that it's being invoked by ALTERTABLE, and my original patch wasn't. I changed it and now everything seems to behave as expected. I added a test case pretty much like yours, which now works correctly. I also added another one where the bogus PK is two levels down rather than one. This is because I had originally developeda different fix -- which fixed the problem for your test case, until I realized that since this code is recursive,we could cause trouble at a distance. Thanks for reporting the problem -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services