Thread: problems with foreign keys on partitioned tables
Hi, I noticed a couple of problems with foreign keys on partitioned tables. 1. Foreign keys of partitions stop working correctly after being detached from the parent table create table pk (a int primary key); create table p (a int) partition by list (a); create table p1 partition of p for values in (1) partition by list (a); create table p11 partition of p1 for values in (1); alter table p add foreign key (a) references pk (a); -- these things work correctly insert into p values (1); ERROR: insert or update on table "p11" violates foreign key constraint "p_a_fkey" DETAIL: Key (a)=(1) is not present in table "pk". insert into pk values (1); insert into p values (1); delete from pk where a = 1; ERROR: update or delete on table "pk" violates foreign key constraint "p_a_fkey" on table "p" DETAIL: Key (a)=(1) is still referenced from table "p". -- detach p1, which preserves the foreign key key alter table p detach partition p1; create table p12 partition of p1 for values in (2); -- this part of the foreign key on p1 still works insert into p1 values (2); ERROR: insert or update on table "p12" violates foreign key constraint "p_a_fkey" DETAIL: Key (a)=(2) is not present in table "pk". -- but this seems wrong delete from pk where a = 1; DELETE 1 -- because select * from p1; a ─── 1 (1 row) This happens because the action triggers defined on the PK relation (pk) refers to p as the referencing relation. On detaching p1 from p, p1's data is no longer accessible to that trigger. To fix this problem, we need create action triggers on PK relation that refer to p1 when it's detached (unless such triggers already exist which might be true in some cases). Attached patch 0001 shows this approach. 2. Foreign keys of a partition cannot be dropped in some cases after detaching it from the parent. create table p (a int references pk) partition by list (a); create table p1 partition of p for values in (1) partition by list (a); create table p11 partition of p1 for values in (1); alter table p detach partition p1; -- p1's foreign key is no longer inherited, so should be able to drop it alter table p1 drop constraint p_a_fkey ; ERROR: constraint "p_a_fkey" of relation "p11" does not exist This happens because by the time ATExecDropConstraint tries to recursively drop the p11's inherited foreign key constraint (which is what normally happens for inherited constraints), the latter has already been dropped by dependency management. I think the foreign key inheritance related code doesn't need to add dependencies for something that inheritance recursion can take of and I can't think of any other reason to have such dependencies around. I thought maybe they're needed for pg_dump to work correctly, but apparently not so. Interestingly, the above problem doesn't occur if the constraint is added to partitions by inheritance recursion. create table p (a int) partition by list (a); create table p1 partition of p for values in (1) partition by list (a); create table p11 partition of p1 for values in (1); alter table p add foreign key (a) references pk (a); alter table p detach partition p1; alter table p1 drop constraint p_a_fkey ; ALTER TABLE Looking into it, that happens to work *accidentally*. ATExecDropInherit() doesn't try to recurse, which prevents the error in this case, because it finds that the constraint on p1 is marked NO INHERIT (non-inheritable), which is incorrect. The value of p1's constraint's connoinherit (in fact, other inheritance related properties too) is incorrect, because ATAddForeignKeyConstraint doesn't bother to set them correctly. This is what the inheritance properties of various copies of 'p_a_fkey' looks like in the catalog in this case: -- case 1: foreign key added to partitions recursively create table p (a int) partition by list (a); create table p1 partition of p for values in (1) partition by list (a); create table p11 partition of p1 for values in (1); alter table p add foreign key (a) references pk (a); select conname, conrelid::regclass, conislocal, coninhcount, connoinherit from pg_constraint where conname like 'p%fkey%'; conname │ conrelid │ conislocal │ coninhcount │ connoinherit ──────────┼──────────┼────────────┼─────────────┼────────────── p_a_fkey │ p │ t │ 0 │ t p_a_fkey │ p1 │ t │ 0 │ t p_a_fkey │ p11 │ t │ 0 │ t (3 rows) In this case, after detaching p1 from p, p1's foreign key's coninhcount turns to -1, which is not good. alter table p detach partition p1; select conname, conrelid::regclass, conislocal, coninhcount, connoinherit from pg_constraint where conname like 'p%fkey%'; conname │ conrelid │ conislocal │ coninhcount │ connoinherit ──────────┼──────────┼────────────┼─────────────┼────────────── p_a_fkey │ p │ t │ 0 │ t p_a_fkey │ p11 │ t │ 0 │ t p_a_fkey │ p1 │ t │ -1 │ t (3 rows) -- case 2: foreign keys cloned to partitions after adding partitions create table p (a int references pk) partition by list (a); create table p1 partition of p for values in (1) partition by list (a); create table p11 partition of p1 for values in (1); select conname, conrelid::regclass, conislocal, coninhcount, connoinherit from pg_constraint where conname like 'p%fkey%'; conname │ conrelid │ conislocal │ coninhcount │ connoinherit ──────────┼──────────┼────────────┼─────────────┼────────────── p_a_fkey │ p │ t │ 0 │ t p_a_fkey │ p1 │ f │ 1 │ f p_a_fkey │ p11 │ f │ 1 │ f (3 rows) Anyway, I propose we fix this by first getting rid of dependencies for foreign key constraints and instead rely on inheritance recursion for dropping the inherited constraints. Before we can do that, we'll need to consistently set the inheritance properties of foreign key constraints correctly, that is, teach ATAddForeignKeyConstraint what clone_fk_constraints already does correctly. See the attached patch 0002 for that. I'm also attaching versions of 0001 and 0002 that can be applied to PG 11. Thanks, Amit
Attachment
Hi Amit On 2019-Jan-09, Amit Langote wrote: > I noticed a couple of problems with foreign keys on partitioned tables. Ouch, thanks for reporting. I think 0001 needs a bit of a tweak in pg11 to avoid an ABI break -- I intend to study this one and try to push early next week. I'm going to see about pushing 0002 shortly, adding some of your tests. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Jan-09, Amit Langote wrote: > 1. Foreign keys of partitions stop working correctly after being detached > from the parent table > This happens because the action triggers defined on the PK relation (pk) > refers to p as the referencing relation. On detaching p1 from p, p1's > data is no longer accessible to that trigger. Ouch. > To fix this problem, we need create action triggers on PK relation > that refer to p1 when it's detached (unless such triggers already > exist which might be true in some cases). Attached patch 0001 shows > this approach. Hmm, okay. I'm not in love with the idea that such triggers might already exist -- seems unclean. We should remove redundant action triggers when we attach a table as a partition, no? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019/01/18 7:54, Alvaro Herrera wrote:
> On 2019-Jan-09, Amit Langote wrote:
>
>> 1. Foreign keys of partitions stop working correctly after being detached
>> from the parent table
>
>> This happens because the action triggers defined on the PK relation (pk)
>> refers to p as the referencing relation. On detaching p1 from p, p1's
>> data is no longer accessible to that trigger.
>
> Ouch.
>
>> To fix this problem, we need create action triggers on PK relation
>> that refer to p1 when it's detached (unless such triggers already
>> exist which might be true in some cases). Attached patch 0001 shows
>> this approach.
>
> Hmm, okay. I'm not in love with the idea that such triggers might
> already exist -- seems unclean. We should remove redundant action
> triggers when we attach a table as a partition, no?
OK, I agree. I have updated the patch to make things work that way. With
the patch:
create table pk (a int primary key);
create table p (a int references pk) partition by list (a);
-- this query shows the action triggers on the referenced rel ('pk'), name
-- of the constraint that the trigger is part of and the foreign key rel
-- ('p', etc.)
select tgrelid::regclass as pkrel, c.conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
pkrel │ fkconname │ fkrel
───────┼───────────┼───────
pk │ p_a_fkey │ p
pk │ p_a_fkey │ p
(2 rows)
create table p1 (
a int references pk,
foreign key (a) references pk (a) ON UPDATE CASCADE ON DELETE CASCADE
DEFERRABLE,
foreign key (a) references pk (a) MATCH FULL ON UPDATE CASCADE ON
DELETE CASCADE
) partition by list (a);
-- p1_a_fkey on 'p1' is equivalent to p_a_fkey on 'p', but they're not
-- attached yet
select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
pkrel │ fkconname │ fkrel
───────┼────────────┼───────
pk │ p_a_fkey │ p
pk │ p_a_fkey │ p
pk │ p1_a_fkey │ p1
pk │ p1_a_fkey │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey2 │ p1
(8 rows)
create table p11 (like p1, foreign key (a) references pk);
-- again, p11_a_fkey, p1_a_fkey, and p_a_fkey are equivalent
select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
pkrel │ fkconname │ fkrel
───────┼────────────┼───────
pk │ p_a_fkey │ p
pk │ p_a_fkey │ p
pk │ p1_a_fkey │ p1
pk │ p1_a_fkey │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p11_a_fkey │ p11
pk │ p11_a_fkey │ p11
(10 rows)
alter table p1 attach partition p11 for values in (1);
-- p1_a_fkey and p11_a_fkey merged, so triggers for the latter dropped
select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
pkrel │ fkconname │ fkrel
───────┼────────────┼───────
pk │ p_a_fkey │ p
pk │ p_a_fkey │ p
pk │ p1_a_fkey │ p1
pk │ p1_a_fkey │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey2 │ p1
(8 rows)
-- p_a_fkey and p1_a_fkey merged, so triggers for the latter dropped
alter table p attach partition p1 for values in (1);
select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
pkrel │ fkconname │ fkrel
───────┼────────────┼───────
pk │ p_a_fkey │ p
pk │ p_a_fkey │ p
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey2 │ p1
(6 rows)
alter table p detach partition p1;
-- p1_a_fkey needs its own triggers again
select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
pkrel │ fkconname │ fkrel
───────┼────────────┼───────
pk │ p_a_fkey │ p
pk │ p_a_fkey │ p
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey │ p1
pk │ p1_a_fkey │ p1
(8 rows)
alter table p1 detach partition p11;
-- p11_a_fkey needs its own triggers again
select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
pkrel │ fkconname │ fkrel
───────┼────────────┼───────
pk │ p_a_fkey │ p
pk │ p_a_fkey │ p
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey │ p1
pk │ p1_a_fkey │ p1
pk │ p11_a_fkey │ p11
pk │ p11_a_fkey │ p11
pk │ p1_a_fkey1 │ p11
pk │ p1_a_fkey1 │ p11
pk │ p1_a_fkey2 │ p11
pk │ p1_a_fkey2 │ p11
(14 rows)
-- try again
alter table p1 attach partition p11 for values in (1);
select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
pkrel │ fkconname │ fkrel
───────┼────────────┼───────
pk │ p_a_fkey │ p
pk │ p_a_fkey │ p
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey │ p1
pk │ p1_a_fkey │ p1
(8 rows)
alter table p attach partition p1 for values in (1);
select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
pkrel │ fkconname │ fkrel
───────┼────────────┼───────
pk │ p_a_fkey │ p
pk │ p_a_fkey │ p
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey1 │ p1
pk │ p1_a_fkey2 │ p1
pk │ p1_a_fkey2 │ p1
(6 rows)
By the way, I also noticed that there's duplicated code in
clone_fk_constraints() which 0001 gets rid of:
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_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));
-
I know you're working on a bug fix in the thread on pgsql-bugs which is
related to the patch 0002 here, but attaching it here anyway, because it
proposes to get rid of the needless dependencies which I didn't see
mentioned on the other thread. Also, updated 0001 needed it to be rebased.
Like the last time, I've also attached the patches that can be applied
PG11 branch.
Thanks,
Amit
Attachment
On 2019-Jan-18, Amit Langote wrote: > OK, I agree. I have updated the patch to make things work that way. Thanks, this is better. There were a few other things I didn't like, so I updated it. Mostly, two things: 1. I didn't like a seqscan on pg_trigger, so I turned that into an indexed scan on the constraint OID, and then the other two conditions are checked in the returned tuples. Also, what's the point on duplicating code and checking how many you deleted? Just delete them all. 2. I didn't like the ABI break, and it wasn't necessary: you can just call createForeignKeyActionTriggers directly. That's much simpler. I also added tests. While running them, I noticed that my previous commit was broken in terms of relcache invalidation. I don't really know if this is a new problem with that commit, or an existing one. The fix is 0001. Haven't got around to your 0002 yet. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Sat, Jan 19, 2019 at 7:16 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Thanks, this is better. There were a few other things I didn't like, so > I updated it. Mostly, two things: > > 1. I didn't like a seqscan on pg_trigger, so I turned that into an > indexed scan on the constraint OID, and then the other two conditions > are checked in the returned tuples. Also, what's the point on > duplicating code and checking how many you deleted? Just delete them > all. Yeah, I didn't quite like what that code looked like, but it didn't occur to me that there's an index on tgconstraint. It looks much better now. > 2. I didn't like the ABI break, and it wasn't necessary: you can just > call createForeignKeyActionTriggers directly. That's much simpler. OK. > I also added tests. While running them, I noticed that my previous > commit was broken in terms of relcache invalidation. I don't really > know if this is a new problem with that commit, or an existing one. The > fix is 0001. Looks good. Thanks, Amit
Pushed now, thanks. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Amit, Will you please rebase 0002? Please add your proposed tests cases to it, too. Thanks, -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019/01/22 8:30, Alvaro Herrera wrote: > Hi Amit, > > Will you please rebase 0002? Please add your proposed tests cases to > it, too. Done. See the attached patches for HEAD and PG 11. Thanks, Amit
Attachment
On 2019-Jan-22, Amit Langote wrote: > On 2019/01/22 8:30, Alvaro Herrera wrote: > > Hi Amit, > > > > Will you please rebase 0002? Please add your proposed tests cases to > > it, too. > > Done. See the attached patches for HEAD and PG 11. I'm not quite sure I understand why the one in DefineIndex needs the deps but nothing else does. I fear that you added that one just to appease the existing test that breaks otherwise, and I worry that with that addition we're papering over some other, more fundamental bug. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019/01/24 6:13, Alvaro Herrera wrote: > On 2019-Jan-22, Amit Langote wrote: >> Done. See the attached patches for HEAD and PG 11. > > I'm not quite sure I understand why the one in DefineIndex needs the > deps but nothing else does. I fear that you added that one just to > appease the existing test that breaks otherwise, and I worry that with > that addition we're papering over some other, more fundamental bug. Thinking more on this, my proposal to rip dependencies between parent and child constraints altogether to resolve the bug I initially reported is starting to sound a bit overambitious especially considering that we'd need to back-patch it (the patch didn't even consider index constraints properly, creating a divergence between the behaviors of inherited foreign key constraints and inherited index constraints). We can pursue it if only to avoid bloating the catalog for what can be achieved with little bit of additional code in tablecmds.c, but maybe we should refrain from doing it in reaction to this particular bug. I've updated the patch that implements a much simpler fix for this particular bug. Just to reiterate, the following illustrates the bug: create table pk (a int primary key); create table p (a int references pk) partition by list (a); create table p1 partition of p for values in (1) partition by list (a); create table p11 partition of p1 for values in (1); alter table p detach partition p1; alter table p1 drop constraint p_a_fkey; ERROR: constraint "p_a_fkey" of relation "p11" does not exist The error occurs because ATExecDropConstraint when recursively called on p11 cannot find the constraint as the dependency mechanism already dropped it. The new fix is to return from ATExecDropConstraint without recursing if the constraint being dropped is index or foreign constraint. A few hunks of the originally proposed patch are attached here as 0001, especially the part which fixes ATAddForeignKeyConstraint to pass the correct value of connoninherit to CreateConstraintEntry (which should be false for partitioned tables). With that change, many tests start failing because of the above bug. That patch also adds a test case like the one above, but it fails along with others due to the bug. Patch 0002 is the aforementioned simpler fix to make the errors (existing and the newly added) go away. These patches apply unchanged to the PG 11 branch. Thanks, Amit
Attachment
Hello On 2019-Jan-24, Amit Langote wrote: > Thinking more on this, my proposal to rip dependencies between parent and > child constraints altogether to resolve the bug I initially reported is > starting to sound a bit overambitious especially considering that we'd > need to back-patch it (the patch didn't even consider index constraints > properly, creating a divergence between the behaviors of inherited foreign > key constraints and inherited index constraints). We can pursue it if > only to avoid bloating the catalog for what can be achieved with little > bit of additional code in tablecmds.c, but maybe we should refrain from > doing it in reaction to this particular bug. While studying your fix it occurred to me that perhaps we could change things so that we first collect a list of objects to drop, and only when we're done recursing perform the deletion, as per the attached patch. However, this fails for the test case in your 0001 patch (but not the one you show in your email body), because you added a stealthy extra ingredient to it: the constraint in the grandchild has a different name, so when ATExecDropConstraint() tries to search for it by name, it's just not there, not because it was dropped but because it has never existed in the first place. Unless I misunderstand, this means that your plan to remove those catalog tuples won't work at all, because there is no way to find those constraints other than via pg_depend if they have different names. I'm leaning towards the idea that your patch is the definitive fix and not just a backpatchable band-aid. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Fri, Jan 25, 2019 at 12:08 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> On 2019-Jan-24, Amit Langote wrote:
>
> > Thinking more on this, my proposal to rip dependencies between parent and
> > child constraints altogether to resolve the bug I initially reported is
> > starting to sound a bit overambitious especially considering that we'd
> > need to back-patch it (the patch didn't even consider index constraints
> > properly, creating a divergence between the behaviors of inherited foreign
> > key constraints and inherited index constraints). We can pursue it if
> > only to avoid bloating the catalog for what can be achieved with little
> > bit of additional code in tablecmds.c, but maybe we should refrain from
> > doing it in reaction to this particular bug.
>
> While studying your fix it occurred to me that perhaps we could change
> things so that we first collect a list of objects to drop, and only when
> we're done recursing perform the deletion, as per the attached patch.
> However, this fails for the test case in your 0001 patch (but not the
> one you show in your email body), because you added a stealthy extra
> ingredient to it: the constraint in the grandchild has a different name,
> so when ATExecDropConstraint() tries to search for it by name, it's just
> not there, not because it was dropped but because it has never existed
> in the first place.
Doesn't the following performDeletion() at the start of
ATExecDropConstraint(), through findDependentObject()'s own recursion,
take care of deleting *all* constraints, including those of?
/*
* Perform the actual constraint deletion
*/
conobj.classId = ConstraintRelationId;
conobj.objectId = con->oid;
conobj.objectSubId = 0;
performDeletion(&conobj, behavior, 0);
> Unless I misunderstand, this means that your plan to remove those
> catalog tuples won't work at all, because there is no way to find those
> constraints other than via pg_depend if they have different names.
Yeah, that's right. Actually, I gave up on developing the patch
further based on that approach (no-dependencies approach) when I
edited the test to give the grandchild constraint its own name.
> I'm leaning towards the idea that your patch is the definitive fix and
> not just a backpatchable band-aid.
Yeah, I think so too.
Thanks,
Amit
On Fri, Jan 25, 2019 at 12:30 AM Amit Langote <amitlangote09@gmail.com> wrote: > > On Fri, Jan 25, 2019 at 12:08 AM Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > On 2019-Jan-24, Amit Langote wrote: > > > > > Thinking more on this, my proposal to rip dependencies between parent and > > > child constraints altogether to resolve the bug I initially reported is > > > starting to sound a bit overambitious especially considering that we'd > > > need to back-patch it (the patch didn't even consider index constraints > > > properly, creating a divergence between the behaviors of inherited foreign > > > key constraints and inherited index constraints). We can pursue it if > > > only to avoid bloating the catalog for what can be achieved with little > > > bit of additional code in tablecmds.c, but maybe we should refrain from > > > doing it in reaction to this particular bug. > > > > While studying your fix it occurred to me that perhaps we could change > > things so that we first collect a list of objects to drop, and only when > > we're done recursing perform the deletion, as per the attached patch. > > However, this fails for the test case in your 0001 patch (but not the > > one you show in your email body), because you added a stealthy extra > > ingredient to it: the constraint in the grandchild has a different name, > > so when ATExecDropConstraint() tries to search for it by name, it's just > > not there, not because it was dropped but because it has never existed > > in the first place. > > Doesn't the following performDeletion() at the start of > ATExecDropConstraint(), through findDependentObject()'s own recursion, > take care of deleting *all* constraints, including those of? Meant to say: "...including those of the grandchildren?" > /* > * Perform the actual constraint deletion > */ > conobj.classId = ConstraintRelationId; > conobj.objectId = con->oid; > conobj.objectSubId = 0; > > performDeletion(&conobj, behavior, 0); Thanks, Amit
On 2019-Jan-25, Amit Langote wrote: > On Fri, Jan 25, 2019 at 12:30 AM Amit Langote <amitlangote09@gmail.com> wrote: > > Doesn't the following performDeletion() at the start of > > ATExecDropConstraint(), through findDependentObject()'s own recursion, > > take care of deleting *all* constraints, including those of? > > Meant to say: "...including those of the grandchildren?" > > > /* > > * Perform the actual constraint deletion > > */ > > conobj.classId = ConstraintRelationId; > > conobj.objectId = con->oid; > > conobj.objectSubId = 0; > > > > performDeletion(&conobj, behavior, 0); Of course it does when the dependencies are set up -- but in the approach we just gave up on, those dependencies would not exist. Anyway, my motivation was that performMultipleDeletions has the advantage that it collects all objects to be dropped before deleting anyway, and so the error that a constraint was dropped in a previous recursion step would not occur. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Jan-24, Amit Langote wrote: > A few hunks of the originally proposed patch are attached here as 0001, > especially the part which fixes ATAddForeignKeyConstraint to pass the > correct value of connoninherit to CreateConstraintEntry (which should be > false for partitioned tables). With that change, many tests start failing > because of the above bug. That patch also adds a test case like the one > above, but it fails along with others due to the bug. Patch 0002 is the > aforementioned simpler fix to make the errors (existing and the newly > added) go away. Cool, thanks. I made a bunch of fixes -- I added an elog(ERROR) check to ensure a constraint whose parent is being set does not already have a parent; that seemed in line with the new asserts that check the coninhcount. I also moved those asserts, changing the spirit of what they checked. Also: I wasn't sure about stopping recursion for legacy inheritance in ATExecDropConstraint() for non-check constraints, so I changed that to occur in partitioned only. Also, stylistic fixes. I was mildly surprised to realize that the my_fkey constraint on fk_part_1_1 is gone after dropping fkey on its parent, since it was declared locally when that table was created. However, it makes perfect sense in retrospect, since we made it dependent on its parent. I'm not terribly happy about this, but I don't quite see a way to make it better that doesn't require much more code than is warranted. > These patches apply unchanged to the PG 11 branch. Yeah, only if you tried to compile, it would have complained about table_close() ;-) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019/01/25 2:18, Alvaro Herrera wrote: > On 2019-Jan-24, Amit Langote wrote: > >> A few hunks of the originally proposed patch are attached here as 0001, >> especially the part which fixes ATAddForeignKeyConstraint to pass the >> correct value of connoninherit to CreateConstraintEntry (which should be >> false for partitioned tables). With that change, many tests start failing >> because of the above bug. That patch also adds a test case like the one >> above, but it fails along with others due to the bug. Patch 0002 is the >> aforementioned simpler fix to make the errors (existing and the newly >> added) go away. > > Cool, thanks. I made a bunch of fixes -- I added an elog(ERROR) check > to ensure a constraint whose parent is being set does not already have a > parent; that seemed in line with the new asserts that check the > coninhcount. I also moved those asserts, changing the spirit of what > they checked. Also: I wasn't sure about stopping recursion for legacy > inheritance in ATExecDropConstraint() for non-check constraints, so I > changed that to occur in partitioned only. Also, stylistic fixes. Thanks for the fixes and committing. > I was mildly surprised to realize that the my_fkey constraint on > fk_part_1_1 is gone after dropping fkey on its parent, since it was > declared locally when that table was created. However, it makes perfect > sense in retrospect, since we made it dependent on its parent. I'm not > terribly happy about this, but I don't quite see a way to make it better > that doesn't require much more code than is warranted. Fwiw, CHECK constraints behave that way too. OTOH, detaching a partition preserves all the constraints, even the ones that were never locally defined on the partition. >> These patches apply unchanged to the PG 11 branch. > > Yeah, only if you tried to compile, it would have complained about > table_close() ;-) Oops, sorry. I was really in a hurry that day as the dinnertime had passed. Thanks, Amit