Thread: [HACKERS] dropping partitioned tables without CASCADE

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.

Thanks,
Amit

[0] https://postgr.es/m/ca132b99-0d18-439a-fe65-024085449259%40lab.ntt.co.jp
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c397814

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

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

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.

> 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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

>
> 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 think that's what this patch fixes. Do you see this behaviour after
applying the patch?

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

I will confirm this once I have access to the code.

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

AFAIK, catalogs have everything needed to fix this; it's just the
matter to using that information wherever it's needed.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 5 March 2017 at 07:59, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
>>
>> 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 think that's what this patch fixes. Do you see this behaviour after
> applying the patch?

It does seems as if I've made a mistake there. The patch passes.
Thanks for checking.

I will apply tomorrow if no further comments.

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

On 2017/03/05 16:20, Simon Riggs wrote:
> I notice also that
>   \d+ <tablename>
> does not show which partitions have subpartitions.

Do you mean showing just whether a partition is itself partitioned or
showing its partitions and so on (because those partitions may themselves
be partitioned)?  Maybe, we could do the former.

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

Perhaps.  As Ashutosh said though, it does not seem like a big problem in
this particular case.

Thanks,
Amit

On 6 March 2017 at 00:51, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/03/05 16:20, Simon Riggs wrote:
>> I notice also that
>>   \d+ <tablename>
>> does not show which partitions have subpartitions.
>
> Do you mean showing just whether a partition is itself partitioned or
> showing its partitions and so on (because those partitions may themselves
> be partitioned)?  Maybe, we could do the former.

I think \d+ should show the full information, in some form.

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

On Mon, Mar 6, 2017 at 8:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 6 March 2017 at 00:51, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/03/05 16:20, Simon Riggs wrote:
>>> I notice also that
>>>   \d+ <tablename>
>>> does not show which partitions have subpartitions.
>>
>> Do you mean showing just whether a partition is itself partitioned or
>> showing its partitions and so on (because those partitions may themselves
>> be partitioned)?  Maybe, we could do the former.
>
> I think \d+ should show the full information, in some form.

For a multi-level inheritance hierarchy, we don't show children which
are further inherited. Same behaviour has been carried over to
partitioning. I don't say that that's good or bad.

Given the recursive structure of partitioned tables, it looks readable
and manageable to print only the direct partitions in \d+. May be we
want to indicate the partitions that are further partitioned. If user
wants information about partitioned partitions, s/he can execute \d+
on the partition, repeating this process to any desired level. This
would work well in the interactive mode, keeping the output of \d+
manageable. Further someone writing a script to consume \d+ output of
a multi-level partitioned table, can code the above process in a
script.

Thinking about how to display partition which are further partitioned,
there are two options. Assume a partitioned table t1 with partitions
t1p1, which is further partitioned and t1p2. One could display \d+ t1
as

\d+ t1                                   Table "public.t1"Column |  Type   | Collation | Nullable | Default | Storage |
Stats
target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------a      | integer |
|not null |         | plain   |              |
 
Partition key: RANGE (a)
Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS           t1p2 FOR VALUES FROM (100) TO (200)

OR

\d+ t1                                   Table "public.t1"Column |  Type   | Collation | Nullable | Default | Storage |
Stats
target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------a      | integer |
|not null |         | plain   |              |
 
Partition key: RANGE (a)
Partitions: t1p1 FOR VALUES FROM (0) TO (100), PARTITION BY LIST(a)           t1p2 FOR VALUES FROM (100) TO (200)

To me the first option looks fine. If the user is interested in
looking at the subpartitioned in any case s/he will have to execute
\d+. So beyond indicating that a partition has subpartitions, what
other information to include is debatable, given that \d+ on that
partition is going to print it anyway.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 6 March 2017 at 04:00, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Mon, Mar 6, 2017 at 8:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 6 March 2017 at 00:51, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> On 2017/03/05 16:20, Simon Riggs wrote:
>>>> I notice also that
>>>>   \d+ <tablename>
>>>> does not show which partitions have subpartitions.
>>>
>>> Do you mean showing just whether a partition is itself partitioned or
>>> showing its partitions and so on (because those partitions may themselves
>>> be partitioned)?  Maybe, we could do the former.
>>
>> I think \d+ should show the full information, in some form.
>
> For a multi-level inheritance hierarchy, we don't show children which
> are further inherited. Same behaviour has been carried over to
> partitioning. I don't say that that's good or bad.
>
> Given the recursive structure of partitioned tables, it looks readable
> and manageable to print only the direct partitions in \d+. May be we
> want to indicate the partitions that are further partitioned. If user
> wants information about partitioned partitions, s/he can execute \d+
> on the partition, repeating this process to any desired level. This
> would work well in the interactive mode, keeping the output of \d+
> manageable. Further someone writing a script to consume \d+ output of
> a multi-level partitioned table, can code the above process in a
> script.
>
> Thinking about how to display partition which are further partitioned,
> there are two options. Assume a partitioned table t1 with partitions
> t1p1, which is further partitioned and t1p2. One could display \d+ t1
> as
>
> \d+ t1
>                                     Table "public.t1"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> --------+---------+-----------+----------+---------+---------+--------------+-------------
>  a      | integer |           | not null |         | plain   |              |
> Partition key: RANGE (a)
> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS
>             t1p2 FOR VALUES FROM (100) TO (200)
>
> OR
>
> \d+ t1
>                                     Table "public.t1"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> --------+---------+-----------+----------+---------+---------+--------------+-------------
>  a      | integer |           | not null |         | plain   |              |
> Partition key: RANGE (a)
> Partitions: t1p1 FOR VALUES FROM (0) TO (100), PARTITION BY LIST(a)
>             t1p2 FOR VALUES FROM (100) TO (200)
>
> To me the first option looks fine.

+1

lowercase please

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

On Mon, Mar 6, 2017 at 10:55 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 6 March 2017 at 04:00, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> On Mon, Mar 6, 2017 at 8:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> On 6 March 2017 at 00:51, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>> On 2017/03/05 16:20, Simon Riggs wrote:
>>>>> I notice also that
>>>>>   \d+ <tablename>
>>>>> does not show which partitions have subpartitions.
>>>>
>>>> Do you mean showing just whether a partition is itself partitioned or
>>>> showing its partitions and so on (because those partitions may themselves
>>>> be partitioned)?  Maybe, we could do the former.
>>>
>>> I think \d+ should show the full information, in some form.
>>
>> For a multi-level inheritance hierarchy, we don't show children which
>> are further inherited. Same behaviour has been carried over to
>> partitioning. I don't say that that's good or bad.
>>
>> Given the recursive structure of partitioned tables, it looks readable
>> and manageable to print only the direct partitions in \d+. May be we
>> want to indicate the partitions that are further partitioned. If user
>> wants information about partitioned partitions, s/he can execute \d+
>> on the partition, repeating this process to any desired level. This
>> would work well in the interactive mode, keeping the output of \d+
>> manageable. Further someone writing a script to consume \d+ output of
>> a multi-level partitioned table, can code the above process in a
>> script.
>>
>> Thinking about how to display partition which are further partitioned,
>> there are two options. Assume a partitioned table t1 with partitions
>> t1p1, which is further partitioned and t1p2. One could display \d+ t1
>> as
>>
>> \d+ t1
>>                                     Table "public.t1"
>>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
>> target | Description
>> --------+---------+-----------+----------+---------+---------+--------------+-------------
>>  a      | integer |           | not null |         | plain   |              |
>> Partition key: RANGE (a)
>> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS
>>             t1p2 FOR VALUES FROM (100) TO (200)
>>
>> OR
>>
>> \d+ t1
>>                                     Table "public.t1"
>>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
>> target | Description
>> --------+---------+-----------+----------+---------+---------+--------------+-------------
>>  a      | integer |           | not null |         | plain   |              |
>> Partition key: RANGE (a)
>> Partitions: t1p1 FOR VALUES FROM (0) TO (100), PARTITION BY LIST(a)
>>             t1p2 FOR VALUES FROM (100) TO (200)
>>
>> To me the first option looks fine.
>
> +1

Just to confirm, you want the output to look like this
>> \d+ t1
>>                                     Table "public.t1"
>>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
>> target | Description
>> --------+---------+-----------+----------+---------+---------+--------------+-------------
>>  a      | integer |           | not null |         | plain   |              |
>> Partition key: RANGE (a)
>> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS
>>             t1p2 FOR VALUES FROM (100) TO (200)

>
> lowercase please

Except for HAS PARTITIONS, everything is part of today's output. Given
the current output, HAS PARTITIONS should be in upper case.

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

On 6 March 2017 at 05:29, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

> Just to confirm, you want the output to look like this
>>> \d+ t1
>>>                                     Table "public.t1"
>>>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
>>> target | Description
>>> --------+---------+-----------+----------+---------+---------+--------------+-------------
>>>  a      | integer |           | not null |         | plain   |              |
>>> Partition key: RANGE (a)
>>> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS
>>>             t1p2 FOR VALUES FROM (100) TO (200)
>
>>
>> lowercase please
>
> Except for HAS PARTITIONS, everything is part of today's output. Given
> the current output, HAS PARTITIONS should be in upper case.

"has partitions" is not part of the DDL, whereas "FOR VALUES FROM (0)
TO (100)" is. So ISTM sensible to differentiate between DDL and
non-ddl using upper and lower case.

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

On Mon, Mar 6, 2017 at 11:05 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 6 March 2017 at 05:29, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>
>> Just to confirm, you want the output to look like this
>>>> \d+ t1
>>>>                                     Table "public.t1"
>>>>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
>>>> target | Description
>>>> --------+---------+-----------+----------+---------+---------+--------------+-------------
>>>>  a      | integer |           | not null |         | plain   |              |
>>>> Partition key: RANGE (a)
>>>> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS
>>>>             t1p2 FOR VALUES FROM (100) TO (200)
>>
>>>
>>> lowercase please
>>
>> Except for HAS PARTITIONS, everything is part of today's output. Given
>> the current output, HAS PARTITIONS should be in upper case.
>
> "has partitions" is not part of the DDL, whereas "FOR VALUES FROM (0)
> TO (100)" is. So ISTM sensible to differentiate between DDL and
> non-ddl using upper and lower case.
>

Make sense. Will try to cook up a patch soon.

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

On 2017/03/06 14:25, Simon Riggs wrote:
> On 6 March 2017 at 04:00, Ashutosh Bapat wrote:
>> Thinking about how to display partition which are further partitioned,
>> there are two options. Assume a partitioned table t1 with partitions
>> t1p1, which is further partitioned and t1p2. One could display \d+ t1
>> as
>>
>> \d+ t1
>>                                     Table "public.t1"
>>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
>> target | Description
>> --------+---------+-----------+----------+---------+---------+--------------+-------------
>>  a      | integer |           | not null |         | plain   |              |
>> Partition key: RANGE (a)
>> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS
>>             t1p2 FOR VALUES FROM (100) TO (200)
>>
>> OR
>>
>> \d+ t1
>>                                     Table "public.t1"
>>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
>> target | Description
>> --------+---------+-----------+----------+---------+---------+--------------+-------------
>>  a      | integer |           | not null |         | plain   |              |
>> Partition key: RANGE (a)
>> Partitions: t1p1 FOR VALUES FROM (0) TO (100), PARTITION BY LIST(a)
>>             t1p2 FOR VALUES FROM (100) TO (200)
>>
>> To me the first option looks fine.
> 
> +1
> 
> lowercase please

+1

Thanks,
Amit

On Mon, Mar 6, 2017 at 11:12 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Mon, Mar 6, 2017 at 11:05 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 6 March 2017 at 05:29, Ashutosh Bapat
>> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>>> Just to confirm, you want the output to look like this
>>>>> \d+ t1
>>>>>                                     Table "public.t1"
>>>>>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
>>>>> target | Description
>>>>> --------+---------+-----------+----------+---------+---------+--------------+-------------
>>>>>  a      | integer |           | not null |         | plain   |              |
>>>>> Partition key: RANGE (a)
>>>>> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS
>>>>>             t1p2 FOR VALUES FROM (100) TO (200)
>>>
>>>>
>>>> lowercase please
>>>
>>> Except for HAS PARTITIONS, everything is part of today's output. Given
>>> the current output, HAS PARTITIONS should be in upper case.
>>
>> "has partitions" is not part of the DDL, whereas "FOR VALUES FROM (0)
>> TO (100)" is. So ISTM sensible to differentiate between DDL and
>> non-ddl using upper and lower case.
>>
>
> Make sense. Will try to cook up a patch soon.

here's the patch. I have added a testcase in insert.sql to test \d+
output for a partitioned table which has partitions which are further
partitioned and also some partitions which are not partitioned
themselves. I have also refactored a statement few lines above,
replacing an if condition with ? : operator similar to code few lines
below.

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

On 2017/03/06 18:19, Ashutosh Bapat wrote:
> On Mon, Mar 6, 2017 at 11:12 AM, Ashutosh Bapat wrote:
>> On Mon, Mar 6, 2017 at 11:05 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> "has partitions" is not part of the DDL, whereas "FOR VALUES FROM (0)
>>> TO (100)" is. So ISTM sensible to differentiate between DDL and
>>> non-ddl using upper and lower case.
>>>
>>
>> Make sense. Will try to cook up a patch soon.
> 
> here's the patch. I have added a testcase in insert.sql to test \d+
> output for a partitioned table which has partitions which are further
> partitioned and also some partitions which are not partitioned
> themselves. I have also refactored a statement few lines above,
> replacing an if condition with ? : operator similar to code few lines
> below.

Thanks for cooking up the patch.  Looks good and works as expected.
Regression tests pass.

About the other statement you changed, I just realized that we should
perhaps do one more thing.  Show the Number of partitions, even if it's 0.
 In case of inheritance, the parent table stands on its own when there are
no child tables, but a partitioned table doesn't in the same sense.  I
tried to implement that in attached patch 0002.  Example below:

create table p (a int) partition by list (a);
\d p
<snip>
Partition key: LIST (a)
Number of partitions: 0

\d+ p
<snip>
Partition key: LIST (a)
Number of partitions: 0

create table p1 partition of p for values in (1);
\d p
<snip>
Partition key: LIST (a)
Number of partitions: 1 (Use \d+ to list them.)

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

0001 is your original patch.

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

>
> About the other statement you changed, I just realized that we should
> perhaps do one more thing.  Show the Number of partitions, even if it's 0.
>  In case of inheritance, the parent table stands on its own when there are
> no child tables, but a partitioned table doesn't in the same sense.  I
> tried to implement that in attached patch 0002.  Example below:
>
> create table p (a int) partition by list (a);
> \d p
> <snip>
> Partition key: LIST (a)
> Number of partitions: 0
>
> \d+ p
> <snip>
> Partition key: LIST (a)
> Number of partitions: 0
>
> create table p1 partition of p for values in (1);
> \d p
> <snip>
> Partition key: LIST (a)
> Number of partitions: 1 (Use \d+ to list them.)
>
> \d+ p
> <snip>
> Partition key: LIST (a)
> Partitions: p1 FOR VALUES IN (1)

I liked that. PFA 0002 updated. I changed one of \d output to \d+ to
better test partitioned tables without partitions in verbose and
non-verbose mode. Also, refactored the your code to have less number
of conditions. Please let me know if it looks good.

-- 
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/03/08 18:27, Ashutosh Bapat wrote:
>>
>> About the other statement you changed, I just realized that we should
>> perhaps do one more thing.  Show the Number of partitions, even if it's 0.
>>  In case of inheritance, the parent table stands on its own when there are
>> no child tables, but a partitioned table doesn't in the same sense.  I
>> tried to implement that in attached patch 0002.  Example below:
>>
>> create table p (a int) partition by list (a);
>> \d p
>> <snip>
>> Partition key: LIST (a)
>> Number of partitions: 0
>>
>> \d+ p
>> <snip>
>> Partition key: LIST (a)
>> Number of partitions: 0
>>
>> create table p1 partition of p for values in (1);
>> \d p
>> <snip>
>> Partition key: LIST (a)
>> Number of partitions: 1 (Use \d+ to list them.)
>>
>> \d+ p
>> <snip>
>> Partition key: LIST (a)
>> Partitions: p1 FOR VALUES IN (1)
> 
> I liked that. PFA 0002 updated. I changed one of \d output to \d+ to
> better test partitioned tables without partitions in verbose and
> non-verbose mode. Also, refactored the your code to have less number
> of conditions. Please let me know if it looks good.

Thanks, looks good.

Regards,
Amit

Added this to 2017/7 commitfest to keep a track of it.

On Wed, Mar 8, 2017 at 3:39 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/03/08 18:27, Ashutosh Bapat wrote:
>>>
>>> About the other statement you changed, I just realized that we should
>>> perhaps do one more thing.  Show the Number of partitions, even if it's 0.
>>>  In case of inheritance, the parent table stands on its own when there are
>>> no child tables, but a partitioned table doesn't in the same sense.  I
>>> tried to implement that in attached patch 0002.  Example below:
>>>
>>> create table p (a int) partition by list (a);
>>> \d p
>>> <snip>
>>> Partition key: LIST (a)
>>> Number of partitions: 0
>>>
>>> \d+ p
>>> <snip>
>>> Partition key: LIST (a)
>>> Number of partitions: 0
>>>
>>> create table p1 partition of p for values in (1);
>>> \d p
>>> <snip>
>>> Partition key: LIST (a)
>>> Number of partitions: 1 (Use \d+ to list them.)
>>>
>>> \d+ p
>>> <snip>
>>> Partition key: LIST (a)
>>> Partitions: p1 FOR VALUES IN (1)
>>
>> I liked that. PFA 0002 updated. I changed one of \d output to \d+ to
>> better test partitioned tables without partitions in verbose and
>> non-verbose mode. Also, refactored the your code to have less number
>> of conditions. Please let me know if it looks good.
>
> Thanks, looks good.
>
> Regards,
> Amit
>
>



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

Hi,
Thomas's application to track patches told me that this patch needs
rebase. It also required some changes to the code. Here's the updated
version. I have squashed those two patches together.

On Tue, Mar 14, 2017 at 6:35 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> Added this to 2017/7 commitfest to keep a track of it.
>
> On Wed, Mar 8, 2017 at 3:39 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/03/08 18:27, Ashutosh Bapat wrote:
>>>>
>>>> About the other statement you changed, I just realized that we should
>>>> perhaps do one more thing.  Show the Number of partitions, even if it's 0.
>>>>  In case of inheritance, the parent table stands on its own when there are
>>>> no child tables, but a partitioned table doesn't in the same sense.  I
>>>> tried to implement that in attached patch 0002.  Example below:
>>>>
>>>> create table p (a int) partition by list (a);
>>>> \d p
>>>> <snip>
>>>> Partition key: LIST (a)
>>>> Number of partitions: 0
>>>>
>>>> \d+ p
>>>> <snip>
>>>> Partition key: LIST (a)
>>>> Number of partitions: 0
>>>>
>>>> create table p1 partition of p for values in (1);
>>>> \d p
>>>> <snip>
>>>> Partition key: LIST (a)
>>>> Number of partitions: 1 (Use \d+ to list them.)
>>>>
>>>> \d+ p
>>>> <snip>
>>>> Partition key: LIST (a)
>>>> Partitions: p1 FOR VALUES IN (1)
>>>
>>> I liked that. PFA 0002 updated. I changed one of \d output to \d+ to
>>> better test partitioned tables without partitions in verbose and
>>> non-verbose mode. Also, refactored the your code to have less number
>>> of conditions. Please let me know if it looks good.
>>
>> Thanks, looks good.
>>
>> Regards,
>> Amit
>>
>>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company



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

On 2017/09/04 13:51, Ashutosh Bapat wrote:
> Hi,
> Thomas's application to track patches told me that this patch needs
> rebase. It also required some changes to the code. Here's the updated
> version. I have squashed those two patches together.

Thanks for the rebased patch.

Would it make sense to post them in a new thread?  When this message
popped into my inbox in a thread titled "dropping partitioned tables
without CASCADE", I wondered for a second if there is still something left
to be done about that. :)

Thanks,
Amit

On Mon, Sep 4, 2017 at 10:34 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Hi Ashutosh,
>
> On 2017/09/04 13:51, Ashutosh Bapat wrote:
>> Hi,
>> Thomas's application to track patches told me that this patch needs
>> rebase. It also required some changes to the code. Here's the updated
>> version. I have squashed those two patches together.
>
> Thanks for the rebased patch.
>
> Would it make sense to post them in a new thread?  When this message
> popped into my inbox in a thread titled "dropping partitioned tables
> without CASCADE", I wondered for a second if there is still something left
> to be done about that. :)

That would be good. We will have to update the thread in commitfest
entry though.

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

if (tuples > 0)            {
-                if (tableinfo.relkind != RELKIND_PARTITIONED_TABLE)
-                    printfPQExpBuffer(&buf, _("Number of child tables: %d (Use \\d+ to list them.)"), tuples);
-                else
-                    printfPQExpBuffer(&buf, _("Number of partitions: %d (Use \\d+ to list them.)"), tuples);
+                printfPQExpBuffer(&buf, _("Number of %s: %d (Use \\d+ to list them.)"), ct, tuples);
printTableAddFooter(&cont,buf.data);            }
 

Please don't do this, because it breaks translatability.  I think the
original is fine.

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

On Mon, Sep 4, 2017 at 3:48 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
>                         if (tuples > 0)
>                         {
> -                               if (tableinfo.relkind != RELKIND_PARTITIONED_TABLE)
> -                                       printfPQExpBuffer(&buf, _("Number of child tables: %d (Use \\d+ to list
them.)"),tuples);
 
> -                               else
> -                                       printfPQExpBuffer(&buf, _("Number of partitions: %d (Use \\d+ to list
them.)"),tuples);
 
> +                               printfPQExpBuffer(&buf, _("Number of %s: %d (Use \\d+ to list them.)"), ct, tuples);
>                                 printTableAddFooter(&cont, buf.data);
>                         }
>
> Please don't do this, because it breaks translatability.  I think the
> original is fine.
>

We have used this style in the "else" case of if (!verbose). So, I
just copied it. I have removed that change in the attached patch.

-- 
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 Wed, Sep 6, 2017 at 1:06 PM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
>
> 2) Add partition to the foo;
>
> create table foo_p1 partition of foo for values in (1, 2, 3) partition by
> list (b);
>
> postgres=# \d foo
>                 Table "public.foo"
>  Column |  Type   | Collation | Nullable | Default
> --------+---------+-----------+----------+---------
>  a      | integer |           |          |
>  b      | integer |           |          |
> Partition key: LIST (a)
> Number of partitions: 1 (Use \d+ to list them.)
>
> postgres=# \d+ foo
>                                     Table "public.foo"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats target
> | Description
> --------+---------+-----------+----------+---------+---------+--------------+-------------
>  a      | integer |           |          |         | plain   |
> |
>  b      | integer |           |          |         | plain   |
> |
> Partition key: LIST (a)
> Partitions: foo_p1 FOR VALUES IN (1, 2, 3) has partitions
>
> Above verbose output for foo says, foo_p1 "has partitions". But if I do
>
> postgres=# \d foo_p1
>                Table "public.foo_p1"
>  Column |  Type   | Collation | Nullable | Default
> --------+---------+-----------+----------+---------
>  a      | integer |           |          |
>  b      | integer |           |          |
> Partition of: foo FOR VALUES IN (1, 2, 3)
> Partition key: LIST (b)
> Number of partitions: 0
>
> it tell "Number of partitions: 0".
>
> I feel like information is conflicting with each other. AFAIU, idea about
> adding
> "has partitions" was to let know that it's a partitioned table. So can you
> directly
> add the "is partitioned" in place of "has partitions"?
>
> Did those change in the attached patch and update regression expected
> output.
>

Looks better.

> Also run pgindent on the patch.
>

Thanks for the changes. The patch looks good to me.


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

On 2017/09/06 18:46, Rushabh Lathia wrote:
> Okay, I have marked this as ready for committer.

Thanks Ashutosh and Rushabh for rebasing and improving the patch.  Looks
good to me too.

Regards,
Amit

On 2017/09/06 19:14, Amit Langote wrote:
> On 2017/09/06 18:46, Rushabh Lathia wrote:
>> Okay, I have marked this as ready for committer.
> 
> Thanks Ashutosh and Rushabh for rebasing and improving the patch.  Looks
> good to me too.

Patch needed to be rebased after the default partitions patch went in, so
done.  Per build status on http://commitfest.cputube.org :)

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

Thanks Amit for taking care of this.

On Wed, Sep 13, 2017 at 6:31 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/09/06 19:14, Amit Langote wrote:
>> On 2017/09/06 18:46, Rushabh Lathia wrote:
>>> Okay, I have marked this as ready for committer.
>>
>> Thanks Ashutosh and Rushabh for rebasing and improving the patch.  Looks
>> good to me too.
>
> Patch needed to be rebased after the default partitions patch went in, so
> done.  Per build status on http://commitfest.cputube.org :)
>
> 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

Amit Langote wrote:
> On 2017/09/06 19:14, Amit Langote wrote:
> > On 2017/09/06 18:46, Rushabh Lathia wrote:
> >> Okay, I have marked this as ready for committer.
> > 
> > Thanks Ashutosh and Rushabh for rebasing and improving the patch.  Looks
> > good to me too.
> 
> Patch needed to be rebased after the default partitions patch went in, so
> done.  Per build status on http://commitfest.cputube.org :)

I think adding "is partitioned" at end of line isn't good; looks like a
phrase but isn't translatable.  Maybe add keyword PARTITIONED instead?

Having the DEFAULT partition show up in the middle of the list is weird.
Is it possible to put it at either start or end of the list?

-- 
Álvaro Herrera                https://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 Fri, Nov 3, 2017 at 1:42 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Amit Langote wrote:
>> On 2017/09/06 19:14, Amit Langote wrote:
>> > On 2017/09/06 18:46, Rushabh Lathia wrote:
>> >> Okay, I have marked this as ready for committer.
>> >
>> > Thanks Ashutosh and Rushabh for rebasing and improving the patch.  Looks
>> > good to me too.
>>
>> Patch needed to be rebased after the default partitions patch went in, so
>> done.  Per build status on http://commitfest.cputube.org :)
>
> I think adding "is partitioned" at end of line isn't good; looks like a
> phrase but isn't translatable.  Maybe add keyword PARTITIONED instead?

In that case may be we should separate bounds and "PARTITIONED" with a
",". "part_default DEFAULT, PARTITIONED" would read better than
"part_default DEFAULT PARTITIONED"?

>
> Having the DEFAULT partition show up in the middle of the list is weird.

Agreed. But that's true even without this patch.

> Is it possible to put it at either start or end of the list?
>

Right now, we could do that if we order the list by bound expression;
lexically DEFAULT would come before FOR VALUES ... . But that's not
future-safe; we may have a bound expression starting with A, B or C.
Beyond that it really gets tricky to order the partitions by bounds.

The goal of this patch is to mark the partitioned partitions as such
and show the number of partitions. While your suggestion is a valid
request, it's kind of beyond the scope of this patch. Someone might
want to extend this request and say that partitions should be listed
in the order of their bounds (I do feel that we should do some effort
in that direction). But I am not sure whether it should be done in
this patch.

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

Ashutosh Bapat wrote:
> On Fri, Nov 3, 2017 at 1:42 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > I think adding "is partitioned" at end of line isn't good; looks like a
> > phrase but isn't translatable.  Maybe add keyword PARTITIONED instead?
> 
> In that case may be we should separate bounds and "PARTITIONED" with a
> ",". "part_default DEFAULT, PARTITIONED" would read better than
> "part_default DEFAULT PARTITIONED"?

Hmm, I vote +0.5 for the comma.

> > Having the DEFAULT partition show up in the middle of the list is weird.
> 
> Agreed. But that's true even without this patch.

Yes.

> > Is it possible to put it at either start or end of the list?
> 
> Right now, we could do that if we order the list by bound expression;
> lexically DEFAULT would come before FOR VALUES ... . But that's not
> future-safe; we may have a bound expression starting with A, B or C.
> Beyond that it really gets tricky to order the partitions by bounds.

I was just thinking in changing the query to be "order by
is_the_default_partition, partition_name" instead of just "order by
partition_name".  Sorting by bounds rather than name (a feature whose
worth should definitely be discussed separately IMV) sounds a lot more
complicated.

-- 
Álvaro Herrera                https://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/11/03 21:39, Alvaro Herrera wrote:
> Ashutosh Bapat wrote:
>> On Fri, Nov 3, 2017 at 1:42 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 
>>> I think adding "is partitioned" at end of line isn't good; looks like a
>>> phrase but isn't translatable.  Maybe add keyword PARTITIONED instead?
>>
>> In that case may be we should separate bounds and "PARTITIONED" with a
>> ",". "part_default DEFAULT, PARTITIONED" would read better than
>> "part_default DEFAULT PARTITIONED"?
> 
> Hmm, I vote +0.5 for the comma.

Me too.

>>> Is it possible to put it at either start or end of the list?
>>
>> Right now, we could do that if we order the list by bound expression;
>> lexically DEFAULT would come before FOR VALUES ... . But that's not
>> future-safe; we may have a bound expression starting with A, B or C.
>> Beyond that it really gets tricky to order the partitions by bounds.
> 
> I was just thinking in changing the query to be "order by
> is_the_default_partition, partition_name" instead of just "order by
> partition_name".  Sorting by bounds rather than name (a feature whose
> worth should definitely be discussed separately IMV) sounds a lot more
> complicated.

Yeah, it sounds like a desirable feature, but as you both say, should be
discussed separately.  Since the facility to order partitions in the bound
order is internal to the server yet, we'd need some new server-side
functionality to expose the same with sane SQL-callable interface, which
clearly needs its own separate discussion.

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

Somehow the earlier patches missed qualifying pg_get_expr() by
pg_catalog. Fixed it along with annotating the partitioned partition
as ", PARTITIONED".

On Fri, Nov 3, 2017 at 6:09 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>
>> Right now, we could do that if we order the list by bound expression;
>> lexically DEFAULT would come before FOR VALUES ... . But that's not
>> future-safe; we may have a bound expression starting with A, B or C.
>> Beyond that it really gets tricky to order the partitions by bounds.
>
> I was just thinking in changing the query to be "order by
> is_the_default_partition, partition_name" instead of just "order by
> partition_name".  Sorting by bounds rather than name (a feature whose
> worth should definitely be discussed separately IMV) sounds a lot more
> complicated.

Right now we don't have a catalog column or a SQL function which can
tell whether a given partition is default partition based on the
partition bounds or otherwise. That's what it seemed when you
suggested ordering by "is_the_default_partition". Instead I have
ordered the partitions by pg_catalog.pg_get_expr(...) = 'DEFAULT'. We
can introduce a SQL function which takes child and parent oids and
return true if it's default partition and use that here, but that
seems more than what you are suggesting here. I have added that as a
separate patch.

If we tackle the problem of listing partitions by their bounds
somehow, DEFAULT partition listing would be tackled anyway. So, may be
we should leave it outside the scope of this patch.

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