BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT

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
/messages/by-id/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
/messages/by-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
/messages/by-id/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

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

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