Re: [HACKERS] Dropping partitioned table drops a previously detached partition

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