BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

PG Doc comments form
The following bug has been logged on the website:

Bug reference:      15587
Logged by:          Jesper Pedersen
Email address:      [hidden email]
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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Tom Lane-2
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Jesper Pedersen
In reply to this post by Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Alvaro Herrera-9
In reply to this post by Tom Lane-2
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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Tom Lane-2
In reply to this post by Alvaro Herrera-9
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Alvaro Herrera-9
On 2019-Jan-15, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Jesper Pedersen
Hi,

On 1/15/19 2:35 PM, Alvaro Herrera wrote:

> On 2019-Jan-15, Tom Lane wrote:
>> Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Alvaro Herrera-9
In reply to this post by Alvaro Herrera-9
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@...
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@...
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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

Amit Langote-2
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@...
> 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

check-exist-FK-constr-in-part-before-recursing.patch (15K) Download Attachment