Thread: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
The following bug has been logged on the website:
Bug reference: 15587
Logged by: Jesper Pedersen
Email address: jesper.pedersen@redhat.com
PostgreSQL version: 11.1
Operating system: Fedora 28
Description:
Hi,
The following works
CREATE TABLE t1 (i1 INT NOT NULL, i2 INT NOT NULL) PARTITION BY HASH (i1);
\o /dev/null
SELECT 'CREATE TABLE t1_p' || x::text || ' PARTITION OF t1
FOR VALUES WITH (MODULUS 64, REMAINDER ' || x::text || ');'
from generate_series(0,63) x;
\gexec
\o
ALTER TABLE ONLY t1 ADD CONSTRAINT uniq_t1_i1_i2 UNIQUE (i1, i2);
which gives
test=# \d+ t1
Partitioned table "public.t1"
Column | Type | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
i1 | integer | | not null | | plain |
|
i2 | integer | | not null | | plain |
|
Partition key: HASH (i1)
Indexes:
"uniq_t1_i1_i2" UNIQUE CONSTRAINT, btree (i1, i2) INVALID
Removing ONLY from the ALTER command makes the index correct.
All branches.
Best regards,
Jesper
On 2019-Jan-10, PG Bug reporting form wrote: > The following works > > CREATE TABLE t1 (i1 INT NOT NULL, i2 INT NOT NULL) PARTITION BY HASH (i1); > > \o /dev/null > SELECT 'CREATE TABLE t1_p' || x::text || ' PARTITION OF t1 > FOR VALUES WITH (MODULUS 64, REMAINDER ' || x::text || ');' > from generate_series(0,63) x; > \gexec > \o > > ALTER TABLE ONLY t1 ADD CONSTRAINT uniq_t1_i1_i2 UNIQUE (i1, i2); > > which gives > > test=# \d+ t1 > Partitioned table "public.t1" > Column | Type | Collation | Nullable | Default | Storage | Stats target > | Description > --------+---------+-----------+----------+---------+---------+--------------+------------- > i1 | integer | | not null | | plain | > | > i2 | integer | | not null | | plain | > | > Partition key: HASH (i1) > Indexes: > "uniq_t1_i1_i2" UNIQUE CONSTRAINT, btree (i1, i2) INVALID > > > Removing ONLY from the ALTER command makes the index correct. I'm not clear what problem you're reporting. If you use ONLY, then the command doesn't cascade to create the index on partitions, and the index is marked invalid. If you add the constraint to each partition and ALTER INDEX ATTACH PARTITION, the index on t1 should become valid when every partition of the table has its index. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-10, PG Bug reporting form wrote:
>> ALTER TABLE ONLY t1 ADD CONSTRAINT uniq_t1_i1_i2 UNIQUE (i1, i2);
>> [ leads to ]
>> Indexes:
>> "uniq_t1_i1_i2" UNIQUE CONSTRAINT, btree (i1, i2) INVALID
> I'm not clear what problem you're reporting. If you use ONLY, then the
> command doesn't cascade to create the index on partitions, and the index
> is marked invalid. If you add the constraint to each partition and
> ALTER INDEX ATTACH PARTITION, the index on t1 should become valid when
> every partition of the table has its index.
I concur that the code is operating as designed. I think however that
there's a user-experience problem here, which is that INVALID suggests
that something's broken. I wonder if we could improve matters by
making psql and the docs describe this state of a partitioned index
as INCOMPLETE.
regards, tom lane
Hi, On 1/10/19 12:11 PM, Alvaro Herrera wrote: > On 2019-Jan-10, PG Bug reporting form wrote: >> Removing ONLY from the ALTER command makes the index correct. > > I'm not clear what problem you're reporting. If you use ONLY, then the > command doesn't cascade to create the index on partitions, and the index > is marked invalid. If you add the constraint to each partition and > ALTER INDEX ATTACH PARTITION, the index on t1 should become valid when > every partition of the table has its index. > However, when you use ADD CONSTRAINT FOREIGN KEY you can't use ONLY, so would it be a good idea to make ADD CONSTRAINT UNIQUE behave the same way, i.e. error out ? Best regards, Jesper
On 2019-Jan-10, Jesper Pedersen wrote: > On 1/10/19 12:11 PM, Alvaro Herrera wrote: > > On 2019-Jan-10, PG Bug reporting form wrote: > > > Removing ONLY from the ALTER command makes the index correct. > > > > I'm not clear what problem you're reporting. If you use ONLY, then the > > command doesn't cascade to create the index on partitions, and the index > > is marked invalid. If you add the constraint to each partition and > > ALTER INDEX ATTACH PARTITION, the index on t1 should become valid when > > every partition of the table has its index. > > However, when you use ADD CONSTRAINT FOREIGN KEY you can't use ONLY, so > would it be a good idea to make ADD CONSTRAINT UNIQUE behave the same way, > i.e. error out ? I haven't investigated this angle. It seems more complex than just a simple bugfix, right? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Jan-10, Tom Lane wrote: > > On 2019-Jan-10, PG Bug reporting form wrote: > >> ALTER TABLE ONLY t1 ADD CONSTRAINT uniq_t1_i1_i2 UNIQUE (i1, i2); > >> [ leads to ] > >> Indexes: > >> "uniq_t1_i1_i2" UNIQUE CONSTRAINT, btree (i1, i2) INVALID > I concur that the code is operating as designed. I think however that > there's a user-experience problem here, which is that INVALID suggests > that something's broken. I wonder if we could improve matters by > making psql and the docs describe this state of a partitioned index > as INCOMPLETE. Hmm, yeah, I can see how that can be confusing. Not sure of the difficulty of a fix ... I'll have a think about it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-10, Jesper Pedersen wrote:
>> However, when you use ADD CONSTRAINT FOREIGN KEY you can't use ONLY, so
>> would it be a good idea to make ADD CONSTRAINT UNIQUE behave the same way,
>> i.e. error out ?
> I haven't investigated this angle. It seems more complex than just a
> simple bugfix, right?
Wouldn't that be throwing away the entire point of the ONLY behavior,
ie to allow the component indexes to be built one at a time, without
holding locks across the whole partition tree?
I'm having a hard time getting from "ONLY confuses me" to "nobody
should be allowed to do this". I think there is a documentation
and UX issue here, but I don't see that there's anything wrong
with the functionality.
regards, tom lane
On 2019-Jan-15, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Jan-10, Jesper Pedersen wrote:
> >> However, when you use ADD CONSTRAINT FOREIGN KEY you can't use ONLY, so
> >> would it be a good idea to make ADD CONSTRAINT UNIQUE behave the same way,
> >> i.e. error out ?
>
> > I haven't investigated this angle. It seems more complex than just a
> > simple bugfix, right?
>
> Wouldn't that be throwing away the entire point of the ONLY behavior,
> ie to allow the component indexes to be built one at a time, without
> holding locks across the whole partition tree?
I now see that Jesper was talking about a completely different thing
than I was thinking. I agree with you there -- it makes no sense to
reject that command ... particularly because pg_dump uses it.
What was on my head ("can we add ONLY to ADD FOREIGN KEY?") was the idea
that it'd be useful to construct the foreign keys in partitions, one by
one, and as a final step you construct a foreign key in the partitioned
table and then attach each FK in partition to the master one. Right
now, adding the foreign key in the parent table just creates duplicates
in the partitions, which is silly.
create table foo (a int primary key);
create table barp (a int) partition by list (a);
create table barp1 partition of barp for values in (1);
create table barp2 partition of barp for values in (2);
alter table barp1 add foreign key (a) references foo;
alter table barp2 add foreign key (a) references foo;
At this point, the partitions have one FK each, and it would be neat to
merge them as a unit, creating a parent constraint. But if you do:
alter table barp add foreign key (a) references foo;
you end up with two FKs in each partition.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 1/15/19 2:35 PM, Alvaro Herrera wrote: > On 2019-Jan-15, Tom Lane wrote: >> Alvaro Herrera <alvherre@2ndquadrant.com> writes: >>> I haven't investigated this angle. It seems more complex than just a >>> simple bugfix, right? >> >> Wouldn't that be throwing away the entire point of the ONLY behavior, >> ie to allow the component indexes to be built one at a time, without >> holding locks across the whole partition tree? > > I now see that Jesper was talking about a completely different thing > than I was thinking. I agree with you there -- it makes no sense to > reject that command ... particularly because pg_dump uses it. > I now think Tom is correct that it is UX and documentation issue, and changing the existing behavior is probably not a good thing. Changing "invalid" to "incomplete" would be a good idea. Maybe "partial" could be a good descriptor if not all partitions shares the unique constraint. Best regards, Jesper
On 2019-Jan-15, Alvaro Herrera wrote:
> What was on my head ("can we add ONLY to ADD FOREIGN KEY?") was the idea
> that it'd be useful to construct the foreign keys in partitions, one by
> one, and as a final step you construct a foreign key in the partitioned
> table and then attach each FK in partition to the master one. Right
> now, adding the foreign key in the parent table just creates duplicates
> in the partitions, which is silly.
I had put this aside and started reviewing Amit's patch 0002 here
https://postgr.es/m/f2b8ead5-4131-d5a8-8016-2ea0a31250af@lab.ntt.co.jp
when I realized that this is already implemented ... for the case where
we attach a new partition, and the new partition already contains the
constraint. The case of creating a constraint from scratch is just
doing the recursion badly and not checking for pre-existing matching
constraints, which is why it ends up with a dupe. Fixing it is pretty
simple -- we just need to call clone_fk_constraints() with only the
constraint being created, and everything works correctly as far as I can
tell.
The only issue is that clone_fk_constraints is a static function in
pg_constraint.c, so I'd have to export it for use in tablecmds.c ... or
I could just apply patch 0002 that I posted here
https://www.postgresql.org/message-id/20181130191216.5xcxcsx3ascgqayv@alvherre.pgsql
which takes care precisely of moving that function to tablecmds.c (with
a better name, too).
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019/01/16 7:55, Alvaro Herrera wrote:
> On 2019-Jan-15, Alvaro Herrera wrote:
>
>> What was on my head ("can we add ONLY to ADD FOREIGN KEY?") was the idea
>> that it'd be useful to construct the foreign keys in partitions, one by
>> one, and as a final step you construct a foreign key in the partitioned
>> table and then attach each FK in partition to the master one. Right
>> now, adding the foreign key in the parent table just creates duplicates
>> in the partitions, which is silly.
>
> I had put this aside and started reviewing Amit's patch 0002 here
> https://postgr.es/m/f2b8ead5-4131-d5a8-8016-2ea0a31250af@lab.ntt.co.jp
> when I realized that this is already implemented ... for the case where
> we attach a new partition, and the new partition already contains the
> constraint. The case of creating a constraint from scratch is just
> doing the recursion badly and not checking for pre-existing matching
> constraints, which is why it ends up with a dupe.
Yeah, that seems to be the problem.
> Fixing it is pretty
> simple -- we just need to call clone_fk_constraints() with only the
> constraint being created, and everything works correctly as far as I can
> tell.
Why not just move the code in clone_fk_constraints() that checks if the
constraint equivalent of the parent's constraint is present in the
partition and simply attach the two without creating a new copy for the
partition to a new function in tablecmds.c and call the function from both
clone_fk_constraints() and ATAddForeignKeyConstraint()? Attached is what
I'm thinking.
Thanks,
Amit
Attachment
On 2019-Jan-16, Amit Langote wrote: > Why not just move the code in clone_fk_constraints() that checks if the > constraint equivalent of the parent's constraint is present in the > partition and simply attach the two without creating a new copy for the > partition to a new function in tablecmds.c and call the function from both > clone_fk_constraints() and ATAddForeignKeyConstraint()? Attached is what > I'm thinking. Well, the whole point of my proposal is that the FK-creating code recurses to partitions by calling ATAddForeignKeyConstraint, and IMO that's the wrong level to recurse at; we should instead recurse by calling clone_fk_constraints() as a whole. That's not readibly possible with the current code arrangement because of layering, but after backpatching (as attached) the two patches I mentioned, I end up with the following, which I think is much cleaner. (Also, this code layout plays much better with my project to continue to extend FKs so that they are allowed to point to partitioned tables; see the other thread). The attached patches are for pg11; they don't apply to master. The changes are uninteresting. The tests added by the final commit clearly show dupe FKs being created in some of the cases, if you run them without applying the code fixes, and none afterwards. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 2019-Jan-17, Alvaro Herrera wrote: > Well, the whole point of my proposal is that the FK-creating code > recurses to partitions by calling ATAddForeignKeyConstraint, and IMO > that's the wrong level to recurse at; we should instead recurse by > calling clone_fk_constraints() as a whole. That's not readibly possible > with the current code arrangement because of layering, but after > backpatching (as attached) the two patches I mentioned, I end up with > the following, which I think is much cleaner. (Also, this code layout > plays much better with my project to continue to extend FKs so that they > are allowed to point to partitioned tables; see the other thread). > > The attached patches are for pg11; they don't apply to master. The > changes are uninteresting. Pushed this. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019/01/18 7:23, Alvaro Herrera wrote: > On 2019-Jan-16, Amit Langote wrote: > >> Why not just move the code in clone_fk_constraints() that checks if the >> constraint equivalent of the parent's constraint is present in the >> partition and simply attach the two without creating a new copy for the >> partition to a new function in tablecmds.c and call the function from both >> clone_fk_constraints() and ATAddForeignKeyConstraint()? Attached is what >> I'm thinking. > > Well, the whole point of my proposal is that the FK-creating code > recurses to partitions by calling ATAddForeignKeyConstraint, and IMO > that's the wrong level to recurse at; we should instead recurse by > calling clone_fk_constraints() as a whole. Sorry about the noise. I agree with the committed approach. With this, ALTER TABLE ADD FOREIGN KEY's inheritance recursion path now looks completely different from ALTER TABLE ADD CHECK's, but that's fine. Actually, if we had the same "clone" approach for check constraints, which both checks if a child already has the constraint being cloned and creates one if not, we could do away with errors like the following: create table p (a int, constraint check_a check (a > 0)) partition by list create table p1 (a int); alter table p attach partition p1 for values in (1); ERROR: child table is missing constraint "check_a" But of course that would be a different feature. Thanks, Amit
On 2019-Jan-21, Amit Langote wrote: > Sorry about the noise. I agree with the committed approach. Great, thanks for checking. > With this, > ALTER TABLE ADD FOREIGN KEY's inheritance recursion path now looks > completely different from ALTER TABLE ADD CHECK's, but that's fine. > Actually, if we had the same "clone" approach for check constraints, which > both checks if a child already has the constraint being cloned and creates > one if not, we could do away with errors like the following: > > create table p (a int, constraint check_a check (a > 0)) partition by list > create table p1 (a int); > alter table p attach partition p1 for values in (1); > ERROR: child table is missing constraint "check_a" > > But of course that would be a different feature. Heh, I wasn't aware that this failed in this silly way. But yeah, that's a different feature and we would certainly not backpatch a fix for it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019/01/22 1:29, Alvaro Herrera wrote: > On 2019-Jan-21, Amit Langote wrote: >> With this, >> ALTER TABLE ADD FOREIGN KEY's inheritance recursion path now looks >> completely different from ALTER TABLE ADD CHECK's, but that's fine. >> Actually, if we had the same "clone" approach for check constraints, which >> both checks if a child already has the constraint being cloned and creates >> one if not, we could do away with errors like the following: >> >> create table p (a int, constraint check_a check (a > 0)) partition by list >> create table p1 (a int); >> alter table p attach partition p1 for values in (1); >> ERROR: child table is missing constraint "check_a" >> >> But of course that would be a different feature. > > Heh, I wasn't aware that this failed in this silly way. But yeah, > that's a different feature and we would certainly not backpatch a fix > for it. Are you be willing to try to fix that in HEAD if someone sends a patch? :) Thanks, Amit
Hello > Are you be willing to try to fix that in HEAD if someone sends a patch? :) Well, we can discuss it in a new thread I suppose. Do we need non-inheritable constraints for that case? I think not, but maybe I'm wrong. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019/01/24 6:07, Alvaro Herrera wrote: > Hello > >> Are you be willing to try to fix that in HEAD if someone sends a patch? :) > > Well, we can discuss it in a new thread I suppose. Sure, a new thread on -hackers. > Do we need > non-inheritable constraints for that case? I think not, but maybe I'm > wrong. Defining non-inheritable constraints on *partitioned* parent tables is disallowed because they would never be checked and so pointless. I'm not sure if we'd want to include the regular inheritance case (ALTER TABLE child INHERIT new_parent) in this new development, but if we do, we'll have to arrange to skip cloning parent's non-inheritable constraints. Thanks, Amit