dropping partitioned tables without CASCADE

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.

On Mon, Feb 20, 2017 at 7:46 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Re-posting the patch I posted in a nearby thread [0].

On 2017/02/16 2:08, Robert Haas wrote:

On Wed, Feb 15, 2017 at 11:34 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

I think new-style partitioning is supposed to consider each partition as
an implementation detail of the table; the fact that you can manipulate
partitions separately does not really mean that they are their own
independent object. You don't stop to think "do I really want to drop
the TOAST table attached to this main table?" and attach a CASCADE
clause if so. You just drop the main table, and the toast one is
dropped automatically. I think new-style partitions should behave
equivalently.

That's a reasonable point of view. I'd like to get some more opinions
on this topic. I'm happy to have us do whatever most people want, but
I'm worried that having table inheritance and table partitioning work
differently will be create confusion. I'm also suspicious that there
may be some implementation difficulties. On the hand, it does seem a
little silly to say that DROP TABLE partitioned_table should always
fail except in the degenerate case where there are no partitions, so
maybe changing it is for the best.

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);

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.

--- 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.

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.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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]https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items 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.

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.

--- 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;

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,
Amit

[0]: https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items

On 2017/02/21 15:35, Amit Langote wrote:

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;

Oops, failed to address this in the last email. Updated patches attached.

Thanks,
Amit

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/02/21 20:17, Ashutosh Bapat wrote:

On Tue, Feb 21, 2017 at 12:05 PM, Amit Langote wrote:

On 2017/02/20 21:49, Ashutosh Bapat wrote:

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?

Oops, really fixed this time.

--- 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.

Sure.

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.

Posted in a new thread titled "foreign partition DDL regression tests".

Thanks,
Amit

On 2017/02/22 10:49, Amit Langote wrote:

On 2017/02/21 20:17, Ashutosh Bapat wrote:

Are you sure you have attached the right patch?

Oops, really fixed this time.

Sorry again, 3rd time's a charm. I copy-paste the hunk below from the
patch file before I attach it to make sure:

-	recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
+	if (child_is_partition)
+		recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
+	else
+		recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);

Thanks,
Amit

Looks good to me. In the attached patch I have added a comment
explaining the reason to make partition tables "Auto" dependent upon
the corresponding partitioned tables.

In the tests we are firing commands to drop partitioned table, but are
not checking whether those tables or the partitions are getting
dropped or not. Except for drop_if_exists.sql, I did not find that we
really check this. Should we try a query on pg_class to ensure that
the tables get really dropped?

On Wed, Feb 22, 2017 at 7:26 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/02/22 10:49, Amit Langote wrote:

On 2017/02/21 20:17, Ashutosh Bapat wrote:

Are you sure you have attached the right patch?

Oops, really fixed this time.

Sorry again, 3rd time's a charm. I copy-paste the hunk below from the
patch file before I attach it to make sure:

-       recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
+       if (child_is_partition)
+               recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
+       else
+               recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);

Thanks,
Amit

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

On 2017/02/22 13:46, Ashutosh Bapat wrote:

Looks good to me. In the attached patch I have added a comment
explaining the reason to make partition tables "Auto" dependent upon
the corresponding partitioned tables.

Good call.

+	/*
+	 * Unlike inheritance children, partition tables are expected to be dropped
+	 * when the parent partitioned table gets dropped.
+	 */

Hmm. Partitions *are* inheritance children, so we perhaps don't need the
part before the comma. Also, adding "automatically" somewhere in there
would be nice.

Or, one could just write: /* add an auto dependency for partitions */

In the tests we are firing commands to drop partitioned table, but are
not checking whether those tables or the partitions are getting
dropped or not. Except for drop_if_exists.sql, I did not find that we
really check this. Should we try a query on pg_class to ensure that
the tables get really dropped?

I don't see why this patch should do it, if dependency.sql itself does
not? I mean dropping AUTO dependent objects is one of the contracts of
dependency.c, so perhaps it would make sense to query pg_class in
dependency.sql to check if AUTO dependencies work correctly.

Thanks,
Amit

On Wed, Feb 22, 2017 at 11:11 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/02/22 13:46, Ashutosh Bapat wrote:

Looks good to me. In the attached patch I have added a comment
explaining the reason to make partition tables "Auto" dependent upon
the corresponding partitioned tables.

Good call.

+       /*
+        * Unlike inheritance children, partition tables are expected to be dropped
+        * when the parent partitioned table gets dropped.
+        */

Hmm. Partitions *are* inheritance children, so we perhaps don't need the
part before the comma. Also, adding "automatically" somewhere in there
would be nice.

Or, one could just write: /* add an auto dependency for partitions */

I changed it in the attached patch to
+    /*
+     * Partition tables are expected to be dropped when the parent partitioned
+     * table gets dropped.
+     */

In the tests we are firing commands to drop partitioned table, but are
not checking whether those tables or the partitions are getting
dropped or not. Except for drop_if_exists.sql, I did not find that we
really check this. Should we try a query on pg_class to ensure that
the tables get really dropped?

I don't see why this patch should do it, if dependency.sql itself does
not? I mean dropping AUTO dependent objects is one of the contracts of
dependency.c, so perhaps it would make sense to query pg_class in
dependency.sql to check if AUTO dependencies work correctly.

Hmm, I agree.

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

On 2017/02/22 21:24, Ashutosh Bapat wrote:

On Wed, Feb 22, 2017 at 11:11 AM, Amit Langote wrote:

+       /*
+        * Unlike inheritance children, partition tables are expected to be dropped
+        * when the parent partitioned table gets dropped.
+        */

Hmm. Partitions *are* inheritance children, so we perhaps don't need the
part before the comma. Also, adding "automatically" somewhere in there
would be nice.

Or, one could just write: /* add an auto dependency for partitions */

I changed it in the attached patch to
+    /*
+     * Partition tables are expected to be dropped when the parent partitioned
+     * table gets dropped.
+     */

OK, thanks.

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

I think this is ready for committer.

On Thu, Feb 23, 2017 at 12:02 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/02/22 21:24, Ashutosh Bapat wrote:

On Wed, Feb 22, 2017 at 11:11 AM, Amit Langote wrote:

+       /*
+        * Unlike inheritance children, partition tables are expected to be dropped
+        * when the parent partitioned table gets dropped.
+        */

Hmm. Partitions *are* inheritance children, so we perhaps don't need the
part before the comma. Also, adding "automatically" somewhere in there
would be nice.

Or, one could just write: /* add an auto dependency for partitions */

I changed it in the attached patch to
+    /*
+     * Partition tables are expected to be dropped when the parent partitioned
+     * table gets dropped.
+     */

OK, thanks.

Thanks,
Amit

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 23 February 2017 at 06:40, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

I think this is ready for committer.

Thanks for writing and reviewing this. I'll be happy to review and
commit. Please add to CF.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/02/24 1:33, Simon Riggs wrote:

On 23 February 2017 at 06:40, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

I think this is ready for committer.

Thanks for writing and reviewing this. I'll be happy to review and
commit. Please add to CF.

Thanks. I've added it to CF: https://commitfest.postgresql.org/13/1030/

Regards,
Amit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 23 February 2017 at 16:33, Simon Riggs <simon@2ndquadrant.com> wrote:

I'll be happy to review

Patch looks OK so far, but fails on a partition that has partitions,
probably because of the way we test relkind in the call to
StoreCatalogInheritance1().

Please add a test for that so we can check automatically.

Thanks very much.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/02/26 5:30, Simon Riggs wrote:

On 23 February 2017 at 16:33, Simon Riggs <simon@2ndquadrant.com> wrote:

I'll be happy to review

Patch looks OK so far, but fails on a partition that has partitions,
probably because of the way we test relkind in the call to
StoreCatalogInheritance1().

Thanks for the review.

I could not reproduce the failure you are seeing; could you perhaps share
the failing test case? Here's mine that seems to work as expected:

create table p (a int, b char) partition by list (a);
create table p1 (a int, b char) partition by list (b);
alter table p attach partition p1 for values in (1);

-- add a partition to p1
create table p1a (like p1);
alter table p1 attach partition p1a for values in ('a');

create table p2 partition of p for values in (1)

\d+ p
<snip>
Partition key: LIST (a)
Partitions: p1 FOR VALUES IN (1),
p2 FOR VALUES IN (2)

-- this works (remember that p1 is a partitioned table)
drop table p1;
DROP TABLE

\d+ p
<snip>
Partition key: LIST (a)
Partitions: p2 FOR VALUES IN (2)

Please add a test for that so we can check automatically.

OK, done.

Thanks,
Amit

On Mon, Feb 27, 2017 at 8:08 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/02/26 5:30, Simon Riggs wrote:

On 23 February 2017 at 16:33, Simon Riggs <simon@2ndquadrant.com> wrote:

I'll be happy to review

Patch looks OK so far, but fails on a partition that has partitions,
probably because of the way we test relkind in the call to
StoreCatalogInheritance1().

Thanks for the review.

I could not reproduce the failure you are seeing; could you perhaps share
the failing test case? Here's mine that seems to work as expected:

create table p (a int, b char) partition by list (a);
create table p1 (a int, b char) partition by list (b);
alter table p attach partition p1 for values in (1);

-- add a partition to p1
create table p1a (like p1);
alter table p1 attach partition p1a for values in ('a');

create table p2 partition of p for values in (1)

\d+ p
<snip>
Partition key: LIST (a)
Partitions: p1 FOR VALUES IN (1),
p2 FOR VALUES IN (2)

-- this works (remember that p1 is a partitioned table)
drop table p1;
DROP TABLE

\d+ p
<snip>
Partition key: LIST (a)
Partitions: p2 FOR VALUES IN (2)

Please add a test for that so we can check automatically.

OK, done.

Isn't list_range_parted multilevel partitioned table. It gets dropped
in the testcases. So, I guess, we already have a testcase there.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/02/27 13:35, Ashutosh Bapat wrote:

On Mon, Feb 27, 2017 at 8:08 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/02/26 5:30, Simon Riggs wrote:

On 23 February 2017 at 16:33, Simon Riggs <simon@2ndquadrant.com> wrote:

I'll be happy to review

Patch looks OK so far, but fails on a partition that has partitions,
probably because of the way we test relkind in the call to
StoreCatalogInheritance1().

Thanks for the review.

I could not reproduce the failure you are seeing; could you perhaps share
the failing test case? Here's mine that seems to work as expected:

create table p (a int, b char) partition by list (a);
create table p1 (a int, b char) partition by list (b);
alter table p attach partition p1 for values in (1);

-- add a partition to p1
create table p1a (like p1);
alter table p1 attach partition p1a for values in ('a');

create table p2 partition of p for values in (1)

\d+ p
<snip>
Partition key: LIST (a)
Partitions: p1 FOR VALUES IN (1),
p2 FOR VALUES IN (2)

-- this works (remember that p1 is a partitioned table)
drop table p1;
DROP TABLE

\d+ p
<snip>
Partition key: LIST (a)
Partitions: p2 FOR VALUES IN (2)

Please add a test for that so we can check automatically.

OK, done.

Isn't list_range_parted multilevel partitioned table. It gets dropped
in the testcases. So, I guess, we already have a testcase there.

I thought Simon meant the test case where a partition that is itself
partitioned is dropped. At least that's what I took from "... fails *on*
partition that has partitions". So in the example I posted, drop table p1.

Anyway, there might be the confusion that *only* the root level
partitioned table is of RELKIND_PARTITIONED_TABLE. That's not true - any
partitioned table (even one that's a partition) is of that relkind. So
the condition in the call to StoreCatalogInheritance1() is correct. The
following hunk:

@@ -10744,7 +10756,9 @@ CreateInheritance(Relation child_rel, Relation
parent_rel)
     StoreCatalogInheritance1(RelationGetRelid(child_rel),
                              RelationGetRelid(parent_rel),
                              inhseqno + 1,
-                             catalogRelation);
+                             catalogRelation,
+                             parent_rel->rd_rel->relkind ==
+                                            RELKIND_PARTITIONED_TABLE);

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

Isn't list_range_parted multilevel partitioned table. It gets dropped
in the testcases. So, I guess, we already have a testcase there.

I thought Simon meant the test case where a partition that is itself
partitioned is dropped. At least that's what I took from "... fails *on*
partition that has partitions". So in the example I posted, drop table p1.

Ok. Thanks for the explanation.

Anyway, there might be the confusion that *only* the root level
partitioned table is of RELKIND_PARTITIONED_TABLE. That's not true - any
partitioned table (even one that's a partition) is of that relkind. So
the condition in the call to StoreCatalogInheritance1() is correct. The
following hunk:

@@ -10744,7 +10756,9 @@ CreateInheritance(Relation child_rel, Relation
parent_rel)
StoreCatalogInheritance1(RelationGetRelid(child_rel),
RelationGetRelid(parent_rel),
inhseqno + 1,
-                             catalogRelation);
+                             catalogRelation,
+                             parent_rel->rd_rel->relkind ==
+                                            RELKIND_PARTITIONED_TABLE);

Thanks,
Amit

I agree.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 27 February 2017 at 02:38, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/02/26 5:30, Simon Riggs wrote:

On 23 February 2017 at 16:33, Simon Riggs <simon@2ndquadrant.com> wrote:

I'll be happy to review

Patch looks OK so far, but fails on a partition that has partitions,
probably because of the way we test relkind in the call to
StoreCatalogInheritance1().

Thanks for the review.

I could not reproduce the failure you are seeing; could you perhaps share
the failing test case?

I used a slight modification of the case mentioned on the docs. I
confirm this fails repeatably for me on current HEAD.

CREATE TABLE cities (
city_id bigserial not null,
name text not null,
population bigint
) PARTITION BY LIST (left(lower(name), 1));

CREATE TABLE cities_ab
PARTITION OF cities (
CONSTRAINT city_id_nonzero CHECK (city_id != 0)
) FOR VALUES IN ('a', 'b')
PARTITION BY RANGE (population);

drop table cities;
ERROR: cannot drop table cities because other objects depend on it
DETAIL: table cities_ab depends on table cities
HINT: Use DROP ... CASCADE to drop the dependent objects too.

I notice also that
\d+ <tablename>
does not show which partitions have subpartitions.

I'm worried that these things illustrate something about the catalog
representation that we may need to improve, but I don't have anything
concrete to say on that at present.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers