Horton

Thread

Commits

  1. Fix failure to remove dependencies when a partition is detached.

  1. Dropping partitioned table drops a previously detached partition

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-12T11:56:21Z

    Hi,
If we detach a partition and drop the corresponding partitioned table,
it drops the once-partition now-standalone table as well.

create table prt1 (a int, b int) partition by range(a);
create table prt1_p1 partition of prt1 for values from (0) to (100);
select oid, relname, relpartbound, relispartition, relkind from
pg_class where relname like 'prt%';
  oid  | relname
-------+---------
 16453 | prt1
 16456 | prt1_p1
(2 rows)

select * from pg_depend where refobjid = 'prt1'::regclass and objid =
'prt1_p1'::regclass;
 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
---------+-------+----------+------------+----------+-------------+---------
    1259 | 16456 |        0 |       1259 |    16453 |           0 | a
(1 row)

alter table prt1 detach partition prt1_p1;

-- so the dependency exists even after detach
select * from pg_depend where refobjid = 'prt1'::regclass and objid =
'prt1_p1'::regclass;
 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
---------+-------+----------+------------+----------+-------------+---------
    1259 | 16456 |        0 |       1259 |    16453 |           0 | a
(1 row)

drop table prt1;

-- Oops, we deleted the once-partition now-standalone table as well
select oid, relname, relpartbound, relispartition, relkind from
pg_class where relname like 'prt%';
 oid | relname | relpartbound | relispartition | relkind
-----+---------+--------------+----------------+---------
(0 rows)

The reason for this is as follows
    An AUTO dependency is recorded between a partitioned table and partition. In
    case of inheritance we record a NORMAL dependency between parent
and child. While
    detaching a partition, we call RemoveInheritance(), which should have taken
    care of removing this dependency. But it removes the dependencies which are
    marked as NORMAL. When we changed the dependency for partitioned case from
    NORMAL to AUTO by updating StoreCatalogInheritance1(), this function was not
    updated. This caused the dependency to linger behind even after
detaching the
    partition, thus causing the now standalone table (which was once a
partition)
    to be dropped when the parent partitioned table is dropped. This patch fixes
    RemoveInheritance() to choose appropriate dependency.

Attached patch 0001 to fix this.

I see a similar issue-in-baking in case we change the type of
dependency recorded between a table and the composite type associated
with using CREATE TABLE ... OF command. 0002 patch addresses the
potential issue by using a macro while creating and dropping the
dependency in corresponding functions. There might be more such
places, but I haven't searched for those.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  2. Re: Dropping partitioned table drops a previously detached partition

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-06-13T04:55:20Z

    Hi Ashutosh,

On 2017/06/12 20:56, Ashutosh Bapat wrote:
> Hi,
> If we detach a partition and drop the corresponding partitioned table,
> it drops the once-partition now-standalone table as well.

Thanks for the report.  Looks like 8b4d582d279d78 missed the bit about
drop_parent_dependency() that you describe in your analysis below.

> The reason for this is as follows
>     An AUTO dependency is recorded between a partitioned table and partition. In
>     case of inheritance we record a NORMAL dependency between parent
> and child. While
>     detaching a partition, we call RemoveInheritance(), which should have taken
>     care of removing this dependency. But it removes the dependencies which are
>     marked as NORMAL. When we changed the dependency for partitioned case from
>     NORMAL to AUTO by updating StoreCatalogInheritance1(), this function was not
>     updated. This caused the dependency to linger behind even after
> detaching the
>     partition, thus causing the now standalone table (which was once a
> partition)
>     to be dropped when the parent partitioned table is dropped. This patch fixes
>     RemoveInheritance() to choose appropriate dependency.
> 
> Attached patch 0001 to fix this.

Looks good to me.  Perhaps, the example in your email could be added as a
test case.

> I see a similar issue-in-baking in case we change the type of
> dependency recorded between a table and the composite type associated
> with using CREATE TABLE ... OF command. 0002 patch addresses the
> potential issue by using a macro while creating and dropping the
> dependency in corresponding functions. There might be more such
> places, but I haven't searched for those.

Might be a good idea too.

Adding this to the open items list.

Thanks,
Amit

  3. Re: Dropping partitioned table drops a previously detached partition

    Rahila Syed <rahilasyed90@gmail.com> — 2017-06-13T13:44:12Z

    I reviewed and tested
0001-Dependency-between-partitioned-table-and-partition_v1.patch
It applies cleanly on master and make check passes.

Following are few comments:

>/*
> * Drop the dependency created by StoreCatalogInheritance1 (CREATE TABLE
> * INHERITS/ALTER TABLE INHERIT -- refclassid will be RelationRelationId)
or
> * heap_create_with_catalog (CREATE TABLE OF/ALTER TABLE OF -- refclassid
will
> * be TypeRelationId).  There's no convenient way to do this, so go
trawling
> * through pg_depend.
> */
>static void
>drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
                       DependencyType deptype)

This is not directly related to this patch but still would like to mention.
drop_parent_dependency() is being used to drop the dependency created
by CREATE TABLE PARTITION OF command also, so probably the comment
needs to be modified to reflect that.

>+/*
>+ * Partition tables are expected to be dropped when the parent partitioned
>+ * table gets dropped. Hence for partitioning we use AUTO dependency.
>+ * Otherwise, for regular inheritance use NORMAL dependency.
A minor cosmetic correction:
+ Hence for *declarative* partitioning we use AUTO dependency
+ * Otherwise, for regular inheritance *we* use NORMAL dependency.

I have added tests to the
0001-Dependency-between-partitioned-table-and-partition_v1.patch. Please
find attached the v2 patch.



On Tue, Jun 13, 2017 at 10:25 AM, Amit Langote <
Langote_Amit_f8@lab.ntt.co.jp> wrote:

> Hi Ashutosh,
>
> On 2017/06/12 20:56, Ashutosh Bapat wrote:
> > Hi,
> > If we detach a partition and drop the corresponding partitioned table,
> > it drops the once-partition now-standalone table as well.
>
> Thanks for the report.  Looks like 8b4d582d279d78 missed the bit about
> drop_parent_dependency() that you describe in your analysis below.
>
> > The reason for this is as follows
> >     An AUTO dependency is recorded between a partitioned table and
> partition. In
> >     case of inheritance we record a NORMAL dependency between parent
> > and child. While
> >     detaching a partition, we call RemoveInheritance(), which should
> have taken
> >     care of removing this dependency. But it removes the dependencies
> which are
> >     marked as NORMAL. When we changed the dependency for partitioned
> case from
> >     NORMAL to AUTO by updating StoreCatalogInheritance1(), this function
> was not
> >     updated. This caused the dependency to linger behind even after
> > detaching the
> >     partition, thus causing the now standalone table (which was once a
> > partition)
> >     to be dropped when the parent partitioned table is dropped. This
> patch fixes
> >     RemoveInheritance() to choose appropriate dependency.
> >
> > Attached patch 0001 to fix this.
>
> Looks good to me.  Perhaps, the example in your email could be added as a
> test case.
>
> > I see a similar issue-in-baking in case we change the type of
> > dependency recorded between a table and the composite type associated
> > with using CREATE TABLE ... OF command. 0002 patch addresses the
> > potential issue by using a macro while creating and dropping the
> > dependency in corresponding functions. There might be more such
> > places, but I haven't searched for those.
>
> Might be a good idea too.
>
> Adding this to the open items list.
>
> Thanks,
> Amit
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

  4. Re: Dropping partitioned table drops a previously detached partition

    Robert Haas <robertmhaas@gmail.com> — 2017-06-13T15:53:55Z

    On Tue, Jun 13, 2017 at 9:44 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> I have added tests to the
> 0001-Dependency-between-partitioned-table-and-partition_v1.patch. Please
> find attached the v2 patch.

Thanks.  Committed.  I don't think the 0002 patch is an improvement -
sure, it keeps those things in sync, but it also makes the code harder
to read.  On balance I think it's a negative.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  5. Re: Dropping partitioned table drops a previously detached partition

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-14T04:51:54Z

    On Tue, Jun 13, 2017 at 7:14 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
> I reviewed and tested
> 0001-Dependency-between-partitioned-table-and-partition_v1.patch
> It applies cleanly on master and make check passes.
>
> Following are few comments:
>
>>/*
>> * Drop the dependency created by StoreCatalogInheritance1 (CREATE TABLE
>> * INHERITS/ALTER TABLE INHERIT -- refclassid will be RelationRelationId)
>> or
>> * heap_create_with_catalog (CREATE TABLE OF/ALTER TABLE OF -- refclassid
>> will
>> * be TypeRelationId).  There's no convenient way to do this, so go
>> trawling
>> * through pg_depend.
>> */
>>static void
>>drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
>                        DependencyType deptype)
>
> This is not directly related to this patch but still would like to mention.
> drop_parent_dependency() is being used to drop the dependency created
> by CREATE TABLE PARTITION OF command also, so probably the comment
> needs to be modified to reflect that.
>

The comment is fine for dependency created by CREATE TABLE PARTITION
OF since that too goes through StoreCatalogInheritance1(). But it's
not correct for CREATE TABLE ... OF <composite type> since that does
not go through StoreCatalogInheritance1().

>>+/*
>>+ * Partition tables are expected to be dropped when the parent partitioned
>>+ * table gets dropped. Hence for partitioning we use AUTO dependency.
>>+ * Otherwise, for regular inheritance use NORMAL dependency.
> A minor cosmetic correction:
> + Hence for declarative partitioning we use AUTO dependency
> + * Otherwise, for regular inheritance we use NORMAL dependency.
>
> I have added tests to the
> 0001-Dependency-between-partitioned-table-and-partition_v1.patch. Please
> find attached the v2 patch.
>
>
>
> On Tue, Jun 13, 2017 at 10:25 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>
>> Hi Ashutosh,
>>
>> On 2017/06/12 20:56, Ashutosh Bapat wrote:
>> > Hi,
>> > If we detach a partition and drop the corresponding partitioned table,
>> > it drops the once-partition now-standalone table as well.
>>
>> Thanks for the report.  Looks like 8b4d582d279d78 missed the bit about
>> drop_parent_dependency() that you describe in your analysis below.
>>
>> > The reason for this is as follows
>> >     An AUTO dependency is recorded between a partitioned table and
>> > partition. In
>> >     case of inheritance we record a NORMAL dependency between parent
>> > and child. While
>> >     detaching a partition, we call RemoveInheritance(), which should
>> > have taken
>> >     care of removing this dependency. But it removes the dependencies
>> > which are
>> >     marked as NORMAL. When we changed the dependency for partitioned
>> > case from
>> >     NORMAL to AUTO by updating StoreCatalogInheritance1(), this function
>> > was not
>> >     updated. This caused the dependency to linger behind even after
>> > detaching the
>> >     partition, thus causing the now standalone table (which was once a
>> > partition)
>> >     to be dropped when the parent partitioned table is dropped. This
>> > patch fixes
>> >     RemoveInheritance() to choose appropriate dependency.
>> >
>> > Attached patch 0001 to fix this.
>>
>> Looks good to me.  Perhaps, the example in your email could be added as a
>> test case.
>>
>> > I see a similar issue-in-baking in case we change the type of
>> > dependency recorded between a table and the composite type associated
>> > with using CREATE TABLE ... OF command. 0002 patch addresses the
>> > potential issue by using a macro while creating and dropping the
>> > dependency in corresponding functions. There might be more such
>> > places, but I haven't searched for those.
>>
>> Might be a good idea too.
>>
>> Adding this to the open items list.
>>
>> Thanks,
>> Amit
>>
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  6. Re: Dropping partitioned table drops a previously detached partition

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-14T05:01:41Z

    On Tue, Jun 13, 2017 at 9:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jun 13, 2017 at 9:44 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>> I have added tests to the
>> 0001-Dependency-between-partitioned-table-and-partition_v1.patch. Please
>> find attached the v2 patch.
>
> Thanks.  Committed.

Thanks.

> I don't think the 0002 patch is an improvement -
> sure, it keeps those things in sync, but it also makes the code harder
> to read.  On balance I think it's a negative.
>

I don't think the code is hard to read, but I agree that the macro
name TABLE_COMPOSITE_TYPE_DEPENDENCY isn't conveying the real sense.
But that's not a topic for this thread. I will start a separate a
thread.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  7. Re: Dropping partitioned table drops a previously detached partition

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-14T05:02:57Z

    On Wed, Jun 14, 2017 at 10:21 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Tue, Jun 13, 2017 at 7:14 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>> I reviewed and tested
>> 0001-Dependency-between-partitioned-table-and-partition_v1.patch
>> It applies cleanly on master and make check passes.
>>
>> Following are few comments:
>>
>>>/*
>>> * Drop the dependency created by StoreCatalogInheritance1 (CREATE TABLE
>>> * INHERITS/ALTER TABLE INHERIT -- refclassid will be RelationRelationId)
>>> or
>>> * heap_create_with_catalog (CREATE TABLE OF/ALTER TABLE OF -- refclassid
>>> will
>>> * be TypeRelationId).  There's no convenient way to do this, so go
>>> trawling
>>> * through pg_depend.
>>> */
>>>static void
>>>drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
>>                        DependencyType deptype)
>>
>> This is not directly related to this patch but still would like to mention.
>> drop_parent_dependency() is being used to drop the dependency created
>> by CREATE TABLE PARTITION OF command also, so probably the comment
>> needs to be modified to reflect that.
>>
>
> The comment is fine for dependency created by CREATE TABLE PARTITION
> OF since that too goes through StoreCatalogInheritance1(). But it's
> not correct for CREATE TABLE ... OF <composite type> since that does
> not go through StoreCatalogInheritance1().
>
I missed "or" part of the prologue while writing this. Now that I have
read it carefully, the prologue looks good to me.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company