Обсуждение: ToDo: show size of partitioned table

On Fri, Jun 1, 2018 at 12:56 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hi
>
> postgres=# SELECT count(*) from data;
> ┌─────────┐
> │  count  │
> ╞═════════╡
> │ 1000000 │
> └─────────┘
> (1 row)
>
> \dt+ can display actual size of partitioned table data - now zero is
> displayed
>
> postgres=# \dt+ data
>                     List of relations
> ┌────────┬──────┬───────┬───────┬─────────┬─────────────┐
> │ Schema │ Name │ Type  │ Owner │  Size   │ Description │
> ╞════════╪══════╪═══════╪═══════╪═════════╪═════════════╡
> │ public │ data │ table │ pavel │ 0 bytes │             │
> └────────┴──────┴───────┴───────┴─────────┴─────────────┘
> (1 row)

I think we should at least display "Type" as "partitioned table" for a
partitioned table, so that it's easy to understand why the size is 0;
partitioned tables do not hold any data by themselves.

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

On 2018/06/02 0:15, Ashutosh Bapat wrote:
> I think we should at least display "Type" as "partitioned table" for a
> partitioned table, so that it's easy to understand why the size is 0;
> partitioned tables do not hold any data by themselves.

There was a long discussion last year (during PG 10 beta period), such as
[1], and it seems most of us agreed to doing the above.  Maybe, we should
finally do it for PG 12, if not PG 11.

Regarding showing the size of partitioned tables, there are many opinions
and it's not clear if showing it in \dt itself is appropriate.  For one,
there is no pg_relation_size() or pg_table_size() equivalent in the
backend for aggregating the size of all tables in a partition tree and I
think people are not quite on board about having such a function in the
backend [2].

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/flat/7dfc13c5-d6b7-1ff1-4bef-d75d6d2f76d9%40lab.ntt.co.jp

[2]
https://www.postgresql.org/message-id/flat/495cec7e-f8d9-7e13-4807-90dbf4eec4ea%40lab.ntt.co.jp

On 2018/06/20 16:21, Pavel Stehule wrote:
> 2018-06-20 7:44 GMT+02:00 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>:
> 
>> On 2018/06/02 0:15, Ashutosh Bapat wrote:
>>> I think we should at least display "Type" as "partitioned table" for a
>>> partitioned table, so that it's easy to understand why the size is 0;
>>> partitioned tables do not hold any data by themselves.
>>
>> There was a long discussion last year (during PG 10 beta period), such as
>> [1], and it seems most of us agreed to doing the above.  Maybe, we should
>> finally do it for PG 12, if not PG 11.
>>
>> Regarding showing the size of partitioned tables, there are many opinions
>> and it's not clear if showing it in \dt itself is appropriate.  For one,
>> there is no pg_relation_size() or pg_table_size() equivalent in the
>> backend for aggregating the size of all tables in a partition tree and I
>> think people are not quite on board about having such a function in the
>> backend [2].
> 
> Now, the number of partitions can be low, but if the Postgres can better
> process high number of partitions, then for some tables we can have
> hundreds partitions.
> 
> Then usual \dt can be not too much usable. The aggregation can be done on
> client side. But maybe this idea is premature. Now, for PG 12, we can start
> with
> 
> \dtP+ command for showing partition tables only with aggregate size via all
> related partitions.
> 
> Is it acceptable idea?

Do you mean \dt continues to show size 0 for partitioned tables, but with
the new option (\dtP+) shows the actual size by aggregating across
partitions?  +1 to such a feature, but we need to agree on an acceptable
implementation for that.  How does the aggregation happen:

1. In a new dedicated function in the backend (parallel to pg_table_size)?

or

2. psql issues a separate query to compute the total size of a partition
   tree

For option 2, I had posted a patch that simplifies writing such a query
and posted that here:

https://www.postgresql.org/message-id/7a9c5328-5328-52a3-2a3d-bf1434b4dd1d%40lab.ntt.co.jp

With that patch, the query to get the total size of a partition tree
becomes as simple as:

select  sum(pg_table_size(p)) as size
from    pg_get_inheritance_tables('partitioned_table_name') p

Thanks,
Amit

On 2018/06/20 16:50, Pavel Stehule wrote:
> 2018-06-20 9:44 GMT+02:00 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>:
>> Do you mean \dt continues to show size 0 for partitioned tables, but with
>> the new option (\dtP+) shows the actual size by aggregating across
>> partitions?  +1 to such a feature, but we need to agree on an acceptable
>> implementation for that.  How does the aggregation happen:
>>
> 
> yes - my proposal is no change for \dt for now. I think so we will have to
> change it, when partitioning will be more common and number of partitions
> will be high. But it is not today.
> 
> \dtP shows only partitions tables (like \dtS shows only system tables),
> with "+" shows sum of all related partitions.

Ah, okay.  That makes sense.

>> 1. In a new dedicated function in the backend (parallel to pg_table_size)?
>>
>> or
>>
>> 2. psql issues a separate query to compute the total size of a partition
>>    tree
>>
> 
> In this moment we can simply do sum on client side, so it is related to @2.

I see, okay.

>> For option 2, I had posted a patch that simplifies writing such a query
>> and posted that here:
>>
>> https://www.postgresql.org/message-id/7a9c5328-5328-52a3-
>> 2a3d-bf1434b4dd1d%40lab.ntt.co.jp
>>
>> With that patch, the query to get the total size of a partition tree
>> becomes as simple as:
>>
>> select  sum(pg_table_size(p)) as size
>> from    pg_get_inheritance_tables('partitioned_table_name') p
>>
> 
> good to know it. Thank you. Do you think so your patch should be included
> to this feature or will be processed independently?

It seems that it would be useful on its own, as people may want to do
various things once we provide them pg_get_inheritance_table.

Thanks,
Amit

Hi Pavel.

On 2018/07/23 20:46, Pavel Stehule wrote:
> Hi
> 
> I am sending a prototype of patch. Now, it calculates size of partitioned
> tables with recursive query. When any more simple method will be possible,
> the size calculation will be changed.
> 
> postgres=# \dt+
>                        List of relations
> +--------+------------+-------+-------+---------+-------------+
> | Schema |    Name    | Type  | Owner |  Size   | Description |
> +--------+------------+-------+-------+---------+-------------+
> | public | data       | table | pavel | 0 bytes |             |
> | public | data_2016  | table | pavel | 15 MB   |             |
> | public | data_2017  | table | pavel | 15 MB   |             |
> | public | data_other | table | pavel | 11 MB   |             |
> +--------+------------+-------+-------+---------+-------------+
> (4 rows)
> 
> postgres=# \dP+
>           List of partitioned tables
> +--------+------+-------+-------+-------------+
> | Schema | Name | Owner | Size  | Description |
> +--------+------+-------+-------+-------------+
> | public | data | pavel | 42 MB |             |
> +--------+------+-------+-------+-------------+
> (1 row)

This looks nice, although I haven't looked at the patch yet.  Also, as you
said, we could later replace the method of directly querying pg_inherits
by something else.

> p.s. Another patch can be replacement of relation type from "table" to
> "partitioned table"
> 
> postgres=# \dt+
>                              List of relations
> +--------+------------+-------------------+-------+---------+-------------+
> | Schema |    Name    |       Type        | Owner |  Size   | Description |
> +--------+------------+-------------------+-------+---------+-------------+
> | public | data       | partitioned table | pavel | 0 bytes |             |
> | public | data_2016  | table             | pavel | 15 MB   |             |
> | public | data_2017  | table             | pavel | 15 MB   |             |
> | public | data_other | table             | pavel | 11 MB   |             |
> +--------+------------+-------------------+-------+---------+-------------+
> (4 rows)
> 
> A patch is simple for this case
> 
> diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
> index c3bdf8555d..491e58eb29 100644
> --- a/src/bin/psql/describe.c
> +++ b/src/bin/psql/describe.c
> @@ -3490,8 +3490,8 @@ listTables(const char *tabtypes, const char *pattern,
> bool verbose, bool showSys
>                       gettext_noop("sequence"),
>                       gettext_noop("special"),
>                       gettext_noop("foreign table"),
> -                     gettext_noop("table"),    /* partitioned table */
> -                     gettext_noop("index"),    /* partitioned index */
> +                     gettext_noop("partitioned table"),    /* partitioned
> table */
> +                     gettext_noop("partitioned index"),    /* partitioned
> index */
>                       gettext_noop("Type"),
>                       gettext_noop("Owner"));

Inclined to +1 this, too.  Although, one might ask why partitioned tables
are called so only in the psql's output, but not in the backend's error
messages, for example, as was discussed in the past.

Thanks,
Amit

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

Hi,

I'm with Melanie Plageman running the SVPUG Patch Review Meetup. I tested this feature on my Mac. The patch applied
cleanlyon master, and the feature worked as expected with the SQL at the bottom of this email (Jesse Zhang suggested
thetwo-level partitioning). installcheck passed but installcheck-world did not.
 

I do have a feedback on the implementation. The code tries to support older PostgreSQL server versions when declarative
partitionswere not supported before version 10 (relkind value of 'p'). Those versions will never return any result from
thequery being built. So I would suggest an early return from the function. The upside would be that the query building
wouldbe simpler. I can make patch implementing that suggestion if you want.
 

Sincerely,
-- Mathias Brossard


CREATE TABLE partition (
    part  int not null,
    value int not null
) PARTITION BY RANGE (part);

CREATE TABLE partition_0 PARTITION OF partition FOR VALUES FROM (0) TO (10);
CREATE TABLE partition_1 PARTITION OF partition FOR VALUES FROM (10) TO (20);
CREATE TABLE partition_2 PARTITION OF partition FOR VALUES FROM (20) TO (30);
CREATE TABLE partition_3 PARTITION OF partition FOR VALUES FROM (30) TO (40);
CREATE TABLE partition_4 PARTITION OF partition FOR VALUES FROM (40) TO (50);
CREATE TABLE partition_5 PARTITION OF partition FOR VALUES FROM (50) TO (60);
CREATE TABLE partition_6 PARTITION OF partition FOR VALUES FROM (60) TO (70);
CREATE TABLE partition_7 PARTITION OF partition FOR VALUES FROM (70) TO (80);
CREATE TABLE partition_8 PARTITION OF partition FOR VALUES FROM (80) TO (90);
CREATE TABLE partition_9 (
    part  int not null,
    value int not null
) PARTITION BY RANGE (part);

CREATE TABLE partition_9a PARTITION OF partition_9 FOR VALUES FROM (90) TO (95);
CREATE TABLE partition_9b PARTITION OF partition_9 FOR VALUES FROM (95) TO (100);
ALTER TABLE partition ATTACH PARTITION partition_9 FOR VALUES FROM (90) TO (100);

INSERT INTO partition SELECT i % 100 AS part, i AS value FROM generate_series(1, 1000000) AS i;

-------------------
-- Below is the resulting output

test=# \dt+
                         List of relations
 Schema |     Name     | Type  |   Owner   |  Size   | Description
--------+--------------+-------+-----------+---------+-------------
 public | partition    | table | mbrossard | 0 bytes |
 public | partition_0  | table | mbrossard | 3568 kB |
 public | partition_1  | table | mbrossard | 3568 kB |
 public | partition_2  | table | mbrossard | 3568 kB |
 public | partition_3  | table | mbrossard | 3568 kB |
 public | partition_4  | table | mbrossard | 3568 kB |
 public | partition_5  | table | mbrossard | 3568 kB |
 public | partition_6  | table | mbrossard | 3568 kB |
 public | partition_7  | table | mbrossard | 3568 kB |
 public | partition_8  | table | mbrossard | 3568 kB |
 public | partition_9  | table | mbrossard | 0 bytes |
 public | partition_9a | table | mbrossard | 1800 kB |
 public | partition_9b | table | mbrossard | 1800 kB |
(13 rows)

test=# \dP+
                List of partitioned tables
 Schema |    Name     |   Owner   |  Size   | Description
--------+-------------+-----------+---------+-------------
 public | partition   | mbrossard | 35 MB   |
 public | partition_9 | mbrossard | 3600 kB |
(2 rows)

test=# \dP+ *9
                List of partitioned tables
 Schema |    Name     |   Owner   |  Size   | Description
--------+-------------+-----------+---------+-------------
 public | partition_9 | mbrossard | 3600 kB |
(1 row)

On Wed, Jul 25, 2018 at 06:09:05PM +0900, Amit Langote wrote:
> This looks nice, although I haven't looked at the patch yet.  Also, as you
> said, we could later replace the method of directly querying pg_inherits
> by something else.

Yes, at the end we may be looking at something like that which would
avoid the need of any WITH RECURSIVE queries:
https://commitfest.postgresql.org/19/1694/

Tha patch still applies, but I think that we'd want to do something for
the partition tree function first, and then come to this one.  Moved to
next CF.
--
Michael

Hi Mathias, Pavel,

On 2018/08/17 12:26, Mathias Brossard wrote:
> On Thu, Aug 16, 2018 at 12:46 AM Pavel Stehule <pavel.stehule@gmail.com>
>>
>> This is question - maybe we can support older partitioning based on only
>> inheritance - and the query can be more exact on PostgreSQL 10 and newer.
>>
>> Please, send any patch. You are welcome.
> 
> In my very humble opinion, I would restrict the definition of partitions to
> declarative partitioning. My justification would be that partitions all use
> inheritance, but not all inheritance is a partition (how would you handle
> multiple inheritance).
> 
> See patch attached that fails (in a way similar to other features) when
> connected to servers with version earlier than 10.0.

The patch to add the pg_partition_tree() function was just committed:

Add pg_partition_tree to display information about partitions
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d5eec4eefde70

Could one of you please revise the patch to use that function to produce
the output of \dP+?

Note that pg_partition_tree simply scans the underlying catalog to get all
the tables, so if you pass it a table that's not partitioned (relkind ==
'r'), but has inheritance children, the children will be returned in its
output.  So, if you want to limit the output of \dP to partitioned tables,
be sure to include relkind = 'p' condition in the query.

Thanks,
Amit

On Tue, Oct 30, 2018 at 8:04 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> út 30. 10. 2018 v 7:52 odesílatel Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> napsal:
>> The patch to add the pg_partition_tree() function was just committed:
>>
>> Add pg_partition_tree to display information about partitions
>> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d5eec4eefde70
>>
>> Could one of you please revise the patch to use that function to produce
>> the output of \dP+?
>
>
> here it is.
>
> It is based on Mathias's patch. Although we can use pg_partition_tree on PostgreSQL, we still should to support
PostgreSQL10, 11 where this function is not available 

Ah, I forgot that psql will need to consider 10 and 11 servers too.

Thanks,
Amit

On Tue, Oct 30, 2018 at 09:24:01PM +0900, Amit Langote wrote:
>> It is based on Mathias's patch. Although we can use
>> pg_partition_tree on PostgreSQL, we still should to support
>> PostgreSQL 10, 11 where this function is not available
>
> Ah, I forgot that psql will need to consider 10 and 11 servers too.

+    /* PostgreSQL 11 has pg_partition_tree function */
That's v12 here.

+    appendPQExpBuffer(&buf,
+    ",\n  (SELECT pg_catalog.pg_size_pretty(sum(pg_catalog.pg_table_size("
+    "relid)))\n"
+    "     FROM pg_partition_tree(c.oid)) AS \"%s\""
You need to do schema qualification for functions and relations.
--
Michael

On 2018/10/30 20:03, Pavel Stehule wrote:
> út 30. 10. 2018 v 7:52 odesílatel Amit Langote <
> Langote_Amit_f8@lab.ntt.co.jp> napsal:
>> Could one of you please revise the patch to use that function to produce
>> the output of \dP+?
>>
> 
> here it is.
> 
> It is based on Mathias's patch. Although we can use pg_partition_tree on
> PostgreSQL, we still should to support PostgreSQL 10, 11 where this
> function is not available

Thanks for updating the patch.  Just a couple of comments:

+        is used, a sum of size of related partitions and a description

I suggest:

is used, the sum of sizes of related partitions and associated description

+    appendPQExpBufferStr(&buf, "\nWHERE c.relkind IN ('p')\n");

I wonder if we should list partitioned indexes ('I') as well, because
their size information is not available with \di+.  But maybe, they should
have a separate command.

+    if (PQntuples(res) == 0 && !pset.quiet)
+    {
+        if (pattern)
+            psql_error("Did not find any relation named \"%s\".\n",
+                       pattern);
+        else
+            psql_error("Did not find any relations.\n");
+    }

I think we should use "partitioned table" instead of "relation" in the
above error messages, because this command is specifically finding
partitioned tables.

(If we decide to include partitioned indexes as well, then the above error
message should say "partitioned relation")

+    fprintf(output, _("  \\dP[+] [PATTERN]       list partitioned
tables\n"));

Again, if we include indexes, this should be "partitioned relations".

How about adding a couple of regression tests?

Thanks,
Amit

On 2018/10/31 15:30, Pavel Stehule wrote:
> st 31. 10. 2018 v 3:27 odesílatel Amit Langote <
> Langote_Amit_f8@lab.ntt.co.jp> napsal:
>> +    appendPQExpBufferStr(&buf, "\nWHERE c.relkind IN ('p')\n");
>>
>> I wonder if we should list partitioned indexes ('I') as well, because
>> their size information is not available with \di+.  But maybe, they should
>> have a separate command.
>>
> 
> I though about it too and I prefer separate command. Similar to \di+

Okay, maybe \dI+.

> I am not sure. Has not sense run this test over empty database, and some
> bigger database can increase running.
> 
> More the size can be platform depend.

Okay, sure.

Thanks,
Amit

On Wed, Oct 31, 2018 at 03:34:02PM +0900, Amit Langote wrote:
> On 2018/10/31 15:30, Pavel Stehule wrote:
>> I am not sure. Has not sense run this test over empty database, and some
>> bigger database can increase running.
>>
>> More the size can be platform depend.
>
> Okay, sure.

Well, that would mostly depend on the relation page size which is
defined at compilation, no?  The plans of some queries are unstable
depending on the page size so they would fail, but let's limit the
damage if possible.
--
Michael

On Wed, Oct 31, 2018 at 08:41:45AM +0100, Pavel Stehule wrote:
> I am not sure - I remember one private test that we did on our patches, and
> this tests fails sometimes on 32bits. So I afraid about stability.

That could be possible as well.  I think that you are right to be afraid
of such things, and keeping the tests portable a maximum is always a
good thing I think.
--
Michael

Hi,

On 2018/11/01 2:19, Pavel Stehule wrote:
> st 31. 10. 2018 v 7:34 odesílatel Amit Langote <
> Langote_Amit_f8@lab.ntt.co.jp> napsal:
>> On 2018/10/31 15:30, Pavel Stehule wrote:
>>> st 31. 10. 2018 v 3:27 odesílatel Amit Langote <
>>> Langote_Amit_f8@lab.ntt.co.jp> napsal:
>>>> +    appendPQExpBufferStr(&buf, "\nWHERE c.relkind IN ('p')\n");
>>>>
>>>> I wonder if we should list partitioned indexes ('I') as well, because
>>>> their size information is not available with \di+.  But maybe, they
>> should
>>>> have a separate command.
>>>>
>>>
>>> I though about it too and I prefer separate command. Similar to \di+
>>
>> Okay, maybe \dI+.
>>
>>
> what about combination
> 
> \dPt+ for partitined tables and size
> \dPi+ for indexes on partitioned tables and size
> \dP+ for total partition size (tables + indexes)

+1

Thanks,
Amit

On 2018/11/04 4:58, Pavel Stehule wrote:
> here is a patch

Thank you, Pavel.

Here are some comments.

I mentioned it during the last review, but maybe you missed it due to the
other discussion.

+        the pattern are listed.  If the form <literal>\dP+</literal>
+        is used, a sum of size of related partitions and a description
+        are also displayed.

+        the pattern are listed.  If the form <literal>\dPi+</literal>
+        is used, a sum of size of related indexes and a description
+        are also displayed.

+        the pattern are listed.  If the form <literal>\dPt+</literal>
+        is used, a sum of size of related indexes and a description
+        are also displayed.

I suggest:

"is used, the sum of sizes of related partitions / index partitions /
table partitions and associated description are also displayed."

Note that I also fixed the typo (indexes -> tables) in the last one.

Also, I wonder if we should mention in the description of \dP+ that the
displayed size considers the sizes of both the tables and indexes on the
individual partitions, because in the code, I see pg_total_relation_size
being used.  So, the text should be something like:

"is used, the sum of size of related partitions (including the table and
indexes, if any) and associated description are also displayed."

Code itself looks to me to be in good shape, except you seem to have also
missed Michael's comment upthread:

+            /* PostgreSQL 11 has pg_partition_tree function */

/* PostgreSQL 12 has pg_partition_tree function */

Thanks again.

Regards,
Amit

On 2018-Jul-23, Pavel Stehule wrote:

> p.s. Another patch can be replacement of relation type from "table" to
> "partitioned table"
> 
> postgres=# \dt+
>                              List of relations
> +--------+------------+-------------------+-------+---------+-------------+
> | Schema |    Name    |       Type        | Owner |  Size   | Description |
> +--------+------------+-------------------+-------+---------+-------------+
> | public | data       | partitioned table | pavel | 0 bytes |             |
> | public | data_2016  | table             | pavel | 15 MB   |             |
> | public | data_2017  | table             | pavel | 15 MB   |             |
> | public | data_other | table             | pavel | 11 MB   |             |
> +--------+------------+-------------------+-------+---------+-------------+
> (4 rows)

I think this is a clear improvement.  The term "table" was introduced
for this case by f0e44751d7 ("Implement table partitioning.") and now
the author of that commit supports this change.  I used the term "index"
for partitioned indexes originally because I was copying the existing
term, but now I too think they should say "partitioned indexes" instead,
because they are different enough objects from plain indexes.

To be certain I'm not going against some old decision, I digged up
Amit's old patches.  Turns out he submitted psql's describe.c using the
term "partitioned table" on August 10th [1] and then based on a
discussion where Robert suggested calling these new objects "partition
roots" instead to avoid confusion, it was changed to "table" in the next
submission on August 26th [2].  It seems the right call to have used the
term "table" in many places (rather than "partition roots"), but at
least in psql's \dt it seems extremely useful to show the type as
"partitioned table" instead, because it is one place where the
distinction is clearly useful.

In this thread there have been no contrary votes, so I'm pushing this
part soon.

[1] https://postgr.es/m/ad16e2f5-fc7c-cc2d-333a-88d4aa446f96@lab.ntt.co.jp
[2] https://postgr.es/m/169708f6-6e5a-18d1-707b-1b323e4a6baf@lab.ntt.co.jp

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

On 2018/11/19 11:17, Alvaro Herrera wrote:
> On 2018-Jul-23, Pavel Stehule wrote:
> 
>> p.s. Another patch can be replacement of relation type from "table" to
>> "partitioned table"
>>
>> postgres=# \dt+
>>                              List of relations
>> +--------+------------+-------------------+-------+---------+-------------+
>> | Schema |    Name    |       Type        | Owner |  Size   | Description |
>> +--------+------------+-------------------+-------+---------+-------------+
>> | public | data       | partitioned table | pavel | 0 bytes |             |
>> | public | data_2016  | table             | pavel | 15 MB   |             |
>> | public | data_2017  | table             | pavel | 15 MB   |             |
>> | public | data_other | table             | pavel | 11 MB   |             |
>> +--------+------------+-------------------+-------+---------+-------------+
>> (4 rows)
> 
> I think this is a clear improvement.  The term "table" was introduced
> for this case by f0e44751d7 ("Implement table partitioning.") and now
> the author of that commit supports this change.  I used the term "index"
> for partitioned indexes originally because I was copying the existing
> term, but now I too think they should say "partitioned indexes" instead,
> because they are different enough objects from plain indexes.
> 
> To be certain I'm not going against some old decision, I digged up
> Amit's old patches.  Turns out he submitted psql's describe.c using the
> term "partitioned table" on August 10th [1] and then based on a
> discussion where Robert suggested calling these new objects "partition
> roots" instead to avoid confusion, it was changed to "table" in the next
> submission on August 26th [2].  It seems the right call to have used the
> term "table" in many places (rather than "partition roots"), but at
> least in psql's \dt it seems extremely useful to show the type as
> "partitioned table" instead, because it is one place where the
> distinction is clearly useful.
> 
> In this thread there have been no contrary votes, so I'm pushing this
> part soon.
> 
> [1] https://postgr.es/m/ad16e2f5-fc7c-cc2d-333a-88d4aa446f96@lab.ntt.co.jp
> [2] https://postgr.es/m/169708f6-6e5a-18d1-707b-1b323e4a6baf@lab.ntt.co.jp

Yeah, I agree that showing "partitioned table" for partitioned tables in
this case is helpful.

Earlier on this thread [1], I had expressed a slight concern about the
consistency of mentioning "partitioned" in various outputs, because many
error messages say "table" even if the table is partitioned.  But now I
think that it's orthogonal.  We should show "partitioned" where it is helpful.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/5474c8b6-04e7-1afc-97b6-adb7471c2c71%40lab.ntt.co.jp

On Sun, Nov 18, 2018 at 11:17:37PM -0300, Alvaro Herrera wrote:
> To be certain I'm not going against some old decision, I digged up
> Amit's old patches.  Turns out he submitted psql's describe.c using the
> term "partitioned table" on August 10th [1] and then based on a
> discussion where Robert suggested calling these new objects "partition
> roots" instead to avoid confusion, it was changed to "table" in the next
> submission on August 26th [2].  It seems the right call to have used the
> term "table" in many places (rather than "partition roots"), but at
> least in psql's \dt it seems extremely useful to show the type as
> "partitioned table" instead, because it is one place where the
> distinction is clearly useful.

+1.

> In this thread there have been no contrary votes, so I'm pushing this
> part soon.
>
> [1] https://postgr.es/m/ad16e2f5-fc7c-cc2d-333a-88d4aa446f96@lab.ntt.co.jp
> [2] https://postgr.es/m/169708f6-6e5a-18d1-707b-1b323e4a6baf@lab.ntt.co.jp

Sorry for degressing, but could you also update \di at the same time so
as it shows "partitioned index"?  listTables() should be switched to use
partitioned tables and partitioned indexes, and permissionsList() has a
reference to partitioned tables.  While on it, this gives the attached..
--
Michael

On Mon, Nov 19, 2018 at 09:33:58AM +0100, Pavel Stehule wrote:
> po 19. 11. 2018 v 3:42 odesílatel Michael Paquier <michael@paquier.xyz>
> napsal:
>> Sorry for degressing, but could you also update \di at the same time so
>> as it shows "partitioned index"?  listTables() should be switched to use
>> partitioned tables and partitioned indexes, and permissionsList() has a
>> reference to partitioned tables.  While on it, this gives the attached..
>>
>
> It has sense

For the archive's sake, d56e0fde has been committed for this purpose.
--
Michael

On Mon, Nov 05, 2018 at 11:43:16AM +0100, Pavel Stehule wrote:
> should be fixed now.

Here are some notes on the last version.

+   "              FROM pg_inherits i\n"
Missing schema qualification.

+           case 'P':
+               if (cmd[2] == 'i')
+                   success = listPartitions(pattern, show_verbose,
true, false);
+               else if (cmd[2] == 't')
+                   success = listPartitions(pattern, show_verbose,
false, true);
+               else if (cmd[2] == '+' || cmd[2] == '\0')
+                   success = listPartitions(pattern, show_verbose,
false, false);
+               else
+                   status = PSQL_CMD_UNKNOWN;
+               break;
The style is heavy.  Perhaps it would be cleaner to have a
switch/case..  Not a big deal visibly.  show_indexes is true only if the
subcommand is 'i'.  show_tables is true only if the subcommand is 't'.

Using "\dP" with a pattern matching a partitioned index should show a
partitioned index, no?  As far as I know, a partitioned relation can be
either an index or a table.

Testing the feature, \dP shows all partitioned relations, still does not
show the relationship when multiple levels are used.  Could it make
sense to also show the direct parent of a partitioned table when
verbose mode is used?

Could it be possible to have tests for \dP, \dPi and \dPt with matching
patterns?  You could just place that in one of the existing tests where
there are partitioned tables and indexes.
--
Michael

On 2018/11/20 16:50, Michael Paquier wrote:
> Testing the feature, \dP shows all partitioned relations, still does not
> show the relationship when multiple levels are used.  Could it make
> sense to also show the direct parent of a partitioned table when
> verbose mode is used?

Yeah.  I think it would make sense for \dP output to have an additional
column(s) for partitioning-specific information if we're building a
special command for partitioning anyway.

Thanks,
Amit

Hmm, these tests are not going to work, because they have "pavel" in the
expected output.

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

On Wed, Nov 21, 2018 at 05:37:33PM +0100, Pavel Stehule wrote:
> st 21. 11. 2018 v 17:21 odesílatel Alvaro Herrera <alvherre@2ndquadrant.com>
> napsal:
>> Hmm, these tests are not going to work, because they have "pavel" in the
>> expected output.
>
> I was blind, thank you for check

+create table testtable_apple(logdate date);
+create table testtable_orange(logdate date);
+create index testtable_apple_index on testtable_apple(logdate);
+create index testtable_orange_index on testtable_orange(logdate);
There are already a bunch of partition relations with multiple levels
created as part of the regression tests, so instead of creating more of
those, I would suggest to test \dP and \dPt in create_table.sql, and
\dPi in indexing.sql (please make sure to add tests for \dP with
partitioned indexes as well).

I think that you should really add the direct parent of a partition in
at least the verbose output, now for multiple partition levels things
are confusing in my opinion.  For example with such a schema:
CREATE TABLE parent_tab (id int) PARTITION BY RANGE (id);
CREATE INDEX parent_index ON parent_tab (id);
CREATE TABLE child_0_10 PARTITION OF parent_tab
  FOR VALUES FROM (0) TO (10);
CREATE TABLE child_10_20 PARTITION OF parent_tab
  FOR VALUES FROM (10) TO (20);
CREATE TABLE child_20_30 PARTITION OF parent_tab
  FOR VALUES FROM (20) TO (30);
INSERT INTO parent_tab VALUES (generate_series(0,29));
CREATE TABLE child_30_40 PARTITION OF parent_tab
FOR VALUES FROM (30) TO (40)
  PARTITION BY RANGE(id);
CREATE TABLE child_30_35 PARTITION OF child_30_40
  FOR VALUES FROM (30) TO (35);
CREATE TABLE child_35_40 PARTITION OF child_30_40
   FOR VALUES FROM (35) TO (40);
INSERT INTO parent_tab VALUES (generate_series(30,39));

Then with \dP+ I got that:
=# \dP+
            List of partitioned relations
 Schema |    Name     | Owner  |  Size  | Description
--------+-------------+--------+--------+-------------
 public | child_30_40 | ioltas | 48 kB  |
 public | parent_tab  | ioltas | 120 kB |
(2 rows)
Showing the parent partition looks like a pretty important to me as I
would expect multi-level partitions to be a frequent case (perhaps it
should show up as well in the non-verbose output?).  The field should be
NULL if the relation is the top of the tree.

Again, with the previous schema:
=# \dPi *idx
            List of partitioned indexes
 Schema |        Name        | Owner  |    Table
--------+--------------------+--------+-------------
 public | child_30_40_id_idx | ioltas | child_30_40
(1 row)
=# \dP *idx
Did not find any partitioned relations named "*idx"
I would have expected in the second case to have the partitioned
*relations* showing up in the output, and a relation can be an index as
well if the pattern matches.

Could you please address those problems first?  The basic shape of the
patch with the three new sub-commands is fine I think, so we can go
ahead with that, but the two problems reported are blockers in my
opinion.
--
Michael

On Thu, Nov 22, 2018 at 12:42:14PM +0100, Pavel Stehule wrote:
> Here my position is strong. \dP for me doesn't mean "tables or
> indexes" - it means "partition tables with total relation size". I
> don't see any sense to show tables and indexes in one report.

Please let me disagree on that point.  \dP, \dPt and \dPi are commands
able to show information about respectively partitioned relations,
partitioned tables and partitioned indexes, which is not something only
related to the size of those partitions.  Showing only the level of a
relation in its hierarchy may be useful, but that's confusing for the
user without knowing its direct parent or its top-most parent.  For
multiple levels, the direct parent without the number in the hierarchy
seems enough to me.  I may be of course wrong in designing those
concepts.
--
Michael

On Thu, Nov 22, 2018 at 03:47:11PM +0100, Pavel Stehule wrote:
> I have not feel well when I see in one report numbers 40 and 16, I see much
> more comfortable when I see 24 and 16, but for this I need a different
> perspective
>
> What do you think about it?

Maybe, my thought is that this would be a separate patch, and that we
had better keep it simple for now because that's complicated enough :)

So I would rather have a wrapper on top of pg_partition_tree which is
useful for the user with matching patterns, which shows roughly:
- the size of the whole partition tree from the root.
- its direct parent if any.  Potentially in the verbose output.  This
depends on how much users have multi-level partitions.  My bet would be
not that much, but more input from others is welcome.
- perhaps its level in the hierarchy, if integrated most likely as part
of the verbose output.

Then with the set of commands, have a mapping close to how \d treats
indexes and relations:
- \dp shows information about partitioned tables or indexes, if no pattern
is defined tables show up.  If a partitioned index pattern shows up,
then index information is displayed.
- \dpi and \dpt for respectively partitioned indexes and tables.
--
Michael

Hi,

Thank you for updating the patch.

On 2018/12/17 17:48, Pavel Stehule wrote:
> new update of this patch

Documentation portion of this patch still contains some typos that I
mentioned before here:

https://www.postgresql.org/message-id/1c83bb5c-47cd-d796-226c-e95795b05551%40lab.ntt.co.jp

+ .. If the form <literal>\dP+</literal>
+        is used, the sum of size of related partitions (including the
+        table and indexes, if any) and a description
+        are also displayed.

+ ... If the form <literal>\dPi+</literal>
+        is used, the sum of size of related indexes and a description
+        are also displayed.

+ ... If the form <literal>\dPt+</literal>
+        is used, the sum of size of related tables and a description
+        are also displayed.

In all of the three hunks:

the sum of size of -> the sum of "sizes" of

and a description -> and associated description

> changes:
> 
> 1. only root partitioned tables are displayed - you can see quickly total
> allocated space. It is not duplicated due nested partitions.

+1

If one wants to see a non-root partitioned table's details, they can use
\dP+ <pattern>.

> I can imagine new additional flag - line "n" nested - and then we can
> display nested partitioned tables with parent table info. Some like
> 
> \dPt - show only root partition tables
> \dPnt or \dPtn - show root and nested partitioned tables

Too much complication maybe?

> 2. \dP without pattern shows root partitioned tables + total relation size.
> When pattern is defined, then shows tables and indexes + table size
> 
> postgres=# \dP+
>             List of partitioned relations
> ┌────────┬────────────┬───────┬────────┬─────────────┐
> │ Schema │    Name    │ Owner │  Size  │ Description │
> ╞════════╪════════════╪═══════╪════════╪═════════════╡
> │ public │ parent_tab │ pavel │ 120 kB │             │
> └────────┴────────────┴───────┴────────┴─────────────┘
> (1 row)
> 
> postgres=# \dP+ *
>                         List of partitioned relations or indexes
> ┌────────┬──────────────┬───────┬───────────────────┬────────────┬───────┬─────────────┐
> │ Schema │     Name     │ Owner │       Type        │   Table    │ Size  │
> Description │
> ╞════════╪══════════════╪═══════╪═══════════════════╪════════════╪═══════╪═════════════╡
> │ public │ parent_index │ pavel │ partitioned index │ parent_tab │ 80 kB
> │             │
> │ public │ parent_tab   │ pavel │ partitioned table │            │ 40 kB
> │             │
> └────────┴──────────────┴───────┴───────────────────┴────────────┴───────┴─────────────┘
> (2 rows)
> 
> postgres=# \dP+ *index
>                         List of partitioned relations or indexes
> ┌────────┬──────────────┬───────┬───────────────────┬────────────┬───────┬─────────────┐
> │ Schema │     Name     │ Owner │       Type        │   Table    │ Size  │
> Description │
> ╞════════╪══════════════╪═══════╪═══════════════════╪════════════╪═══════╪═════════════╡
> │ public │ parent_index │ pavel │ partitioned index │ parent_tab │ 80 kB
> │             │
> └────────┴──────────────┴───────┴───────────────────┴────────────┴───────┴─────────────┘
> (1 row)

Looking at the patch:

+        if (pattern)
+            /* translator: objects_name is "indexes", "tables" or "relations" */
+            psql_error("Did not find any partitioned %s named \"%s\".\n",
+                       objects_name,
+                       pattern);
+        else
+            /* translator: object_name is "index", "table" or "relation" */
+            psql_error("Did not find any partitioned %s.\n",
+                      object_name);

It seems that objects_name and object_name need to be swapped between the
if and else blocks, and so do /* translator: ... */ comments.

if (pattern)
    /* translator: object_name is "index", "table" or "relation" */
    psql_error(..., object_name);
else
    /* translator: objects_name is "indexes", "tables" or "relations" */
    psql_error(..., objects_name);

That is, it should say, "Did not find any partitioned index/table/relation
named "foo" and "Did not find any partitioned indexes/tables/relations".

Thanks,
Amit

On Wed, Dec 19, 2018 at 07:38:16AM +0100, Pavel Stehule wrote:
> I am sending updated patch.

Moved to next CF.
--
Michael

Hi Pavel,

Thanks for sending the updated patch.

On 2018/12/19 15:38, Pavel Stehule wrote:
> út 18. 12. 2018 v 8:49 odesílatel Amit Langote <
>> On 2018/12/17 17:48, Pavel Stehule wrote:
>>> I can imagine new additional flag - line "n" nested - and then we can
>>> display nested partitioned tables with parent table info. Some like
>>>
>>> \dPt - show only root partition tables
>>> \dPnt or \dPtn - show root and nested partitioned tables
>>
>> Too much complication maybe?
>>
> 
> I wrote it - the setup query is more complex, but not too much. I fixed the
> size calculation, when nested partitions tables are visible - it calculate
> partitions only from level1 group. Then the displayed size is same as total
> size

\dPn seems to work fine, but I don't quite understand why \dPn+ should
show the sizes only for nested partitions of level.  Consider the
following example outputs:

create table p (a int, b char) partition by list (a);
create table p_1 partition of p for values in (1) partition by list (b);
create table p_1_a partition of p_1 for values in ('a');
create table p_1_bc partition of p_1 for values in ('b', 'c') partition by
list (b);
create table p_1_b partition of p_1_bc for values in ('b');
create table p_1_c partition of p_1_bc for values in ('c');
create table p_2 partition of p for values in (2);
insert into p values (1, 'a');
insert into p values (1, 'b');
insert into p values (2);

\dP+
        List of partitioned relations
 Schema │ Name │ Owner │ Size  │ Description
────────┼──────┼───────┼───────┼─────────────
 public │ p    │ amit  │ 24 kB │
(1 row)

-- size of 'p' shown as 8KB, whereas it's actually 24KB as per above
-- size of 'p_1' shown as 8KB, whereas it's actually 16KB
\dPn+
                  List of partitioned relations
 Schema │  Name  │ Owner │ Parent name │    Size    │ Description
────────┼────────┼───────┼─────────────┼────────────┼─────────────
 public │ p      │ amit  │             │ 8192 bytes │
 public │ p_1    │ amit  │ p           │ 8192 bytes │
 public │ p_1_bc │ amit  │ p_1         │ 8192 bytes │
(3 rows)

Also, if the root partitioned table doesn't have a directly attached leaf
partition, it's size is shown as 0 bytes, whereas I think it should
consider the sizes of its other nested partitions.

drop table p_2;

\dPn+
                  List of partitioned relations
 Schema │  Name  │ Owner │ Parent name │    Size    │ Description
────────┼────────┼───────┼─────────────┼────────────┼─────────────
 public │ p      │ amit  │             │ 0 bytes    │
 public │ p_1    │ amit  │ p           │ 8192 bytes │
 public │ p_1_bc │ amit  │ p_1         │ 8192 bytes │
(3 rows)

If I remove the following two statements from the patched code:

+            if (show_nested_partitions)
+                appendPQExpBuffer(&buf, "\n         WHERE d.level = 1");

+            /*
+             * Assign size just for directly assigned tables, when nested
+             * partitions are visible
+             */
+            if (show_nested_partitions)
+                appendPQExpBuffer(&buf, "\n     WHERE ppt.isleaf AND
ppt.level = 1");

I get the following output, which I find more intuitive:

create table p_2 partition of p for values in (2);
insert into p values (2);

\dP+
        List of partitioned relations
 Schema │ Name │ Owner │ Size  │ Description
────────┼──────┼───────┼───────┼─────────────
 public │ p    │ amit  │ 24 kB │
(1 row)


\dPn+
                  List of partitioned relations
 Schema │  Name  │ Owner │ Parent name │    Size    │ Description
────────┼────────┼───────┼─────────────┼────────────┼─────────────
 public │ p      │ amit  │             │ 24 kB      │
 public │ p_1    │ amit  │ p           │ 16 kB      │
 public │ p_1_bc │ amit  │ p_1         │ 8192 bytes │
(3 rows)

drop table p_2;

\dPn+
                  List of partitioned relations
 Schema │  Name  │ Owner │ Parent name │    Size    │ Description
────────┼────────┼───────┼─────────────┼────────────┼─────────────
 public │ p      │ amit  │             │ 16 kB      │
 public │ p_1    │ amit  │ p           │ 16 kB      │
 public │ p_1_bc │ amit  │ p_1         │ 8192 bytes │
(3 rows)

Thoughts?


Meanwhile, some comments on the patch:

+         If modifier <literal>n</literal> is used, then nested partition
+         tables are displayed too.

Maybe, say "non-root partitioned tables" instead of "nested partition
tables".  Same comment also applies for the same text in the paragraphs
for \dPni and \dPnt too.

+/*
+ * listPartitions()
+ *
+ * handler for \dP, \dPt and \dPi

Maybe mention the 'n' modifier here?

+ */
+bool
+listPartitions(const char *pattern, bool verbose, bool show_indexes, bool
show_tables, bool show_nested_partitions)
+{

I'm not sure if psql's source code formatting guidelines are different
from the backend code, but putting all the arguments on the same line
causes the line grow well over 78 characters.  Can you wrap maybe?

+        if (pattern)
+            /* translator: object_name is "index", "table" or "relation" */
+            psql_error("Did not find any partitioned %s.\n",
+                      object_name);
+        else
+            /* translator: objects_name is "indexes", "tables" or
"relations" */
+            psql_error("Did not find any partitioned %s named \"%s\".\n",
+                       objects_name,
+                       pattern);

You're using the variable 'pattern' in the block where it's supposed to be
NULL.  Maybe it should be:

+        if (pattern)
+            /* translator: object_name is "index", "table" or "relation" */
+            psql_error("Did not find any partitioned %s named \"%s\".\n",
+                      object_name, pattern);
+        else
+            /* translator: objects_name is "indexes", "tables" or
"relations" */
+            psql_error("Did not find any partitioned %s.\n",
+                       objects_name);

Thanks,
Amit

Hi,

On 2019/02/07 18:08, Pavel Stehule wrote:
> čt 7. 2. 2019 v 9:51 odesílatel Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
>> \dPn seems to work fine, but I don't quite understand why \dPn+ should
>> show the sizes only for nested partitions of level.  Consider the

(correcting words of my previous email: ... of level 1.)

> Show nested objects in rectangular output is a problem. I prefer a design
> where any times the sum of displayed sizes is same like total size.
> 
> So if I have partitions on level1 of size 16KB, and on level 2 8KB, then I
> would to display 16 and 8, and not 24 and 8. If I remember, this rule is
> modified, when filter is used.

Just to recap, the originally proposed feature is to show the size of a
partitioned table by summing the sizes of *all* of its (actually leaf)
partitions, which \dP[t|i]+ gives us.  As you mentioned, a limitation of
the feature as initially proposed is that it only shows partitioned tables
that are roots of their respective partition trees.  That is, there is no
way to see the sizes of the intermediate partitioned tables using any of
psql's \d commands.  So, you introduced the "n" modifier, whereby
\dP[t|i]n+ now shows *also* the intermediate partitioned tables with their
sizes.  But it only considers the directly attached partitions of each
partitioned table that's shown.  So, only those partitioned tables that
have leaf partitions directly attached them are shown with non-0 size (if
leaf partitions are non-empty) and others with size 0 (root partitioned
tables in most cases where nested partitioned tables are involved).  But I
think that means the "n" modifier is changing the behavior of the base
command (\dP+) which is meant to show the total size of *all* partitions
under a given partitioned table.  Maybe, the "n" modifier should only
result in including the nested/intermediate partitioned tables and nothing
more than that.

I see your point that all these tables are appearing in the display as one
flat list and so the sizes of same leaf partitions may be multiply
counted, but it's not totally a flat representation given that you have
added "Parent name" column.  We could document that the size of a nested
partitioned table shown in the display is also counted in the size of its
parent partitioned table.  That I think may be easier to understand than
that the size of each partitioned table shown in the display only
considers the sizes of leaf partitions that are directly attached to it.

Thoughts? Any more opinions on this?

Thanks,
Amit

On 2019-Feb-07, Pavel Stehule wrote:

> Your example
> 
> postgres=# \dPtn+
>                      List of partitioned tables
> ┌────────┬────────┬───────┬─────────────┬────────────┬─────────────┐
> │ Schema │  Name  │ Owner │ Parent name │    Size    │ Description │
> ╞════════╪════════╪═══════╪═════════════╪════════════╪═════════════╡
> │ public │ p      │ pavel │             │ 8192 bytes │             │
> │ public │ p_1    │ pavel │ p           │ 8192 bytes │             │
> │ public │ p_1_bc │ pavel │ p_1         │ 8192 bytes │             │
> └────────┴────────┴───────┴─────────────┴────────────┴─────────────┘
> (3 rows)
> 
> I hope so the interpretation is clean .. there are three partitioned tables
> (two are subpartitioned tables). Any partitioned table has assigned 8KB of
> data.
> 
> We can introduce new column "size with sub partitions" where these numbers
> can be counted together. But for term "size" I expect valid rule S1+S2+..
> SN = total size.

Right now, a partitioned table has no size of its own; its only storage
is in its children.  But that might change in the future, for example if
we implement global indexes.  Then it will be useful to show these sizes
separately.

I suggest that we should have one column for the aggregated size of its
children, and another column for the "local" size.  So currently the
local size would always be zero for partitioned tables.  The other
column (maybe "Children Size") would be the sum of the sizes of all its
partitions.

So I think this should be:

                      List of partitioned tables
 ┌────────┬────────┬───────┬─────────────┬────────────┬───────────────┬────────────
 │ Schema │  Name  │ Owner │ Parent name │    Size    │ Children Size │ Description 
 ╞════════╪════════╪═══════╪═════════════╪════════════╪═══════════════╡
 │ public │ p      │ pavel │             │            │    8192 bytes │
 │ public │ p_1    │ pavel │ p           │            │    8192 bytes │
 │ public │ p_1_bc │ pavel │ p_1         │ 8192 bytes │               │
 └────────┴────────┴───────┴─────────────┴────────────┴───────────────┴─────────────



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

Hi Pavel,

Thanks for updating the patch.

On 2019/02/08 17:26, Pavel Stehule wrote:
> I renamed originally calculated column "size" to "direct partitions size"
> .. see Alvaro's comment. Then I introduced new column "total partitions
> size" that is calculated like you propose.
> 
> Now the result of dPn+ looks like
> 
>                                      List of partitioned relations
> ┌────────┬────────┬───────┬─────────────┬────────────────────────┬───────────────────────┬─────────────┐
> │ Schema │  Name  │ Owner │ Parent name │ Direct partitions size │ Total
> partitions size │ Description │
> ╞════════╪════════╪═══════╪═════════════╪════════════════════════╪═══════════════════════╪═════════════╡
> │ public │ p      │ pavel │             │ 8192 bytes             │ 24
> kB                 │             │
> │ public │ p_1    │ pavel │ p           │ 8192 bytes             │ 16
> kB                 │             │
> │ public │ p_1_bc │ pavel │ p_1         │ 8192 bytes             │ 8192
> bytes            │             │
> └────────┴────────┴───────┴─────────────┴────────────────────────┴───────────────────────┴─────────────┘
> (3 rows)

OK, so for each listed partitioned table (root and nested), this shows the
total size of the directly attached leaf partitions *and* the total size
of all partitions in its (sub-) tree.

By the way, what I think Alvaro meant by "local size" is not what the
"direct partition size" above shows.  I think "local size" means the size
of the storage assigned to the table itself, not to partitions attached to
it, which are distinct relations.  We don't implement that concept in
Postgres today, but may in the future.  I'm not sure if we'll add a
another column to show "local size" in the future when we do implement
that concept or if Alvaro meant that there should only be "local size"
(not "direct partition size") which will always show 0 for now and "total
partition size" columns.


Anyway, I have a few more suggestions to improve the patch, but instead of
sending the minute-level changes in the email, I've gone ahead and made
those changes myself.  I've attached a delta patch that applies on top of
your v9 patch.  Summary of the changes I made is as follows:

* Documentation rewording here and there (also mentioned the "direct
partitions size" and "total partitions size" division in the \dPn output
per the latest patch)

* Wrapped some lines in code so that they don't look too wide

* Renamed show_nested_partitions to show_nested

* Changed "Partitioned relations" in the output headers to say
"Partitioned tables" where appropriate

* Fixed quiet mode output to use correct word between object_name vs
objects_name

Please merge these changes if you think they are reasonable.

Thanks,
Amit

On Sat, Feb 16, 2019 at 10:52:35PM +0100, Pavel Stehule wrote:
> I like your changes. I merged all - updated patch is attached

I applied and tested your v10 patch.

Find attached some light modifications.

Thanks,
Justin

On 2019/02/22 1:41, Pavel Stehule wrote:
> čt 21. 2. 2019 v 0:56 odesílatel Justin Pryzby <pryzby@telsasoft.com>
> napsal:
> 
>> On Sat, Feb 16, 2019 at 10:52:35PM +0100, Pavel Stehule wrote:
>>> I like your changes. I merged all - updated patch is attached
>>
>> I applied and tested your v10 patch.
>>
>> Find attached some light modifications.
>>
> 
> I have not any objection - I cannot to evaluate. It is question for some
> native speaker.

Not a native speaker either, but I like Justin's changes.  Although I
noticed that he missed changing one sentence to look like other similar
sentences.

What Justin did:

-        indexes) and associated description are also displayed.
+        indexes) is displayed, as is the relation's description.
         </para>

What I think he meant to do:

-        indexes) and associated description are also displayed.
+        indexes) is also displayed, along with the associated description.
         </para>


> I am not sure if we use labels in form "some: detail" somewhere.

I haven't seen column names like that either.  How about:

-                              gettext_noop("Direct partitions size"));
+                              gettext_noop("Leaf partition size"));

-                              gettext_noop("Total partitions size"));
+                              gettext_noop("Total size"));

-                              gettext_noop("Partitions size"));
+                              gettext_noop("Total size"));

I've attached v11 of the patch, which merges most of Justin's changes and
some of my own on top -- documentation and partition size column names.

Thanks,
Amit

On 2019/03/14 2:11, Pavel Stehule wrote:
> st 13. 3. 2019 v 8:02 odesílatel Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
> napsal:
>> Not a native speaker either, but I like Justin's changes.  Although I
>> noticed that he missed changing one sentence to look like other similar
>> sentences.
>>
>> What Justin did:
>>
>> -        indexes) and associated description are also displayed.
>> +        indexes) is displayed, as is the relation's description.
>>          </para>
>>
>> What I think he meant to do:
>>
>> -        indexes) and associated description are also displayed.
>> +        indexes) is also displayed, along with the associated description.
>>          </para>
>>
>>
>>> I am not sure if we use labels in form "some: detail" somewhere.
>>
>> I haven't seen column names like that either.  How about:
>>
>> -                              gettext_noop("Direct partitions size"));
>> +                              gettext_noop("Leaf partition size"));
>>
>> -                              gettext_noop("Total partitions size"));
>> +                              gettext_noop("Total size"));
>>
>> -                              gettext_noop("Partitions size"));
>> +                              gettext_noop("Total size"));
>>
> 
> +1
> 
> Pavel
> 
> 
>> I've attached v11 of the patch, which merges most of Justin's changes and
>> some of my own on top -- documentation and partition size column names.

Maybe, we should set this ready for committer then?

Thanks,
Amit

On 3/14/19 6:19 AM, Amit Langote wrote:
> On 2019/03/14 2:11, Pavel Stehule wrote:
>>
>>> I've attached v11 of the patch, which merges most of Justin's changes and
>>> some of my own on top -- documentation and partition size column names.
> 
> Maybe, we should set this ready for committer then?

There don't appear to be any objections.  Perhaps you should do that?

Regards,
-- 
-David
david@pgmasters.net

On 2019/03/22 2:23, David Steele wrote:
> On 3/14/19 6:19 AM, Amit Langote wrote:
>> On 2019/03/14 2:11, Pavel Stehule wrote:
>>>
>>>> I've attached v11 of the patch, which merges most of Justin's changes and
>>>> some of my own on top -- documentation and partition size column names.
>>
>> Maybe, we should set this ready for committer then?
> 
> There don't appear to be any objections.  Perhaps you should do that?

OK, done.

Thanks,
Amit

On 2019-03-22 01:21, Amit Langote wrote:
> On 2019/03/22 2:23, David Steele wrote:
>> On 3/14/19 6:19 AM, Amit Langote wrote:
>>> On 2019/03/14 2:11, Pavel Stehule wrote:
>>>>
>>>>> I've attached v11 of the patch, which merges most of Justin's changes and
>>>>> some of my own on top -- documentation and partition size column names.
>>>
>>> Maybe, we should set this ready for committer then?
>>
>> There don't appear to be any objections.  Perhaps you should do that?
> 
> OK, done.

What is the purpose of this patch (hint: commit message)?  The email
subject is "show size of partitioned table", which seems reasonable, but
looking briefly at a patch, it adds new psql commands to display
partitioned tables only.  I don't understand the purpose of that.

(Moreover, who is the author of this patch?  Is the commit fest entry
accurate?)

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

Hi,

On 2019/03/28 18:12, Peter Eisentraut wrote:
> On 2019-03-22 01:21, Amit Langote wrote:
>> On 2019/03/22 2:23, David Steele wrote:
>>> On 3/14/19 6:19 AM, Amit Langote wrote:
>>>> On 2019/03/14 2:11, Pavel Stehule wrote:
>>>>>
>>>>>> I've attached v11 of the patch, which merges most of Justin's changes and
>>>>>> some of my own on top -- documentation and partition size column names.
>>>>
>>>> Maybe, we should set this ready for committer then?
>>>
>>> There don't appear to be any objections.  Perhaps you should do that?
>>
>> OK, done.
> 
> What is the purpose of this patch (hint: commit message)?  The email
> subject is "show size of partitioned table", which seems reasonable, but
> looking briefly at a patch, it adds new psql commands to display
> partitioned tables only.  I don't understand the purpose of that.
> 
> (Moreover, who is the author of this patch?  Is the commit fest entry
> accurate?)

It's mainly Pavel's patch, which I've occasionally posted updated versions
of, when it appeared to me that Pavel might be busy with other stuff.

As mentioned somewhere at the top of this thread, the main reason behind
proposing a new command \dP is that some people didn't much like the idea
that \d+ command itself be modified to gather all partitions and add their
sizes to be shown as the size of the partitioned tables.  More features
got added later on to account for partitioned indexes, showing
sub-partitioned tables in addition to root tables that are shown by
default, etc.

Maybe, Pavel can say more...

Thanks,
Amit

On 2019/03/28 18:31, Amit Langote wrote:
> Hi,
> 
> On 2019/03/28 18:12, Peter Eisentraut wrote:
>> On 2019-03-22 01:21, Amit Langote wrote:
>>> On 2019/03/22 2:23, David Steele wrote:
>>>> On 3/14/19 6:19 AM, Amit Langote wrote:
>>>>> On 2019/03/14 2:11, Pavel Stehule wrote:
>>>>>>
>>>>>>> I've attached v11 of the patch, which merges most of Justin's changes and
>>>>>>> some of my own on top -- documentation and partition size column names.
>>>>>
>>>>> Maybe, we should set this ready for committer then?
>>>>
>>>> There don't appear to be any objections.  Perhaps you should do that?
>>>
>>> OK, done.
>>
>> What is the purpose of this patch (hint: commit message)?  The email
>> subject is "show size of partitioned table", which seems reasonable, but
>> looking briefly at a patch, it adds new psql commands to display
>> partitioned tables only.  I don't understand the purpose of that.
>>
>> (Moreover, who is the author of this patch?  Is the commit fest entry
>> accurate?)
> 
> It's mainly Pavel's patch, which I've occasionally posted updated versions
> of, when it appeared to me that Pavel might be busy with other stuff.
> 
> As mentioned somewhere at the top of this thread, the main reason behind
> proposing a new command \dP is that some people didn't much like the idea
> that \d+ command itself be modified to gather all partitions and add their
> sizes to be shown as the size of the partitioned tables.  More features
> got added later on to account for partitioned indexes, showing
> sub-partitioned tables in addition to root tables that are shown by
> default, etc.
> 
> Maybe, Pavel can say more...

Ah, his message got in before mine.

Thanks,
Amit

On 2019-Mar-13, Amit Langote wrote:

> +    if (show_indexes)
> +    {
> +        objects_name = gettext_noop("indexes");
...
> +        if (pattern)
> +            /* translator: objects_name is "index", "table" */
> +            psql_error("Did not find any partitioned %s named \"%s\".\n",
> +                       object_name,
> +                       pattern);
...
> +        /* translator: objects_name is "indexes", "tables" */
> +        appendPQExpBuffer(&title, _("List of partitioned %s"), objects_name);
> +

This stuff doesn't work from a translation standpoint; sentence building
is not allowed.  You have to do stuff like:

    if (tables)
    {
        errmsg = gettext_noop("Could not find any partitioned table");
        tabletitle = gettext_noop("List of partitioned tables");
    }
 ...
    if (pattern)
        /* translator: objects_name is "index", "table" */
        psql_error(gettext(errmsg), object_name, pattern);
      ...
    appendPQExpBuffer(&title, gettext(tabletitle));

and so on.

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

Something is not right:

alvherre=# \dP t
                                     List of partitioned tables or indexes
 Esquema | Nombre |  Dueño   |       Tipo        |                           On table                           
---------+--------+----------+-------------------+--------------------------------------------------------------
 public  | t      | alvherre | partitioned table | Project-Id-Version: psql (PostgreSQL) 10                    +
         |        |          |                   | Report-Msgid-Bugs-To: pgsql-bugs@postgresql.org             +
         |        |          |                   | PO-Revision-Date: 2017-07-10 12:14-0400                     +
         |        |          |                   | Last-Translator: Álvaro Herrera <alvherre@alvh.no-ip.org>   +
         |        |          |                   | Language-Team: PgSQL Español <pgsql-es-ayuda@postgresql.org>+
         |        |          |                   | Language: es                                                +
         |        |          |                   | MIME-Version: 1.0                                           +
         |        |          |                   | Content-Type: text/plain; charset=UTF-8                     +
         |        |          |                   | Content-Transfer-Encoding: 8bit                             +
         |        |          |                   | Plural-Forms: nplurals=2; plural=n != 1;                    +
         |        |          |                   | 
(1 fila)


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

On 2019-Apr-02, Alvaro Herrera wrote:

> Something is not right:

Another thing that was not right is that translated_columns was being
marked static, and modified in the function; so if you called \dP twice
where one called for translation and the second not, the second time
we'd also translating that column.

Attached contains fixes for those three issues.

I'm gonna go over the docs now.

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

Hi Alvaro,

On 2019/04/03 4:42, Alvaro Herrera wrote:
> On 2019-Apr-02, Alvaro Herrera wrote:
> 
>> Something is not right:
> 
> Another thing that was not right is that translated_columns was being
> marked static, and modified in the function; so if you called \dP twice
> where one called for translation and the second not, the second time
> we'd also translating that column.
> 
> Attached contains fixes for those three issues.

Thanks.  I'm getting an error while applying v12:

patching file src/bin/psql/po/es.po
Hunk #1 FAILED at 12.
1 out of 2 hunks FAILED -- saving rejects to file src/bin/psql/po/es.po.rej

Did you accidentally add it to the patch?

Thanks,
Amit

Hi Amit

On 2019-Apr-03, Amit Langote wrote:

> Thanks.  I'm getting an error while applying v12:
> 
> patching file src/bin/psql/po/es.po
> Hunk #1 FAILED at 12.
> 1 out of 2 hunks FAILED -- saving rejects to file src/bin/psql/po/es.po.rej

Uh, I certainly did not intend to send the es.po part of the patch, I
was just testing that the translation worked as intended.  That's what I
get for doing stuff till the last minute ...

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

At Tue, 2 Apr 2019 22:49:25 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in
<20190403014925.GA9976@alvherre.pgsql>
> Hi Amit
> 
> On 2019-Apr-03, Amit Langote wrote:
> 
> > Thanks.  I'm getting an error while applying v12:
> > 
> > patching file src/bin/psql/po/es.po
> > Hunk #1 FAILED at 12.
> > 1 out of 2 hunks FAILED -- saving rejects to file src/bin/psql/po/es.po.rej
> 
> Uh, I certainly did not intend to send the es.po part of the patch, I
> was just testing that the translation worked as intended.  That's what I
> get for doing stuff till the last minute ...

\dPn doesn't show children because the are of 'r' relkind. And
\dPn * doesn't show type for children.

create table pa (a int, b int) partition by range (a);
create table c1 partition of pa for values from (0) to (10);
create table c2 partition of pa for values from (10) to (20);
create index on pa(a);

postgres=# \dPn *
                  List of partitioned tables and indexes
 Schema |   Name   |  Owner   |       Type        | Parent name | On table 
--------+----------+----------+-------------------+-------------+----------
 public | pa       | horiguti | partitioned table |             | 
 public | pa_a_idx | horiguti | partitioned index |             | pa
(2 rows)


With the attached on top of the latest patch, we get the following result.

postgres=# \dPn *
                  List of partitioned tables and indexes
 Schema |   Name   |  Owner   |       Type        | Parent name | On table 
--------+----------+----------+-------------------+-------------+----------
 public | c1       | horiguti | partition table   | pa          | 
 public | c1_a_idx | horiguti | partition index   | pa_a_idx    | c1
 public | c2       | horiguti | partition table   | pa          | 
 public | c2_a_idx | horiguti | partition index   | pa_a_idx    | c2
 public | pa       | horiguti | partitioned table |             | 
 public | pa_a_idx | horiguti | partitioned index |             | pa
(6 rows)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 736cca33d6..5f97bfc4b2 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3954,7 +3954,11 @@ listPartitions(const char *pattern, bool verbose, bool show_indexes,
         }
     }
 
-    appendPQExpBuffer(&buf, "\nWHERE c.relkind IN (%s)", relkind_str);
+    if (show_nested)
+        appendPQExpBuffer(&buf, "\nWHERE (c.relkind IN (%s) OR c3.relkind IN (%s))", relkind_str, relkind_str);
+    else
+        appendPQExpBuffer(&buf, "\nWHERE c.relkind IN (%s)", relkind_str);
+        
     appendPQExpBufferStr(&buf, !show_nested ? " AND NOT c.relispartition\n" : "\n");
 
     if (!pattern)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 5f97bfc4b2..41075f9a67 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3864,9 +3864,13 @@ listPartitions(const char *pattern, bool verbose, bool show_indexes,
                       ",\n  CASE c.relkind"
                       " WHEN " CppAsString2(RELKIND_PARTITIONED_TABLE) " THEN '%s'"
                       " WHEN " CppAsString2(RELKIND_PARTITIONED_INDEX) " THEN '%s'"
+                      " WHEN " CppAsString2(RELKIND_RELATION) " THEN '%s'"
+                      " WHEN " CppAsString2(RELKIND_INDEX) " THEN '%s'"
                       " END as \"%s\"",
                       gettext_noop("partitioned table"),
                       gettext_noop("partitioned index"),
+                      gettext_noop("partition table"),
+                      gettext_noop("partition index"),
                       gettext_noop("Type"));
 
         translate_columns[3] = true;

Horiguchi-san,

Thanks for taking a look.

On 2019/04/03 12:02, Kyotaro HORIGUCHI wrote:
> \dPn doesn't show children because the are of 'r' relkind. And
> \dPn * doesn't show type for children.
> 
> create table pa (a int, b int) partition by range (a);
> create table c1 partition of pa for values from (0) to (10);
> create table c2 partition of pa for values from (10) to (20);
> create index on pa(a);
> 
> postgres=# \dPn *
>                   List of partitioned tables and indexes
>  Schema |   Name   |  Owner   |       Type        | Parent name | On table 
> --------+----------+----------+-------------------+-------------+----------
>  public | pa       | horiguti | partitioned table |             | 
>  public | pa_a_idx | horiguti | partitioned index |             | pa
> (2 rows)
> 
> 
> With the attached on top of the latest patch, we get the following result.
> 
> postgres=# \dPn *
>                   List of partitioned tables and indexes
>  Schema |   Name   |  Owner   |       Type        | Parent name | On table 
> --------+----------+----------+-------------------+-------------+----------
>  public | c1       | horiguti | partition table   | pa          | 
>  public | c1_a_idx | horiguti | partition index   | pa_a_idx    | c1
>  public | c2       | horiguti | partition table   | pa          | 
>  public | c2_a_idx | horiguti | partition index   | pa_a_idx    | c2
>  public | pa       | horiguti | partitioned table |             | 
>  public | pa_a_idx | horiguti | partitioned index |             | pa
> (6 rows)

I think it's intentional that leaf partitions are not shown, because the
patch is meant to allow showing only the "partitioned" members of
partition trees.  Regular \d[t|i][+] shows normal tables ('r', 'i', etc.)
including their size, but it's useless for partitioned tables ('P', 'I',
etc.) as far as showing the size is concerned, so the patch.  Even \dPn is
meant to only show partitions that are themselves partitioned; note the
"P" in the command.

Thanks,
Amit

Hello.

At Wed, 3 Apr 2019 12:55:06 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<ee892049-0fe4-afe6-cbbf-31cf44fa8522@lab.ntt.co.jp>
> On 2019/04/03 12:02, Kyotaro HORIGUCHI wrote:
> > \dPn doesn't show children because the are of 'r' relkind. And
> > \dPn * doesn't show type for children.
...
> I think it's intentional that leaf partitions are not shown, because the
> patch is meant to allow showing only the "partitioned" members of
> partition trees.  Regular \d[t|i][+] shows normal tables ('r', 'i', etc.)
> including their size, but it's useless for partitioned tables ('P', 'I',
> etc.) as far as showing the size is concerned, so the patch.  Even \dPn is
> meant to only show partitions that are themselves partitioned; note the
> "P" in the command.

+        If the modifier <literal>n</literal> (<quote>nested</quote>) is used,
+        then non-root partitioned tables are included, and a column is shown
+        displaying the parent of each partitioned relation.

Ah. I see. "non-root *partitined* tables". I misread the
phrase. Sorry for the noise.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2019-Apr-03, Kyotaro HORIGUCHI wrote:

> Hello.
> 
> At Wed, 3 Apr 2019 12:55:06 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<ee892049-0fe4-afe6-cbbf-31cf44fa8522@lab.ntt.co.jp>
> > On 2019/04/03 12:02, Kyotaro HORIGUCHI wrote:
> > > \dPn doesn't show children because the are of 'r' relkind. And
> > > \dPn * doesn't show type for children.
> ...
> > I think it's intentional that leaf partitions are not shown, because the
> > patch is meant to allow showing only the "partitioned" members of
> > partition trees.  Regular \d[t|i][+] shows normal tables ('r', 'i', etc.)
> > including their size, but it's useless for partitioned tables ('P', 'I',
> > etc.) as far as showing the size is concerned, so the patch.  Even \dPn is
> > meant to only show partitions that are themselves partitioned; note the
> > "P" in the command.
> 
> +        If the modifier <literal>n</literal> (<quote>nested</quote>) is used,
> +        then non-root partitioned tables are included, and a column is shown
> +        displaying the parent of each partitioned relation.
> 
> Ah. I see. "non-root *partitined* tables". I misread the
> phrase. Sorry for the noise.

Well, is this decision an excellent one, from a UI perspective?  I was
surprised by this too and think this should be reconsidered.

I would opt for having \d NOT show partitions at all, myself.  When you
have many partitions (and we're now making the system scale into the
thousands for a single partitioned table), they clutter the output
making it unusable.  So, rather than think as \dP as "the way to show
the aggregate size of a partitioned table or index", I'd think it as
"the way to show detailed info about a partitioned table or index".
Which includes things like displaying its list of partitions and the
size of each.

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

Hi Alvaro,

On 2019/04/03 21:01, Alvaro Herrera wrote:
> On 2019-Apr-03, Kyotaro HORIGUCHI wrote:
> 
>> Hello.
>>
>> At Wed, 3 Apr 2019 12:55:06 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<ee892049-0fe4-afe6-cbbf-31cf44fa8522@lab.ntt.co.jp>
>>> On 2019/04/03 12:02, Kyotaro HORIGUCHI wrote:
>>>> \dPn doesn't show children because the are of 'r' relkind. And
>>>> \dPn * doesn't show type for children.
>> ...
>>> I think it's intentional that leaf partitions are not shown, because the
>>> patch is meant to allow showing only the "partitioned" members of
>>> partition trees.  Regular \d[t|i][+] shows normal tables ('r', 'i', etc.)
>>> including their size, but it's useless for partitioned tables ('P', 'I',
>>> etc.) as far as showing the size is concerned, so the patch.  Even \dPn is
>>> meant to only show partitions that are themselves partitioned; note the
>>> "P" in the command.
>>
>> +        If the modifier <literal>n</literal> (<quote>nested</quote>) is used,
>> +        then non-root partitioned tables are included, and a column is shown
>> +        displaying the parent of each partitioned relation.
>>
>> Ah. I see. "non-root *partitined* tables". I misread the
>> phrase. Sorry for the noise.
> 
> Well, is this decision an excellent one, from a UI perspective?  I was
> surprised by this too and think this should be reconsidered.
> 
> I would opt for having \d NOT show partitions at all, myself.

Hmm, in the previous long discussions on this topic, people were opposed
[1] to an idea like this, but maybe they are no longer.  Note that the
main opposition came to the idea that \d+ would perform simple
pg_relation_size for regular tables and something else for partitioned
tables, such as, aggregation query over pg_partition_tree.

> When you
> have many partitions (and we're now making the system scale into the
> thousands for a single partitioned table), they clutter the output
> making it unusable.  So, rather than think as \dP as "the way to show
> the aggregate size of a partitioned table or index", I'd think it as
> "the way to show detailed info about a partitioned table or index".
> Which includes things like displaying its list of partitions and the
> size of each.

That might be a good idea, but maybe there isn't time left to revise the
proposed patch like that for 12?

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/flat/495cec7e-f8d9-7e13-4807-90dbf4eec4ea@lab.ntt.co.jp

On 2019-Apr-04, Amit Langote wrote:

> > When you
> > have many partitions (and we're now making the system scale into the
> > thousands for a single partitioned table), they clutter the output
> > making it unusable.  So, rather than think as \dP as "the way to show
> > the aggregate size of a partitioned table or index", I'd think it as
> > "the way to show detailed info about a partitioned table or index".
> > Which includes things like displaying its list of partitions and the
> > size of each.
> 
> That might be a good idea, but maybe there isn't time left to revise the
> proposed patch like that for 12?

Well, the problem with designing psql commands incrementallyj is that
people are angry if you try to change what they show in a future
release.  We only have one shot to get it right, pretty much.

Looking at the current proposal, I think I like \dPn+ very much -- it
shows the aggregated size of all partitioned tables.  But I think \dP
(without the +) is almost completely useless.  I'm not really sure about
having to add the "n" flag, but I suspect in practical use it's okay.

Also, I think \dPn+ shows partitioned tables, but \dPtn+ shows exactly
the same, so why do we have the "t" flag at all?  We have \dPin+ which
shows aggregate size, but the only way to list both tables and indexes
is to add a pattern.  I think this design was confusingly inspired by
the "list" vs.  "describe" schizoid dichotomy of \d, without actually
getting any useful functionality in return.

IMO \dP should be "list partitioned relations with their sizes", and
\dPt "list partitioned tables with their sizes", and \dPi "list
partitioned indexes with their sizes".  If no pattern is given, list
them all, otherwise only list those that match the pattern.

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

On 2019-Apr-05, Alvaro Herrera wrote:

> Looking at the current proposal, I think I like \dPn+ very much -- it
> shows the aggregated size of all partitioned tables.  But I think \dP
> (without the +) is almost completely useless.  I'm not really sure about
> having to add the "n" flag, but I suspect in practical use it's okay.
> 
> Also, I think \dPn+ shows partitioned tables, but \dPtn+ shows exactly
> the same, so why do we have the "t" flag at all?  We have \dPin+ which
> shows aggregate size, but the only way to list both tables and indexes
> is to add a pattern.  I think this design was confusingly inspired by
> the "list" vs.  "describe" schizoid dichotomy of \d, without actually
> getting any useful functionality in return.
> 
> IMO \dP should be "list partitioned relations with their sizes", and
> \dPt "list partitioned tables with their sizes", and \dPi "list
> partitioned indexes with their sizes".  If no pattern is given, list
> them all, otherwise only list those that match the pattern.

After thinking more about this, I'm having second thoughts about the +
thing.  I'm now thinking that requiring the + for computing sizes is
actually a good thing, because if we change it to show all sizes
inconditionally, the command becomes unusable for users with lots of
large partitioned tables.  So the submitted patch is okay in that front.

Maybe the only behavior change I'd do to the submitted patch is to have
\dP show both tables and indexes, while \dPt shows only tables and \dPi
shows only indexes.  Maybe have \dPti show both tables and indexes? (
identical to \dP)  That would be consistent with \d itself.  Also,
compare describeFunctions, which allows multiple type specifiers to be
given.

I'm not in love with the way it handles the "n", "t" and "i" specifiers.
I think we should allow them in any order, not just if the "n" is in
cmd[2].

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

So how about the attached version?

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

On Sun, Apr 07, 2019 at 08:15:06AM -0400, Alvaro Herrera wrote:
> So how about the attached version?

+1

I found a few issues.

\dP+ didn't work.  Fix attached.

+static const SchemaQuery Query_for_list_of_partitioned_relations = {
                         
 
+       .catname = "pg_catalog.pg_class c",
                         
 
+       .selcondition = "c.relkind = " CppAsString2(RELKIND_PARTITIONED_TABLE),
                         
 

=> Should it be called Query_for_list_of_partitioned_tables ?  Or should
c.relkind match indices, too ?

On Sat, Apr 06, 2019 at 01:36:23AM -0300, Alvaro Herrera wrote:
> Maybe the only behavior change I'd do to the submitted patch is to have
> \dP show both tables and indexes, while \dPt shows only tables and \dPi
> shows only indexes.  Maybe have \dPti show both tables and indexes? (
> identical to \dP)  That would be consistent with \d itself.

I think there's an issue with showing indices.  You said that \dP should be
same as \dPti, no?  Right now, indices are not shown in \dP, unless a pattern
is given.  I see you add that behavior in the regression tests; is that really
what's intended ?  Also, right now adding a pattern affects how sizes are
computed, I don't see why that's desirable or, if so, how to resolve that
inconsistency, or how to document it.

Justin

On 2019-Apr-07, Pavel Stehule wrote:

> ne 7. 4. 2019 v 17:27 odesílatel Justin Pryzby <pryzby@telsasoft.com>
> napsal:

> > I think there's an issue with showing indices.  You said that \dP should be
> > same as \dPti, no?  Right now, indices are not shown in \dP, unless a
> > pattern is given.  I see you add that behavior in the regression
> > tests; is that really what's intended ?  Also, right now adding a
> > pattern affects how sizes are computed, I don't see why that's
> > desirable or, if so, how to resolve that inconsistency, or how to
> > document it.
> 
> That depends. If there are not pattern, then \dP show only tables, but with
> total relation size (so size of indexes are nested). It is different than
> \dPti, but I think so it is useful - when you don't specify object type,
> then usually you would to see a tables, but with total size.
> 
> I don't see a benefit from \dP == \dPti. When there are a pattern (that can
> choose some index, then, indexes are displayed and \dP == \dPti.

Well, I think Justin has it right --- \dP should be just like \df, which
means to list "everything".  If you add the "t" or the "i", that means
to list only those kinds of things (just like adding one of a, n, p, t,
w does for \df).  You can add both, and then it list both kinds, just
like \dfanptw list the same things that \df does.

That's also what I changed the docs to say, but I failed to update the
code correctly, and didn't verify the expected output closely either.
So I'm due to resubmit this ...

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

Here.

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

On 2019-Apr-07, Pavel Stehule wrote:

> ne 7. 4. 2019 v 19:16 odesílatel Alvaro Herrera <alvherre@2ndquadrant.com>
> napsal:
> 
> > Here.
> 
> + 1

BTW I'm not sure if you noticed, but I removed the error message "no
partitioned relations found" when the result was empty.  This was
mimicking \d behavior (which was justified on historical grounds), and
\drds behavior (which was justified on pattern ordering grounds); but I
see nothing that justifies it for \dP, so let's make it behave like all
the other commands and display an empty table.

And there's an additional change to make.  In the regression database,
this returns an empty table:

regression=# \dPi regress_indexing.pk5_pkey 
     List of partitioned indexes
 Esquema | Nombre | Dueño | On table 
---------+--------+-------+----------
(0 filas)

but the index does exist, and it is a partitioned one.  So it should be
displayed.  In fact, if I add the "n" flag, it shows:

regression=# \dPin regress_indexing.pk5_pkey 
                               List of partitioned indexes
     Esquema      |  Nombre  |  Dueño   |       Parent name        |       On table       
------------------+----------+----------+--------------------------+----------------------
 regress_indexing | pk5_pkey | alvherre | regress_indexing.pk_pkey | regress_indexing.pk5
(1 fila)

I think the fact that \dPi doesn't show it is broken, and this fixes it:

@@ -3946,7 +3946,8 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
     appendPQExpBufferStr(&buf, "''");    /* dummy */
     appendPQExpBufferStr(&buf, ")\n");
 
-    appendPQExpBufferStr(&buf, !showNested ? " AND NOT c.relispartition\n" : "\n");
+    appendPQExpBufferStr(&buf, !showNested && !pattern ?
+                         " AND NOT c.relispartition\n" : "");
 
     if (!pattern)
         appendPQExpBufferStr(&buf, "      AND n.nspname <> 'pg_catalog'\n"

In order for this to display sanely, I added the "Parent name" column if
either the "n" flag is shown or a pattern is given (previously it only
appeared in the former case).

Note that this changes the expected output in the test; now every test
that uses a pattern displays *all* partitioned relations that match the
pattern, not just top-level ones.  I'm about +0.2 convinced that this is
desirable, but my first example above tilts the scales to changing it as
described.

I noticed this while testing after messing with the tab completion as
suggested by Justin: we already had Query_for_list_of_partitioned_tables
(which you could have used), but since \dP works for both indexes and
tables, I instead modified your new
Query_for_list_of_partitioned_relations to list both relation kinds.

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

On 2019-Apr-07, Pavel Stehule wrote:

> ne 7. 4. 2019 v 20:27 odesílatel Alvaro Herrera <alvherre@2ndquadrant.com>
> napsal:

> > In order for this to display sanely, I added the "Parent name" column if
> > either the "n" flag is shown or a pattern is given (previously it only
> > appeared in the former case).
> 
> I am thinking about it and original behave and this new behave should be
> expected (and unexpected too). We can go this way - I have not
> counter-arguments, but yes, it is more consistent with some other commands,
> pattern disables some other constraints.
> 
> It should be documented - Using any pattern in this case forces 'n' flag.

Added to the docs, and pushed.

I couldn't resist tweaking the ORDER BY clause, too.  I think listing
all tables first, followed by all indexes, and sorting by parent in each
category, is much easier to read.  (Maybe this can use additional
tweaking, but it's a minor thing anyway -- for example putting together
all indexes that correspond to some particular table?)

I noticed that \d never seems to use pg_total_relation_size, so toast
size is never shown.  I did likewise here too and used pg_table_size
everywhere.  I'm not 100% sure this is the most convenient thing.  Maybe
we need yet another column, and/or yet another flag ...?

Also, I think the new \dP should gain a new flag (maybe "l") to make it
list leaf tables/indexes too with their local sizes, and remove those
from the standard \d listing.

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

On Sun, Apr 07, 2019 at 03:13:36PM -0400, Alvaro Herrera wrote:
> On 2019-Apr-07, Pavel Stehule wrote:
> 
> > ne 7. 4. 2019 v 20:27 odesílatel Alvaro Herrera <alvherre@2ndquadrant.com>
> > napsal:
> 
> > > In order for this to display sanely, I added the "Parent name" column if
> > > either the "n" flag is shown or a pattern is given (previously it only
> > > appeared in the former case).
> > 
> > I am thinking about it and original behave and this new behave should be
> > expected (and unexpected too). We can go this way - I have not
> > counter-arguments, but yes, it is more consistent with some other commands,
> > pattern disables some other constraints.
> > 
> > It should be documented - Using any pattern in this case forces 'n' flag.
> 
> Added to the docs, and pushed.

Thanks for helping it across the finish line.

I see you changed from relname to oid::regclass, which is good, thanks.

Then, it's unnecessary to join against pg_class again (and I don't know why
it was a left join) ?

Also docs are wrong re: indices.

Justin

PFA patch intended to have been previously attached...

Hmm..maybe it should say

|        RELATION'S partitions is also displayed, along with the RELATION'S
|        description.
                          

Hi,

On 2019/04/08 12:59, Pavel Stehule wrote:
> ne 7. 4. 2019 v 21:13 odesílatel Alvaro Herrera <alvherre@2ndquadrant.com>
> napsal:
>> Added to the docs, and pushed.
>>
> 
> Thank you very much

Thank you Alvaro for pushing this to completion.  Also, thank you Justin
and Pavel for reviewing it till the last minute.

Looks pretty good in my testing so far.

>> I couldn't resist tweaking the ORDER BY clause, too.  I think listing
>> all tables first, followed by all indexes, and sorting by parent in each
>> category, is much easier to read.  (Maybe this can use additional
>> tweaking, but it's a minor thing anyway -- for example putting together
>> all indexes that correspond to some particular table?)
>>
>> I noticed that \d never seems to use pg_total_relation_size, so toast
>> size is never shown.  I did likewise here too and used pg_table_size
>> everywhere.  I'm not 100% sure this is the most convenient thing.  Maybe
>> we need yet another column, and/or yet another flag ...?
>>
> 
> I prefer some flag - both raw size and total size has sense, but another
> column will be less readable

I noticed that the size shown in the output of both \dP+ and \dPt+ does
include toast relation sizes of (leaf) partitions, because
pg_table_size(), which is used by all the queries that
listPartitionedTables sends to the server, does:

    /*
     * heap size, including FSM and VM
     */
    for (forkNum = 0; forkNum <= MAX_FORKNUM; forkNum++)
        size += calculate_relation_size(&(rel->rd_node), rel->rd_backend,
                                        forkNum);

    /*
     * Size of toast relation
     */
    if (OidIsValid(rel->rd_rel->reltoastrelid))
        size += calculate_toast_table_size(rel->rd_rel->reltoastrelid);

>> Also, I think the new \dP should gain a new flag (maybe "l") to make it
>> list leaf tables/indexes too with their local sizes, and remove those
>> from the standard \d listing.
> 
> +1

+1 to the idea.  Perhaps we'd need to post this in a separate thread to
highlight what may be a substantive change to \d's output?

Thanks,
Amit

On 2019-Apr-08, Amit Langote wrote:

> I noticed that the size shown in the output of both \dP+ and \dPt+ does
> include toast relation sizes of (leaf) partitions, because
> pg_table_size(), which is used by all the queries that
> listPartitionedTables sends to the server, does:
> 
>     /*
>      * heap size, including FSM and VM
>      */
>     for (forkNum = 0; forkNum <= MAX_FORKNUM; forkNum++)
>         size += calculate_relation_size(&(rel->rd_node), rel->rd_backend,
>                                         forkNum);
> 
>     /*
>      * Size of toast relation
>      */
>     if (OidIsValid(rel->rd_rel->reltoastrelid))
>         size += calculate_toast_table_size(rel->rd_rel->reltoastrelid);

Sigh.  I must have mixed up whether pg_table_size did include toast size
or not.  Anyway, I think \dP should report the same thing as \d, which I
think it's doing, so we're good there.

> >> Also, I think the new \dP should gain a new flag (maybe "l") to make it
> >> list leaf tables/indexes too with their local sizes, and remove those
> >> from the standard \d listing.
> > 
> > +1
> 
> +1 to the idea.  Perhaps we'd need to post this in a separate thread to
> highlight what may be a substantive change to \d's output?

Aye aye.

Not in a position to work on that right now; I might get to that in a
few days if nobody beats me to it.  (But then, I might not.)

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

On 2019/04/09 2:37, Alvaro Herrera wrote:
> On 2019-Apr-08, Amit Langote wrote:
> 
>> I noticed that the size shown in the output of both \dP+ and \dPt+ does
>> include toast relation sizes of (leaf) partitions, because
>> pg_table_size(), which is used by all the queries that
>> listPartitionedTables sends to the server, does:
>>
>>     /*
>>      * heap size, including FSM and VM
>>      */
>>     for (forkNum = 0; forkNum <= MAX_FORKNUM; forkNum++)
>>         size += calculate_relation_size(&(rel->rd_node), rel->rd_backend,
>>                                         forkNum);
>>
>>     /*
>>      * Size of toast relation
>>      */
>>     if (OidIsValid(rel->rd_rel->reltoastrelid))
>>         size += calculate_toast_table_size(rel->rd_rel->reltoastrelid);
> 
> Sigh.  I must have mixed up whether pg_table_size did include toast size
> or not.  Anyway, I think \dP should report the same thing as \d, which I
> think it's doing, so we're good there.

Yeah.

>>>> Also, I think the new \dP should gain a new flag (maybe "l") to make it
>>>> list leaf tables/indexes too with their local sizes, and remove those
>>>> from the standard \d listing.
>>>
>>> +1
>>
>> +1 to the idea.  Perhaps we'd need to post this in a separate thread to
>> highlight what may be a substantive change to \d's output?
> 
> Aye aye.
> 
> Not in a position to work on that right now; I might get to that in a
> few days if nobody beats me to it.  (But then, I might not.)

Sure.  I guess I also wanted to hear if you were of thinking it as PG 12
material or PG 13.

Thanks,
Amit

A reminder about these.

Also, I suggest renaming "On Table" to "Table", for consistency with \di.

Justin

On 2019-Apr-17, Justin Pryzby wrote:

> A reminder about these.
> 
> Also, I suggest renaming "On Table" to "Table", for consistency with \di.

Thanks!  Pushed all three.

Please feel free to add things like these to wiki.postgresql.org/wiki/Open_Items
to serve as reminders for the project as a whole.

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