pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
18+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2019-01-18 4dff8935fb 0325d7a595 Fix creation of duplicate foreign keys on partitions
* BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-10 13:55 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 18+ messages in thread
From: PG Bug reporting form @ 2019-01-10 13:55 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: jesper.pedersen@redhat.com
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-10 17:11 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 2 replies; 18+ messages in thread
From: Alvaro Herrera @ 2019-01-10 17:11 UTC (permalink / raw)
To: jesper.pedersen@redhat.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-10 17:41 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 18+ messages in thread
From: Tom Lane @ 2019-01-10 17:41 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: jesper.pedersen@redhat.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-10 17:43 Jesper Pedersen <jesper.pedersen@redhat.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 18+ messages in thread
From: Jesper Pedersen @ 2019-01-10 17:43 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-15 18:24 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Jesper Pedersen <jesper.pedersen@redhat.com>
0 siblings, 1 reply; 18+ messages in thread
From: Alvaro Herrera @ 2019-01-15 18:24 UTC (permalink / raw)
To: Jesper Pedersen <jesper.pedersen@redhat.com>; +Cc: pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-15 18:26 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 18+ messages in thread
From: Alvaro Herrera @ 2019-01-15 18:26 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: jesper.pedersen@redhat.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-15 18:42 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 18+ messages in thread
From: Tom Lane @ 2019-01-15 18:42 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-15 19:35 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 18+ messages in thread
From: Alvaro Herrera @ 2019-01-15 19:35 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-15 19:45 Jesper Pedersen <jesper.pedersen@redhat.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 0 replies; 18+ messages in thread
From: Jesper Pedersen @ 2019-01-15 19:45 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-15 22:55 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 18+ messages in thread
From: Alvaro Herrera @ 2019-01-15 22:55 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-16 09:30 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 18+ messages in thread
From: Amit Langote @ 2019-01-16 09:30 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
Attachments:
check-exist-FK-constr-in-part-before-recursing.patch (text/plain, 11.0K) download
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 3c960c9423..9016f1f093 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -448,7 +448,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
{
TupleDesc tupdesc;
AttrNumber *attmap;
- List *partFKs;
List *subclone = NIL;
ListCell *cell;
@@ -462,8 +461,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
RelationGetDescr(parentRel),
gettext_noop("could not convert row type"));
- partFKs = copyObject(RelationGetFKeyList(partRel));
-
foreach(cell, clone)
{
Oid parentConstrOid = lfirst_oid(cell);
@@ -476,12 +473,10 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
Oid conppeqop[INDEX_MAX_KEYS];
Oid conffeqop[INDEX_MAX_KEYS];
Constraint *fkconstraint;
- bool attach_it;
Oid constrOid;
ObjectAddress parentAddr,
childAddr;
int nelem;
- ListCell *cell;
int i;
ArrayType *arr;
Datum datum;
@@ -547,20 +542,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
elog(ERROR, "conpfeqop is not a 1-D OID array");
memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid));
- datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop,
- tupdesc, &isnull);
- if (isnull)
- elog(ERROR, "null conpfeqop");
- arr = DatumGetArrayTypeP(datum);
- nelem = ARR_DIMS(arr)[0];
- if (ARR_NDIM(arr) != 1 ||
- nelem < 1 ||
- nelem > INDEX_MAX_KEYS ||
- ARR_HASNULL(arr) ||
- ARR_ELEMTYPE(arr) != OIDOID)
- elog(ERROR, "conpfeqop is not a 1-D OID array");
- memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid));
-
datum = fastgetattr(tuple, Anum_pg_constraint_conppeqop,
tupdesc, &isnull);
if (isnull)
@@ -590,83 +571,12 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
memcpy(conffeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid));
/*
- * Before creating a new constraint, see whether any existing FKs are
- * fit for the purpose. If one is, attach the parent constraint to it,
- * and don't clone anything. This way we avoid the expensive
- * verification step and don't end up with a duplicate FK. This also
- * means we don't consider this constraint when recursing to
- * partitions.
- */
- attach_it = false;
- foreach(cell, partFKs)
- {
- ForeignKeyCacheInfo *fk = lfirst_node(ForeignKeyCacheInfo, cell);
- Form_pg_constraint partConstr;
- HeapTuple partcontup;
-
- attach_it = true;
-
- /*
- * Do some quick & easy initial checks. If any of these fail, we
- * cannot use this constraint, but keep looking.
- */
- if (fk->confrelid != constrForm->confrelid || fk->nkeys != nelem)
- {
- attach_it = false;
- continue;
- }
- for (i = 0; i < nelem; i++)
- {
- if (fk->conkey[i] != mapped_conkey[i] ||
- fk->confkey[i] != confkey[i] ||
- fk->conpfeqop[i] != conpfeqop[i])
- {
- attach_it = false;
- break;
- }
- }
- if (!attach_it)
- continue;
-
- /*
- * Looks good so far; do some more extensive checks. Presumably
- * the check for 'convalidated' could be dropped, since we don't
- * really care about that, but let's be careful for now.
- */
- partcontup = SearchSysCache1(CONSTROID,
- ObjectIdGetDatum(fk->conoid));
- if (!partcontup)
- elog(ERROR, "cache lookup failed for constraint %u",
- fk->conoid);
- partConstr = (Form_pg_constraint) GETSTRUCT(partcontup);
- if (OidIsValid(partConstr->conparentid) ||
- !partConstr->convalidated ||
- partConstr->condeferrable != constrForm->condeferrable ||
- partConstr->condeferred != constrForm->condeferred ||
- partConstr->confupdtype != constrForm->confupdtype ||
- partConstr->confdeltype != constrForm->confdeltype ||
- partConstr->confmatchtype != constrForm->confmatchtype)
- {
- ReleaseSysCache(partcontup);
- attach_it = false;
- continue;
- }
-
- ReleaseSysCache(partcontup);
-
- /* looks good! Attach this constraint */
- ConstraintSetParentConstraint(fk->conoid, constrForm->oid);
- CommandCounterIncrement();
- attach_it = true;
- break;
- }
-
- /*
* If we attached to an existing constraint, there is no need to
* create a new one. In fact, there's no need to recurse for this
* constraint to partitions, either.
*/
- if (attach_it)
+ if (attach_to_existing_FK_constraint(partRel, parentRel,
+ pg_constraint, tuple))
{
ReleaseSysCache(tuple);
continue;
@@ -738,7 +648,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
}
pfree(attmap);
- list_free_deep(partFKs);
/*
* If the partition is partitioned, recurse to handle any constraints that
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d2781cbf19..e828bf600c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7654,9 +7654,16 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
if (recurse && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
PartitionDesc partdesc;
+ Relation pg_constraint;
+ HeapTuple tuple;
partdesc = RelationGetPartitionDesc(rel);
+ tuple = SearchSysCache1(CONSTROID, constrOid);
+ if (!tuple)
+ elog(ERROR, "cache lookup failed for constraint %u", constrOid);
+ pg_constraint = heap_open(ConstraintRelationId, RowShareLock);
+
for (i = 0; i < partdesc->nparts; i++)
{
Oid partitionId = partdesc->oids[i];
@@ -7664,6 +7671,18 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
AlteredTableInfo *childtab;
ObjectAddress childAddr;
+ /*
+ * Try to check if the partition already has a FK constraint
+ * that can be attached to the one being added in the parent
+ * instead of creating a new one for the partition.
+ */
+ if (attach_to_existing_FK_constraint(partition, rel, pg_constraint,
+ tuple))
+ {
+ heap_close(partition, NoLock);
+ continue;
+ }
+
CheckTableNotInUse(partition, "ALTER TABLE");
/* Find or create work queue entry for this table */
@@ -7679,6 +7698,9 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
heap_close(partition, NoLock);
}
+
+ heap_close(pg_constraint, RowShareLock);
+ ReleaseSysCache(tuple);
}
/*
@@ -15163,3 +15185,124 @@ update_relispartition(Relation classRel, Oid relationId, bool newval)
if (opened)
heap_close(classRel, RowExclusiveLock);
}
+
+/*
+ * attach_to_existing_FK_constraint
+ * Try to attach a FK constraint described by 'constrTuple' of the parent
+ * relation 'parentRel' to one of the existing FK constraints of
+ * partition 'partRel'
+ *
+ * Returns true if it succeeds in doing so.
+ */
+
+bool
+attach_to_existing_FK_constraint(Relation partRel, Relation parentRel,
+ Relation pg_constraint, HeapTuple constrTuple)
+{
+ bool attached = false;
+ List *partFKs = copyObject(RelationGetFKeyList(partRel));
+ Form_pg_constraint constrForm;
+ TupleDesc tupdesc = RelationGetDescr(pg_constraint);
+ int nelem,
+ i;
+ AttrNumber conkey[INDEX_MAX_KEYS];
+ AttrNumber mapped_conkey[INDEX_MAX_KEYS];
+ AttrNumber confkey[INDEX_MAX_KEYS];
+ Oid conpfeqop[INDEX_MAX_KEYS];
+ AttrNumber *attmap;
+ ArrayType *arr;
+ Datum datum;
+ bool isnull;
+ ListCell *cell;
+
+ constrForm = (Form_pg_constraint) GETSTRUCT(constrTuple);
+ Assert(constrForm->contype == CONSTRAINT_FOREIGN);
+
+ /*
+ * The constraint key may differ, if the columns in the partition are
+ * different. This map is used to convert them.
+ */
+ attmap = convert_tuples_by_name_map(RelationGetDescr(partRel),
+ RelationGetDescr(parentRel),
+ gettext_noop("could not convert row type"));
+
+ datum = fastgetattr(constrTuple, Anum_pg_constraint_conkey, tupdesc,
+ &isnull);
+ Assert(!isnull);
+ arr = DatumGetArrayTypeP(datum);
+ nelem = ARR_DIMS(arr)[0];
+ memcpy(conkey, ARR_DATA_PTR(arr), nelem * sizeof(AttrNumber));
+ for (i = 0; i < nelem; i++)
+ mapped_conkey[i] = attmap[conkey[i] - 1];
+
+ datum = fastgetattr(constrTuple, Anum_pg_constraint_confkey, tupdesc, &isnull);
+ Assert(!isnull);
+ arr = DatumGetArrayTypeP(datum);
+ memcpy(confkey, ARR_DATA_PTR(arr), nelem * sizeof(AttrNumber));
+
+ datum = fastgetattr(constrTuple, Anum_pg_constraint_conpfeqop, tupdesc, &isnull);
+ Assert(!isnull);
+ arr = DatumGetArrayTypeP(datum);
+ memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid));
+
+ /*
+ * Before creating a new constraint, see whether any existing FKs are
+ * fit for the purpose. If one is, attach the parent constraint to it,
+ * and don't clone anything. This way we avoid the expensive
+ * verification step and don't end up with a duplicate FK. This also
+ * means we don't consider this constraint when recursing to
+ * partitions.
+ */
+ foreach(cell, partFKs)
+ {
+ ForeignKeyCacheInfo *fk = lfirst_node(ForeignKeyCacheInfo, cell);
+ Form_pg_constraint partConstr;
+ HeapTuple partcontup;
+
+ /*
+ * Do some quick & easy initial checks. If any of these fail, we
+ * cannot use this constraint, but keep looking.
+ */
+ if (fk->confrelid != constrForm->confrelid || fk->nkeys != nelem)
+ continue;
+
+ for (i = 0; i < nelem; i++)
+ if (fk->conkey[i] != mapped_conkey[i] ||
+ fk->confkey[i] != confkey[i] ||
+ fk->conpfeqop[i] != conpfeqop[i])
+ continue;
+
+ /*
+ * Looks good so far; do some more extensive checks. Presumably
+ * the check for 'convalidated' could be dropped, since we don't
+ * really care about that, but let's be careful for now.
+ */
+ partcontup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid));
+ if (!partcontup)
+ elog(ERROR, "cache lookup failed for constraint %u", fk->conoid);
+ partConstr = (Form_pg_constraint) GETSTRUCT(partcontup);
+ if (OidIsValid(partConstr->conparentid) ||
+ !partConstr->convalidated ||
+ partConstr->condeferrable != constrForm->condeferrable ||
+ partConstr->condeferred != constrForm->condeferred ||
+ partConstr->confupdtype != constrForm->confupdtype ||
+ partConstr->confdeltype != constrForm->confdeltype ||
+ partConstr->confmatchtype != constrForm->confmatchtype)
+ {
+ ReleaseSysCache(partcontup);
+ continue;
+ }
+
+ ReleaseSysCache(partcontup);
+
+ /* looks good! Attach this constraint */
+ ConstraintSetParentConstraint(fk->conoid, constrForm->oid);
+ CommandCounterIncrement();
+ attached = true;
+ break;
+ }
+
+ list_free_deep(partFKs);
+
+ return attached;
+}
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index ec3bb90b01..9255118814 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -96,5 +96,9 @@ extern void RangeVarCallbackOwnsRelation(const RangeVar *relation,
Oid relId, Oid oldRelId, void *noCatalogs);
extern bool PartConstraintImpliedByRelConstraint(Relation scanrel,
List *partConstraint);
+extern bool attach_to_existing_FK_constraint(Relation partRel,
+ Relation parentRel,
+ Relation pg_constraint,
+ HeapTuple constrTuple);
#endif /* TABLECMDS_H */
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-17 22:23 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
0 siblings, 2 replies; 18+ messages in thread
From: Alvaro Herrera @ 2019-01-17 22:23 UTC (permalink / raw)
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
Attachments:
0001-Refactor-duplicate-code-into-DeconstructFkConstraint.patch (text/x-diff, 0B) download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-18 18:08 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 0 replies; 18+ messages in thread
From: Alvaro Herrera @ 2019-01-18 18:08 UTC (permalink / raw)
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-21 08:01 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 18+ messages in thread
From: Amit Langote @ 2019-01-21 08:01 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-21 16:29 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
0 siblings, 1 reply; 18+ messages in thread
From: Alvaro Herrera @ 2019-01-21 16:29 UTC (permalink / raw)
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-22 08:00 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 18+ messages in thread
From: Amit Langote @ 2019-01-22 08:00 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-23 21:07 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
0 siblings, 1 reply; 18+ messages in thread
From: Alvaro Herrera @ 2019-01-23 21:07 UTC (permalink / raw)
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT
@ 2019-01-24 01:59 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 0 replies; 18+ messages in thread
From: Amit Langote @ 2019-01-24 01:59 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jesper Pedersen <jesper.pedersen@redhat.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2019-01-24 01:59 UTC | newest]
Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-01-10 13:55 BUG #15587: Partitions with ALTER TABLE ADD CONSTRAINT PG Bug reporting form <noreply@postgresql.org>
2019-01-10 17:11 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-10 17:41 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-01-15 18:26 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-10 17:43 ` Jesper Pedersen <jesper.pedersen@redhat.com>
2019-01-15 18:24 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-15 18:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-01-15 19:35 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-15 19:45 ` Jesper Pedersen <jesper.pedersen@redhat.com>
2019-01-15 22:55 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-16 09:30 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2019-01-17 22:23 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-18 18:08 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-21 08:01 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2019-01-21 16:29 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-22 08:00 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2019-01-23 21:07 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-24 01:59 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox