Horton

Thread

Commits

  1. Fix droppability of constraints upon partition detach

  2. Create action triggers when partitions are detached

  1. problems with foreign keys on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-01-09T10:21:38Z

    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

  2. Re: problems with foreign keys on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-11T22:39:47Z

    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

  3. Re: problems with foreign keys on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-17T22:54:31Z

    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

  4. Re: problems with foreign keys on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-01-18T05:27:08Z

    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

  5. Re: problems with foreign keys on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-18T22:16:33Z

    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

  6. Re: problems with foreign keys on partitioned tables

    Amit Langote <amitlangote09@gmail.com> — 2019-01-19T12:07:42Z

    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

  7. Re: problems with foreign keys on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-21T23:12:30Z

    Pushed now, thanks.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: problems with foreign keys on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-21T23:30:20Z

    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

  9. Re: problems with foreign keys on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-01-22T04:29:43Z

    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

  10. Re: problems with foreign keys on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-23T21:13:14Z

    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

  11. Re: problems with foreign keys on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-01-24T12:43:20Z

    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

  12. Re: problems with foreign keys on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-24T15:08:43Z

    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

  13. Re: problems with foreign keys on partitioned tables

    Amit Langote <amitlangote09@gmail.com> — 2019-01-24T15:30:08Z

    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

  14. Re: problems with foreign keys on partitioned tables

    Amit Langote <amitlangote09@gmail.com> — 2019-01-24T15:37:40Z

    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

  15. Re: problems with foreign keys on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-24T16:37:41Z

    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

  16. Re: problems with foreign keys on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-24T17:18:11Z

    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

  17. Re: problems with foreign keys on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-01-28T03:02:13Z

    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