Re: [HACKERS] dropping partitioned tables without CASCADE

On Tue, Feb 21, 2017 at 12:05 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Hi Ashutosh,
>
> Thanks for taking a look at the patch.
>
> On 2017/02/20 21:49, Ashutosh Bapat wrote:
>> Thanks for working on all the follow on work for partitioning feature.
>>
>> May be you should add all those patches in the next commitfest, so
>> that we don't forget those.
>
> I think adding these as one of the PostgreSQL 10 Open Items [0] might be
> better.  I've done that.
>
>> On Mon, Feb 20, 2017 at 7:46 AM, Amit Langote wrote:
>>> So I count more than a few votes saying that we should be able to DROP
>>> partitioned tables without specifying CASCADE.
>>>
>>> I tried to implement that using the attached patch by having
>>> StoreCatalogInheritance1() create DEPENDENCY_AUTO dependency between
>>> parent and child if the child is a partition, instead of DEPENDENCY_NORMAL
>>> that would otherwise be created.  Now it seems that that is one way of
>>> making sure that partitions are dropped when the root partitioned table is
>>> dropped, not sure if the best; why create the pg_depend entries at all one
>>> might ask.  I chose it for now because that's the one with fewest lines of
>>> change.  Adjusted regression tests as well, since we recently tweaked
>>> tests [1] to work around the irregularities of test output when using CASCADE.
>>
>> The patch applies cleanly and regression does not show any failures.
>>
>> Here are some comments
>>
>> For the sake of readability you may want reverse the if and else order.
>> -    recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
>> +    if (!child_is_partition)
>> +        recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
>> +    else
>> +        recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
>> like
>> +    if (child_is_partition)
>> +        recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
>> +    else
>> +        recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
>
> Sure, done.

I still see
-    recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
+    if (!child_is_partition)
+        recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
+    else
+        recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);

Are you sure you have attached the right patch?

To avoid duplication you could actually write
recordDependencyOn(&childobject, &parentobject,                                   child_is_partition ?
DEPENDENCY_AUTO : DEPENDENCY_NORMAL);

>
>> It's weird that somebody can perform DROP TABLE on the partition without
>> referring to its parent. That may be a useful feature as it allows one to
>> detach the partition as well as remove the table in one command. But it looks
>> wierd for someone familiar with partitioning features of other DBMSes. But then
>> our partition creation command is CREATE TABLE .... So may be this is expected
>> difference.
>
> There is a line on the CREATE TABLE page in the description of PARTITION
> OF clause:
>
> "Note that dropping a partition with DROP TABLE requires taking an ACCESS
> EXCLUSIVE lock on the parent table."
>
> In earlier proposals I had included the ALTER TABLE parent ADD/DROP
> PARTITION commands, but CRAETE TABLE PARTITION OF / DROP TABLE prevailed.

Ok.

>
>> --- cleanup: avoid using CASCADE
>> -DROP TABLE list_parted, part_1;
>> -DROP TABLE list_parted2, part_2, part_5, part_5_a;
>> -DROP TABLE range_parted, part1, part2;
>> +-- cleanup
>> +DROP TABLE list_parted, list_parted2, range_parted;
>> Testcases usually drop one table at a time, I guess, to reduce the differences
>> when we add or remove tables from testcases. All such blocks should probably
>> follow same policy.
>
> Hmm, I see this in src/test/regress/sql/inherit.sql:141
>
> DROP TABLE firstparent, secondparent, jointchild, thirdparent, otherchild;

Hmm, I can spot some more such usages. Let's keep this for the
committer to decide.

>
>>  drop table list_parted cascade;
>> -NOTICE:  drop cascades to 3 other objects
>> -DETAIL:  drop cascades to table part_ab_cd
>> probably we should remove cascade from there, unless you are testing CASCADE
>> functionality. Similarly for other blocks like
>>  drop table range_parted cascade;
>>
>> BTW, I noticed that although we are allowing foreign tables to be
>> partitions, there are no tests in foreign_data.sql for testing it. If
>> there would have been we would tests DROP TABLE on a partitioned table
>> with foreign partitions as well. That file has testcases for testing
>> foreign table inheritance, and should have tests for foreign table
>> partitions.
>
> That makes sense.  Patch 0002 is for that (I'm afraid this should be
> posted separately though).  I didn't add/repeat all the tests that were
> added by the foreign table inheritance patch again for foreign partitions
> (common inheritance rules apply to both cases), only added those for the
> new partitioning commands and certain new rules.

Thanks. Yes, a separate thread would do. I will review it there. May
be you want to add it to the commitfest too.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company