Обсуждение: partition tree inspection functions
Hi.
As discussed a little while back [1] and also recently mentioned [2], here
is a patch that adds a set of functions to inspect the details of a
partition tree. There are three functions:
pg_partition_parent(regclass) returns regclass
pg_partition_root_parent(regclass) returns regclass
pg_partition_tree_tables(regclass) returns setof regclass
Here is an example showing how one may want to use them.
create table p (a int, b int) partition by range (a);
create table p0 partition of p for values from (minvalue) to (0) partition
by hash (b);
create table p00 partition of p0 for values with (modulus 2, remainder 0);
create table p01 partition of p0 for values with (modulus 2, remainder 1);
create table p1 partition of p for values from (0) to (maxvalue) partition
by hash (b);
create table p10 partition of p1 for values with (modulus 2, remainder 0);
create table p11 partition of p1 for values with (modulus 2, remainder 1);
insert into p select i, i from generate_series(-5, 5) i;
select pg_partition_parent('p0') as parent;
parent
--------
p
(1 row)
Time: 1.469 ms
select pg_partition_parent('p01') as parent;
parent
--------
p0
(1 row)
Time: 1.330 ms
select pg_partition_root_parent('p01') as root_parent;
root_parent
-------------
p
(1 row)
select p as relname,
pg_partition_parent(p) as parent,
pg_partition_root_parent(p) as root_parent
from pg_partition_tree_tables('p') p;
relname | parent | root_parent
---------+--------+-------------
p | | p
p0 | p | p
p1 | p | p
p00 | p0 | p
p01 | p0 | p
p10 | p1 | p
p11 | p1 | p
(7 rows)
select p as relname,
pg_partition_parent(p) as parent,
pg_partition_root_parent(p) as root_parent,
pg_relation_size(p) as size
from pg_partition_tree_tables('p') p;
relname | parent | root_parent | size
---------+--------+-------------+------
p | | p | 0
p0 | p | p | 0
p1 | p | p | 0
p00 | p0 | p | 8192
p01 | p0 | p | 8192
p10 | p1 | p | 8192
p11 | p1 | p | 8192
(7 rows)
select sum(pg_relation_size(p)) as total_size
from pg_partition_tree_tables('p') p;
total_size
-------------
32768
(1 row)
Feedback is welcome!
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/flat/495cec7e-f8d9-7e13-4807-90dbf4eec4ea%40lab.ntt.co.jp
[2]
https://www.postgresql.org/message-id/18e000e8-9bcc-1bb5-2f50-56d434c8be1f%40lab.ntt.co.jp
Вложения
On 2018/06/26 14:08, Amit Langote wrote:
> Hi.
>
> As discussed a little while back [1] and also recently mentioned [2], here
> is a patch that adds a set of functions to inspect the details of a
> partition tree. There are three functions:
>
> pg_partition_parent(regclass) returns regclass
> pg_partition_root_parent(regclass) returns regclass
> pg_partition_tree_tables(regclass) returns setof regclass
>
> Here is an example showing how one may want to use them.
>
> create table p (a int, b int) partition by range (a);
> create table p0 partition of p for values from (minvalue) to (0) partition
> by hash (b);
> create table p00 partition of p0 for values with (modulus 2, remainder 0);
> create table p01 partition of p0 for values with (modulus 2, remainder 1);
> create table p1 partition of p for values from (0) to (maxvalue) partition
> by hash (b);
> create table p10 partition of p1 for values with (modulus 2, remainder 0);
> create table p11 partition of p1 for values with (modulus 2, remainder 1);
> insert into p select i, i from generate_series(-5, 5) i;
>
> select pg_partition_parent('p0') as parent;
> parent
> --------
> p
> (1 row)
>
> Time: 1.469 ms
> select pg_partition_parent('p01') as parent;
> parent
> --------
> p0
> (1 row)
>
> Time: 1.330 ms
> select pg_partition_root_parent('p01') as root_parent;
> root_parent
> -------------
> p
> (1 row)
>
> select p as relname,
> pg_partition_parent(p) as parent,
> pg_partition_root_parent(p) as root_parent
> from pg_partition_tree_tables('p') p;
> relname | parent | root_parent
> ---------+--------+-------------
> p | | p
> p0 | p | p
> p1 | p | p
> p00 | p0 | p
> p01 | p0 | p
> p10 | p1 | p
> p11 | p1 | p
> (7 rows)
>
> select p as relname,
> pg_partition_parent(p) as parent,
> pg_partition_root_parent(p) as root_parent,
> pg_relation_size(p) as size
> from pg_partition_tree_tables('p') p;
> relname | parent | root_parent | size
> ---------+--------+-------------+------
> p | | p | 0
> p0 | p | p | 0
> p1 | p | p | 0
> p00 | p0 | p | 8192
> p01 | p0 | p | 8192
> p10 | p1 | p | 8192
> p11 | p1 | p | 8192
> (7 rows)
>
>
> select sum(pg_relation_size(p)) as total_size
> from pg_partition_tree_tables('p') p;
> total_size
> -------------
> 32768
> (1 row)
>
> Feedback is welcome!
Added this to July CF.
Thanks,
Amit
Hi Amit,
On Tue, Jun 26, 2018 at 1:07 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2018/06/26 14:08, Amit Langote wrote:
> Hi.
>
> As discussed a little while back [1] and also recently mentioned [2], here
> is a patch that adds a set of functions to inspect the details of a
> partition tree. There are three functions:
>
> pg_partition_parent(regclass) returns regclass
> pg_partition_root_parent(regclass) returns regclass
> pg_partition_tree_tables(regclass) returns setof regclass
>
I quickly tried applying your patch. Created couple of tables, subpartitions with
mix of range and list partitions, and I see these 3 functions are working as
documented.
Also, the patch does not have any 'make check' failures.
I will do the further code review and post if any comments.
Regards,
Jeevan Ladhe
On 2018/06/26 16:54, Jeevan Ladhe wrote: > Hi Amit, > > On Tue, Jun 26, 2018 at 1:07 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp >> wrote: > >> On 2018/06/26 14:08, Amit Langote wrote: >>> Hi. >>> >>> As discussed a little while back [1] and also recently mentioned [2], >> here >>> is a patch that adds a set of functions to inspect the details of a >>> partition tree. There are three functions: >>> >>> pg_partition_parent(regclass) returns regclass >>> pg_partition_root_parent(regclass) returns regclass >>> pg_partition_tree_tables(regclass) returns setof regclass >>> >> > > I quickly tried applying your patch. Created couple of tables, > subpartitions with > mix of range and list partitions, and I see these 3 functions are working as > documented. > > Also, the patch does not have any 'make check' failures. > > I will do the further code review and post if any comments. Thanks Jeevan for reviewing. Thanks, Amit
On Tue, Jun 26, 2018 at 04:57:53PM +0900, Amit Langote wrote: > Thanks Jeevan for reviewing. I would like to make things more user-friendly in this area, but could you add a couple of tests to show up how things work? I just had a very quick glance at what's proposed at the top of the thread. -- Michael
Вложения
Thanks for taking a look. On 2018/06/27 21:16, Michael Paquier wrote: > I would like to make things more user-friendly in this area, but could > you add a couple of tests to show up how things work? I just had a very > quick glance at what's proposed at the top of the thread. I thought about adding tests, but wasn't sure where such tests would go. For now, I've added them to create_table.sql, but maybe that's not where they really belong. Attached updated patch with tests. Thanks, Amit
Вложения
On Thu, Jun 28, 2018 at 11:50:13AM +0900, Amit Langote wrote: > For now, I've added them to create_table.sql, but maybe that's not where > they really belong. Attached updated patch with tests. It would have imagined that just creating a new file, say partition_desc.sql or similar is nicer. + ancestors = get_partition_ancestors(relid); + result = llast_oid(ancestors); + list_free(ancestors); Relying on the fact that the top-most parent should be the last one in the list is brittle in my opinion. What this patch proposes is: - pg_partition_root_parent to get the top-most parent within a partition tree for a partition. - pg_partition_parent to get the direct parent for a partition. - pg_partition_tree_tables to get a full list of all the children underneath. As the goal is to facilitate the life of users so as they don't have to craft any WITH RECURSIVE, I think that we could live with that. + <para> + If the table passed to <function>pg_partition_root_parent</function> is not + a partition, the same table is returned as the result. Result of + <function>pg_partition_tree_tables</function> also contains the table + that's passed to it as the first row. + </para> Okay for that part as well. I haven't yet looked at the code in details, but what you are proposing here looks sound. Could you think about adding an example in the docs about how to use them? Say for a measurement table here is a query to get the full size a partition tree takes.. That's one idea. -- Michael
Вложения
Thanks again for quick review. On 2018/06/28 12:43, Michael Paquier wrote: > On Thu, Jun 28, 2018 at 11:50:13AM +0900, Amit Langote wrote: >> For now, I've added them to create_table.sql, but maybe that's not where >> they really belong. Attached updated patch with tests. > > It would have imagined that just creating a new file, say > partition_desc.sql or similar is nicer. How about partition_info.sql because they're testing partitioning information functions? partition_desc reminded me of PartitionDesc, an internal structure used in the partitioning codem which made me a bit uncomfortable. > + ancestors = get_partition_ancestors(relid); > + result = llast_oid(ancestors); > + list_free(ancestors); > > Relying on the fact that the top-most parent should be the last one in > the list is brittle in my opinion. get_partition_ancestor stops adding OIDs to the list once it reaches a table in the ancestor chain that doesn't itself have parent (the root), so the last OID in the returned list *must* be the root parent. Do you think adding a check that the OID in result is indeed NOT a partition would make it look less brittle? I added an Assert below that llast_oid statement. > What this patch proposes is: > - pg_partition_root_parent to get the top-most parent within a partition > tree for a partition. > - pg_partition_parent to get the direct parent for a partition. > - pg_partition_tree_tables to get a full list of all the children > underneath. > > As the goal is to facilitate the life of users so as they don't have to > craft any WITH RECURSIVE, I think that we could live with that. > > + <para> > + If the table passed to <function>pg_partition_root_parent</function> is not > + a partition, the same table is returned as the result. Result of > + <function>pg_partition_tree_tables</function> also contains the table > + that's passed to it as the first row. > + </para> > Okay for that part as well. > > I haven't yet looked at the code in details, but what you are proposing > here looks sound. Could you think about adding an example in the docs > about how to use them? Say for a measurement table here is a query to > get the full size a partition tree takes.. That's one idea. OK, I've added an example below the table of functions added by the patch. Attached updated patch. Thanks, Amit
Вложения
On 6/26/18 07:08, Amit Langote wrote: > As discussed a little while back [1] and also recently mentioned [2], here > is a patch that adds a set of functions to inspect the details of a > partition tree. There are three functions: > > pg_partition_parent(regclass) returns regclass > pg_partition_root_parent(regclass) returns regclass > pg_partition_tree_tables(regclass) returns setof regclass Does this add anything over writing a recursive query on pg_inherits? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/06/28 17:40, Peter Eisentraut wrote: > On 6/26/18 07:08, Amit Langote wrote: >> As discussed a little while back [1] and also recently mentioned [2], here >> is a patch that adds a set of functions to inspect the details of a >> partition tree. There are three functions: >> >> pg_partition_parent(regclass) returns regclass >> pg_partition_root_parent(regclass) returns regclass >> pg_partition_tree_tables(regclass) returns setof regclass > > Does this add anything over writing a recursive query on pg_inherits? As far as the information output is concerned, it doesn't. Thanks, Amit
On 6/28/18 10:59, Amit Langote wrote: > On 2018/06/28 17:40, Peter Eisentraut wrote: >> On 6/26/18 07:08, Amit Langote wrote: >>> As discussed a little while back [1] and also recently mentioned [2], here >>> is a patch that adds a set of functions to inspect the details of a >>> partition tree. There are three functions: >>> >>> pg_partition_parent(regclass) returns regclass >>> pg_partition_root_parent(regclass) returns regclass >>> pg_partition_tree_tables(regclass) returns setof regclass >> >> Does this add anything over writing a recursive query on pg_inherits? > > As far as the information output is concerned, it doesn't. I'm thinking, an SQL query might be more efficient if you want to qualify the query further. For example, give me all tables in this tree that match '2018'. If you wrote your functions as SQL-language functions, the optimizer could perhaps inline them and optimize them further. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jun 28, 2018 at 12:37:14PM +0200, Peter Eisentraut wrote:
> I'm thinking, an SQL query might be more efficient if you want to
> qualify the query further. For example, give me all tables in this tree
> that match '2018'. If you wrote your functions as SQL-language
> functions, the optimizer could perhaps inline them and optimize them
> further.
Are you thinking about SQL functions here? Here is an example of query
able to fetch an entire partition tree.
WITH RECURSIVE partition_info
(relid,
relname,
relsize,
relispartition,
relkind) AS (
SELECT oid AS relid,
relname,
pg_relation_size(oid) AS relsize,
relispartition,
relkind
FROM pg_catalog.pg_class
WHERE relname = 'your_parent_table_name_here' AND
relkind = 'p'
UNION ALL
SELECT
c.oid AS relid,
c.relname AS relname,
pg_relation_size(c.oid) AS relsize,
c.relispartition AS relispartition,
c.relkind AS relkind
FROM partition_info AS p,
pg_catalog.pg_inherits AS i,
pg_catalog.pg_class AS c
WHERE p.relid = i.inhparent AND
c.oid = i.inhrelid AND
c.relispartition
)
SELECT * FROM partition_info;
Getting the direct parent is immediate, and getting the top-most parent
would be rather similar to that. Not much elegant in my opinion, but
that's mainly a matter of taste?
--
Michael
Вложения
On Thu, Jun 28, 2018 at 11:19 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> >> It would have imagined that just creating a new file, say >> partition_desc.sql or similar is nicer. > > How about partition_info.sql because they're testing partitioning > information functions? partition_desc reminded me of PartitionDesc, an > internal structure used in the partitioning codem which made me a bit > uncomfortable. I think we should just add calls to these functions/views wherever we are creating/altering or deleting objects to test a partition tree. I serves two purposes, testing the objects created/modified and testing the functions. Adding a new test file means we have to craft new objects, which are sometimes readily available in some other test files. Of course, we might find that certain cases are not covered by existing tests, but then that also means that those cases are not covered by object modification/creation tests as well. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On 6/28/18 13:30, Michael Paquier wrote: > On Thu, Jun 28, 2018 at 12:37:14PM +0200, Peter Eisentraut wrote: >> I'm thinking, an SQL query might be more efficient if you want to >> qualify the query further. For example, give me all tables in this tree >> that match '2018'. If you wrote your functions as SQL-language >> functions, the optimizer could perhaps inline them and optimize them >> further. > > Are you thinking about SQL functions here? Here is an example of query > able to fetch an entire partition tree. > WITH RECURSIVE partition_info > (relid, > relname, > relsize, > relispartition, > relkind) AS ( > SELECT oid AS relid, > relname, > pg_relation_size(oid) AS relsize, > relispartition, > relkind > FROM pg_catalog.pg_class > WHERE relname = 'your_parent_table_name_here' AND > relkind = 'p' [...] Yes, this kind of thing should be more efficient than building the entire tree in a C function and then filtering it afterwards. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/06/28 22:01, Ashutosh Bapat wrote: > On Thu, Jun 28, 2018 at 11:19 AM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >>> >>> It would have imagined that just creating a new file, say >>> partition_desc.sql or similar is nicer. >> >> How about partition_info.sql because they're testing partitioning >> information functions? partition_desc reminded me of PartitionDesc, an >> internal structure used in the partitioning codem which made me a bit >> uncomfortable. > > I think we should just add calls to these functions/views wherever we > are creating/altering or deleting objects to test a partition tree. I > serves two purposes, testing the objects created/modified and testing > the functions. Adding a new test file means we have to craft new > objects, which are sometimes readily available in some other test > files. Of course, we might find that certain cases are not covered by > existing tests, but then that also means that those cases are not > covered by object modification/creation tests as well. I think that makes sense. I couldn't assess by looking at tests for various functions listed on 9.25. System Information Functions whether there is some established practice about adding tests for them and/or about where to put them. For this particular set of functions, insert.sql may be a good place as it has many tests involving multi-level partitioned tables. Thanks, Amit
On 2018/06/29 6:19, Peter Eisentraut wrote:
> On 6/28/18 13:30, Michael Paquier wrote:
>> On Thu, Jun 28, 2018 at 12:37:14PM +0200, Peter Eisentraut wrote:
>>> I'm thinking, an SQL query might be more efficient if you want to
>>> qualify the query further. For example, give me all tables in this tree
>>> that match '2018'. If you wrote your functions as SQL-language
>>> functions, the optimizer could perhaps inline them and optimize them
>>> further.
>>
>> Are you thinking about SQL functions here? Here is an example of query
>> able to fetch an entire partition tree.
>> WITH RECURSIVE partition_info
>> (relid,
>> relname,
>> relsize,
>> relispartition,
>> relkind) AS (
>> SELECT oid AS relid,
>> relname,
>> pg_relation_size(oid) AS relsize,
>> relispartition,
>> relkind
>> FROM pg_catalog.pg_class
>> WHERE relname = 'your_parent_table_name_here' AND
>> relkind = 'p'
> [...]
>
> Yes, this kind of thing should be more efficient than building the
> entire tree in a C function and then filtering it afterwards.
Hmm, it would be nice if the user-specified filters could get pushed down
under the CTE scan that will get generated for recursive union, but it
doesn't afaics. If there's no way to write the query such that they do
get pushed down, then using a C function to build the tree sounds better
than using a query.
For example, I compared using the quoted query (thanks Michael) and the
proposed pg_partition_tree_tables function on a partition tree with 1000
partitions and don't see much difference.
WITH RECURSIVE partition_info
(relid,
relname,
relsize,
relispartition,
relkind) AS (
SELECT oid AS relid,
relname,
pg_relation_size(oid) AS relsize,
relispartition,
relkind
FROM pg_catalog.pg_class
WHERE relname = 'ht' AND
relkind = 'p'
UNION ALL
SELECT
c.oid AS relid,
c.relname AS relname,
pg_relation_size(c.oid) AS relsize,
c.relispartition AS relispartition,
c.relkind AS relkind
FROM partition_info AS p,
pg_catalog.pg_inherits AS i,
pg_catalog.pg_class AS c
WHERE p.relid = i.inhparent AND
c.oid = i.inhrelid AND
c.relispartition
)
SELECT * FROM partition_info WHERE relname LIKE '%01%';
relid │ relname │ relsize │ relispartition │ relkind
───────┼─────────┼─────────┼────────────────┼─────────
18616 │ ht_101 │ 0 │ t │ r
18916 │ ht_201 │ 0 │ t │ r
19216 │ ht_301 │ 0 │ t │ r
19516 │ ht_401 │ 0 │ t │ r
19816 │ ht_501 │ 0 │ t │ r
20116 │ ht_601 │ 0 │ t │ r
20416 │ ht_701 │ 0 │ t │ r
20716 │ ht_801 │ 0 │ t │ r
21016 │ ht_901 │ 0 │ t │ r
(9 rows)
Time: 47.562 ms
select p::oid as relid, p as relname, pg_relation_size(p) as relsize,
c.relispartition, c.relkind
from pg_partition_tree_tables('ht') p, pg_class c
where p::oid = c.oid and p::text like '%01%';
relid │ relname │ relsize │ relispartition │ relkind
───────┼─────────┼─────────┼────────────────┼─────────
18616 │ ht_101 │ 0 │ t │ r
18916 │ ht_201 │ 0 │ t │ r
19216 │ ht_301 │ 0 │ t │ r
19516 │ ht_401 │ 0 │ t │ r
19816 │ ht_501 │ 0 │ t │ r
20116 │ ht_601 │ 0 │ t │ r
20416 │ ht_701 │ 0 │ t │ r
20716 │ ht_801 │ 0 │ t │ r
21016 │ ht_901 │ 0 │ t │ r
(9 rows)
Time: 34.357 ms
Am I missing something?
Now, if the users write the query themselves and add whatever filters they
want to use, then that might be the fastest.
WITH RECURSIVE partition_info
(relid,
relname,
relsize,
relispartition,
relkind) AS (
SELECT oid AS relid,
relname,
pg_relation_size(oid) AS relsize,
relispartition,
relkind
FROM pg_catalog.pg_class
WHERE relname = 'ht' AND
relkind = 'p'
UNION ALL
SELECT
c.oid AS relid,
c.relname AS relname,
pg_relation_size(c.oid) AS relsize,
c.relispartition AS relispartition,
c.relkind AS relkind
FROM partition_info AS p,
pg_catalog.pg_inherits AS i,
pg_catalog.pg_class AS c
WHERE p.relid = i.inhparent AND
c.oid = i.inhrelid AND
c.relispartition AND c.relname LIKE '%01%'
)
SELECT * FROM partition_info p WHERE p.relname LIKE '%01%';
relid │ relname │ relsize │ relispartition │ relkind
───────┼─────────┼─────────┼────────────────┼─────────
18616 │ ht_101 │ 0 │ t │ r
18916 │ ht_201 │ 0 │ t │ r
19216 │ ht_301 │ 0 │ t │ r
19516 │ ht_401 │ 0 │ t │ r
19816 │ ht_501 │ 0 │ t │ r
20116 │ ht_601 │ 0 │ t │ r
20416 │ ht_701 │ 0 │ t │ r
20716 │ ht_801 │ 0 │ t │ r
21016 │ ht_901 │ 0 │ t │ r
(9 rows)
Time: 27.276 ms
Thanks,
Amit
On Tue, Jun 26, 2018 at 10:38 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Hi.
>
> As discussed a little while back [1] and also recently mentioned [2], here
> is a patch that adds a set of functions to inspect the details of a
> partition tree. There are three functions:
>
> pg_partition_parent(regclass) returns regclass
> pg_partition_root_parent(regclass) returns regclass
> pg_partition_tree_tables(regclass) returns setof regclass
>
>
> select p as relname,
> pg_partition_parent(p) as parent,
> pg_partition_root_parent(p) as root_parent
> from pg_partition_tree_tables('p') p;
> relname | parent | root_parent
> ---------+--------+-------------
> p | | p
> p0 | p | p
> p1 | p | p
> p00 | p0 | p
> p01 | p0 | p
> p10 | p1 | p
> p11 | p1 | p
> (7 rows)
>
Is it a good idea to provide a function or an option which can provide
partitions detail in hierarchical order?
i.e
relname level
p 0
p0 1
p00 2
p01 2
p1 1
....
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Hi Amit,
On 06/28/2018 01:49 AM, Amit Langote wrote:
> OK, I've added an example below the table of functions added by the patch.
>
> Attached updated patch.
>
You forgot to remove the test output in create_table.out, so check-world
is failing.
In pg_partition_parent
+ else
+ /* Not a partition, return NULL. */
+ PG_RETURN_NULL();
I would just remove the "else" such that PG_RETURN_NULL() is fall-through.
I think pg_partition_tree_tables should have an option to exclude the
table that is being queried from the result (bool include_self).
Maybe a function like pg_partition_number_of_partitions() could be of
benefit to count the number of actual partitions in a tree. Especially
useful in complex scenarios,
select pg_partition_number_of_partitions('p') as number;
number
---------
4
(1 row)
New status: WfA
Best regards,
Jesper
Hi Dilip,
Sorry it took me a while to reply.
On 2018/06/29 14:30, Dilip Kumar wrote:
> On Tue, Jun 26, 2018 at 10:38 AM, Amit Langote wrote:
>> As discussed a little while back [1] and also recently mentioned [2], here
>> is a patch that adds a set of functions to inspect the details of a
>> partition tree. There are three functions:
>>
>> pg_partition_parent(regclass) returns regclass
>> pg_partition_root_parent(regclass) returns regclass
>> pg_partition_tree_tables(regclass) returns setof regclass
>>
>>
>> select p as relname,
>> pg_partition_parent(p) as parent,
>> pg_partition_root_parent(p) as root_parent
>> from pg_partition_tree_tables('p') p;
>> relname | parent | root_parent
>> ---------+--------+-------------
>> p | | p
>> p0 | p | p
>> p1 | p | p
>> p00 | p0 | p
>> p01 | p0 | p
>> p10 | p1 | p
>> p11 | p1 | p
>> (7 rows)
>>
>
> Is it a good idea to provide a function or an option which can provide
> partitions detail in hierarchical order?
>
> i.e
> relname level
> p 0
> p0 1
> p00 2
> p01 2
> p1 1
Yeah, might be a good idea. We could have a function
pg_partition_tree_level(OID) which will return the level of the table
that's passed to it the way you wrote above, meaning 0 for the root
parent, 1 for the root's immediate partitions, 2 for their partitions, and
so on.
Thanks,
Amit
Thanks for the review, Jesper.
On 2018/07/18 23:35, Jesper Pedersen wrote:
> On 06/28/2018 01:49 AM, Amit Langote wrote:
>> OK, I've added an example below the table of functions added by the patch.
>>
>> Attached updated patch.
>>
>
> You forgot to remove the test output in create_table.out, so check-world
> is failing.
Oops, I'd noticed that but forgotten to post an updated patch.
Fixed.
> In pg_partition_parent
>
> + else
> + /* Not a partition, return NULL. */
> + PG_RETURN_NULL();
>
> I would just remove the "else" such that PG_RETURN_NULL() is fall-through.
OK, done.
> I think pg_partition_tree_tables should have an option to exclude the
> table that is being queried from the result (bool include_self).
Doesn't sound too bad, so added include_self.
> Maybe a function like pg_partition_number_of_partitions() could be of
> benefit to count the number of actual partitions in a tree. Especially
> useful in complex scenarios,
>
> select pg_partition_number_of_partitions('p') as number;
>
> number
> ---------
> 4
> (1 row)
Okay, adding one more function at this point may not be asking for too
much. Although, select count(*) from pg_partition_tree_tables('p') would
give you the count, a special function seems nice.
> New status: WfA
Attached updated patch.
Thanks,
Amit
Вложения
Hi Amit,
On 07/19/2018 04:39 AM, Amit Langote wrote:
>> I think pg_partition_tree_tables should have an option to exclude the
>> table that is being queried from the result (bool include_self).
>
> Doesn't sound too bad, so added include_self.
>
I'm thinking about how to best use these functions to generate a graph
that represents the partition hierarchy.
What about renaming pg_partition_tree_tables() to
pg_partition_children(), and have it work like
select * from pg_partition_children('p', true);
---------
p
p0
p1
p00
p01
p10
p11
(7 rows)
select * from pg_partition_children('p', false);
---------
p0
p1
(2 rows)
e.g. if 'bool include_all' is true all nodes under the node, including
itself, are fetched. With false only nodes directly under the node,
excluding itself, are returned. If there are no children NULL is returned.
>> Maybe a function like pg_partition_number_of_partitions() could be of
>> benefit to count the number of actual partitions in a tree. Especially
>> useful in complex scenarios,
>>
>> select pg_partition_number_of_partitions('p') as number;
>>
>> number
>> ---------
>> 4
>> (1 row)
>
> Okay, adding one more function at this point may not be asking for too
> much. Although, select count(*) from pg_partition_tree_tables('p') would
> give you the count, a special function seems nice.
>
Yeah, but I was thinking that the function would only return the number
of actual tables that contains data, e.g. not include 'p', 'p0' and 'p1'
in the count; otherwise you could use 'select count(*) from
pg_partition_children('p', true)' like you said.
Thanks for considering.
Best regards,
Jesper
Hi Jesper,
On 2018/07/19 23:18, Jesper Pedersen wrote:
> I'm thinking about how to best use these functions to generate a graph
> that represents the partition hierarchy.
>
> What about renaming pg_partition_tree_tables() to pg_partition_children(),
> and have it work like
>
> select * from pg_partition_children('p', true);
> ---------
> p
> p0
> p1
> p00
> p01
> p10
> p11
> (7 rows)
>
> select * from pg_partition_children('p', false);
> ---------
> p0
> p1
> (2 rows)
>
> e.g. if 'bool include_all' is true all nodes under the node, including
> itself, are fetched. With false only nodes directly under the node,
> excluding itself, are returned. If there are no children NULL is returned.
That's a big change to make to what this function does, but if that's
what's useful we could make it. As an alternative, wouldn't it help to
implement the idea that Dilip mentioned upthread of providing a function
to report the level of a given table in the partition hierarchy -- 0 for
root, 1 for its partitions and so on?
Basically, as also discussed before, users can already use SQL to get the
information they want out of the relevant catalogs (pg_inherits, etc.).
But, such user queries might not be very future-proof as we might want to
change the catalog organization in the future, so we'd like to provide
users a proper interface to begin with. Keeping that in mind, it'd be
better to think carefully about what we ought to be doing here. Input
like yours is greatly helpful for that.
>>> Maybe a function like pg_partition_number_of_partitions() could be of
>>> benefit to count the number of actual partitions in a tree. Especially
>>> useful in complex scenarios,
>>>
>>> select pg_partition_number_of_partitions('p') as number;
>>>
>>> number
>>> ---------
>>> 4
>>> (1 row)
>>
>> Okay, adding one more function at this point may not be asking for too
>> much. Although, select count(*) from pg_partition_tree_tables('p') would
>> give you the count, a special function seems nice.
>
> Yeah, but I was thinking that the function would only return the number of
> actual tables that contains data, e.g. not include 'p', 'p0' and 'p1' in
> the count; otherwise you could use 'select count(*) from
> pg_partition_children('p', true)' like you said.
Maybe call it pg_partition_tree_leaf_count() or some such then?
Thanks,
Amit
Hi Amit,
On 07/19/2018 10:27 PM, Amit Langote wrote:
> On 2018/07/19 23:18, Jesper Pedersen wrote:
>> I'm thinking about how to best use these functions to generate a graph
>> that represents the partition hierarchy.
>>
>> What about renaming pg_partition_tree_tables() to pg_partition_children(),
>> and have it work like
>>
>> select * from pg_partition_children('p', true);
>> ---------
>> p
>> p0
>> p1
>> p00
>> p01
>> p10
>> p11
>> (7 rows)
>>
>> select * from pg_partition_children('p', false);
>> ---------
>> p0
>> p1
>> (2 rows)
>>
>> e.g. if 'bool include_all' is true all nodes under the node, including
>> itself, are fetched. With false only nodes directly under the node,
>> excluding itself, are returned. If there are no children NULL is returned.
>
> That's a big change to make to what this function does, but if that's
> what's useful we could make it. As an alternative, wouldn't it help to
> implement the idea that Dilip mentioned upthread of providing a function
> to report the level of a given table in the partition hierarchy -- 0 for
> root, 1 for its partitions and so on?
>
Yes, Dilip's idea could work. I just don't think that
pg_partition_tree_tables() as is would have a benefit over time.
> Basically, as also discussed before, users can already use SQL to get the
> information they want out of the relevant catalogs (pg_inherits, etc.).
> But, such user queries might not be very future-proof as we might want to
> change the catalog organization in the future, so we'd like to provide
> users a proper interface to begin with. Keeping that in mind, it'd be
> better to think carefully about what we ought to be doing here. Input
> like yours is greatly helpful for that.
>
We could have the patch include pg_partition_root_parent and
pg_partition_parent, and leave the rest for a future CommitFest such
that more people could provide feedback on what they would like to see
in this space.
>>>> Maybe a function like pg_partition_number_of_partitions() could be of
>>>> benefit to count the number of actual partitions in a tree. Especially
>>>> useful in complex scenarios,
>>>>
>>>> select pg_partition_number_of_partitions('p') as number;
>>>>
>>>> number
>>>> ---------
>>>> 4
>>>> (1 row)
>>>
>>> Okay, adding one more function at this point may not be asking for too
>>> much. Although, select count(*) from pg_partition_tree_tables('p') would
>>> give you the count, a special function seems nice.
>>
>> Yeah, but I was thinking that the function would only return the number of
>> actual tables that contains data, e.g. not include 'p', 'p0' and 'p1' in
>> the count; otherwise you could use 'select count(*) from
>> pg_partition_children('p', true)' like you said.
>
> Maybe call it pg_partition_tree_leaf_count() or some such then?
>
That could work.
Best regards,
Jesper
Hi Jesper,
On 2018/07/20 21:26, Jesper Pedersen wrote:
> On 07/19/2018 10:27 PM, Amit Langote wrote:
>> On 2018/07/19 23:18, Jesper Pedersen wrote:
>>> What about renaming pg_partition_tree_tables() to pg_partition_children(),
>>> and have it work like
>>>
>>> select * from pg_partition_children('p', true);
>>> ---------
>>> p
>>> p0
>>> p1
>>> p00
>>> p01
>>> p10
>>> p11
>>> (7 rows)
>>>
>>> select * from pg_partition_children('p', false);
>>> ---------
>>> p0
>>> p1
>>> (2 rows)
>>>
>>> e.g. if 'bool include_all' is true all nodes under the node, including
>>> itself, are fetched. With false only nodes directly under the node,
>>> excluding itself, are returned. If there are no children NULL is returned.
>>
>> That's a big change to make to what this function does, but if that's
>> what's useful we could make it. As an alternative, wouldn't it help to
>> implement the idea that Dilip mentioned upthread of providing a function
>> to report the level of a given table in the partition hierarchy -- 0 for
>> root, 1 for its partitions and so on?
>
> Yes, Dilip's idea could work. I just don't think that
> pg_partition_tree_tables() as is would have a benefit over time.
Alright, I have replaced pg_partition_tree_tables with
pg_partition_children with an 'include_all' argument, as you suggested,
but I implemented it as an optional argument. So, one would use that
argument only if need to get *all* partitions. I have also added a
pg_partition_leaf_children() that returns just the leaf partitions, which
wasn't there in the previous versions.
Further, I've added a pg_partition_level that returns the level of a
partition in the partition tree wrt to the root of the *whole* partition
tree. But maybe we want this function to accept one more argument,
'rootoid', the OID of the root table against which to measure the level?
>> Basically, as also discussed before, users can already use SQL to get the
>> information they want out of the relevant catalogs (pg_inherits, etc.).
>> But, such user queries might not be very future-proof as we might want to
>> change the catalog organization in the future, so we'd like to provide
>> users a proper interface to begin with. Keeping that in mind, it'd be
>> better to think carefully about what we ought to be doing here. Input
>> like yours is greatly helpful for that.
>>
>
> We could have the patch include pg_partition_root_parent and
> pg_partition_parent, and leave the rest for a future CommitFest such that
> more people could provide feedback on what they would like to see in this
> space.
Yeah, that would be appreciated.
>>> Yeah, but I was thinking that the function would only return the number of
>>> actual tables that contains data, e.g. not include 'p', 'p0' and 'p1' in
>>> the count; otherwise you could use 'select count(*) from
>>> pg_partition_children('p', true)' like you said.
>>
>> Maybe call it pg_partition_tree_leaf_count() or some such then?
>
> That could work.
OK, I fixed it to return just the count of leaf partitions and renamed it
as such (pg_partition_children_leaf_count), but wonder if it's been made
redundant by the addition of pg_partition_leaf_children.
Thanks for the feedback.
Regards,
Amit
Вложения
Hi Amit, On 07/26/2018 04:47 AM, Amit Langote wrote: > Alright, I have replaced pg_partition_tree_tables with > pg_partition_children with an 'include_all' argument, as you suggested, > but I implemented it as an optional argument. So, one would use that > argument only if need to get *all* partitions. I have also added a > pg_partition_leaf_children() that returns just the leaf partitions, which > wasn't there in the previous versions. > Great. > Further, I've added a pg_partition_level that returns the level of a > partition in the partition tree wrt to the root of the *whole* partition > tree. But maybe we want this function to accept one more argument, > 'rootoid', the OID of the root table against which to measure the level? > I don't think that is needed, or it should at least be an optional parameter. >>> Maybe call it pg_partition_tree_leaf_count() or some such then? >> >> That could work. > > OK, I fixed it to return just the count of leaf partitions and renamed it > as such (pg_partition_children_leaf_count), but wonder if it's been made > redundant by the addition of pg_partition_leaf_children. > I think with pg_partition_leaf_children that we don't need the _count method, called pg_partition_tree_leaf_count in the docs, as we can just do a COUNT(). Best regards, Jesper
Hi Jesper, Thanks for the quick feedback. On 2018/07/27 1:30, Jesper Pedersen wrote: > On 07/26/2018 04:47 AM, Amit Langote wrote: >> Further, I've added a pg_partition_level that returns the level of a >> partition in the partition tree wrt to the root of the *whole* partition >> tree. But maybe we want this function to accept one more argument, >> 'rootoid', the OID of the root table against which to measure the level? > > I don't think that is needed, or it should at least be an optional parameter. Optional parameter sounds good, so made it get_partition_level(regclass [ , regclass ]) in the updated patch. Although, adding that argument is not without possible surprises its result might evoke. Like, what happens if you try to find the level of the root table by passing a leaf partition oid for the root table argument, or pass a totally unrelated table for the root table argument. For now, I've made the function return 0 for such cases. >> OK, I fixed it to return just the count of leaf partitions and renamed it >> as such (pg_partition_children_leaf_count), but wonder if it's been made >> redundant by the addition of pg_partition_leaf_children. >> > > I think with pg_partition_leaf_children that we don't need the _count > method, called pg_partition_tree_leaf_count in the docs, as we can just do > a COUNT(). Ah, okay. Removed pg_partition_tree_leaf_count. Thanks, Amit
Вложения
Hi Amit, On 07/26/2018 10:33 PM, Amit Langote wrote: > Optional parameter sounds good, so made it get_partition_level(regclass [ > , regclass ]) in the updated patch. Although, adding that argument is not > without possible surprises its result might evoke. Like, what happens if > you try to find the level of the root table by passing a leaf partition > oid for the root table argument, or pass a totally unrelated table for the > root table argument. For now, I've made the function return 0 for such cases. > As 0 is a valid return value for root nodes I think we should use -1 instead for these cases. Otherwise looks good. Best regards, Jesper
Hi, On 2018/07/27 21:21, Jesper Pedersen wrote: > Hi Amit, > > On 07/26/2018 10:33 PM, Amit Langote wrote: >> Optional parameter sounds good, so made it get_partition_level(regclass [ >> , regclass ]) in the updated patch. Although, adding that argument is not >> without possible surprises its result might evoke. Like, what happens if >> you try to find the level of the root table by passing a leaf partition >> oid for the root table argument, or pass a totally unrelated table for the >> root table argument. For now, I've made the function return 0 for such >> cases. >> > > As 0 is a valid return value for root nodes I think we should use -1 > instead for these cases. Makes sense, changed to be that way. Thanks, Amit
Вложения
Hi Amit, On 07/30/2018 05:21 AM, Amit Langote wrote: >> As 0 is a valid return value for root nodes I think we should use -1 >> instead for these cases. > > Makes sense, changed to be that way. > Looks good, the documentation for pg_partition_level could be expanded to describe the -1 scenario. New status: Ready for Committer Best regards, Jesper
Hi Jesper, On 2018/07/30 22:21, Jesper Pedersen wrote: > On 07/30/2018 05:21 AM, Amit Langote wrote: >>> As 0 is a valid return value for root nodes I think we should use -1 >>> instead for these cases. >> >> Makes sense, changed to be that way. >> > > Looks good, the documentation for pg_partition_level could be expanded to > describe the -1 scenario. Done. > New status: Ready for Committer Thanks. Updated patch attached. Regards, Amit
Вложения
On Thu, Jul 26, 2018 at 4:47 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > Alright, I have replaced pg_partition_tree_tables with > pg_partition_children with an 'include_all' argument, as you suggested, > but I implemented it as an optional argument. So, one would use that > argument only if need to get *all* partitions. I have also added a > pg_partition_leaf_children() that returns just the leaf partitions, which > wasn't there in the previous versions. > > Further, I've added a pg_partition_level that returns the level of a > partition in the partition tree wrt to the root of the *whole* partition > tree. But maybe we want this function to accept one more argument, > 'rootoid', the OID of the root table against which to measure the level? I have another idea. Suppose we just have one function, and that function a set of records, and each record contains (1) the OID of a table, (2) the OID of the immediate parent or NULL for the root, and (3) the level (0 = root, 1 = child, 2 = grandchild, etc.). So then to get the immediate children you would say: SELECT * FROM pg_whatever() WHERE level = 1 And to get everything you would just say: SELECT * FROM pg_whatever(); And if you wanted grandchildren or everything but the root or whatever you could just adjust the WHERE clause. By including the OID of the immediate parent, there's enough information for application code to draw an actual graph if it wants, which doesn't work so well if you just know the levels. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2018/08/01 22:21, Robert Haas wrote:
> On Thu, Jul 26, 2018 at 4:47 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Alright, I have replaced pg_partition_tree_tables with
>> pg_partition_children with an 'include_all' argument, as you suggested,
>> but I implemented it as an optional argument. So, one would use that
>> argument only if need to get *all* partitions. I have also added a
>> pg_partition_leaf_children() that returns just the leaf partitions, which
>> wasn't there in the previous versions.
>>
>> Further, I've added a pg_partition_level that returns the level of a
>> partition in the partition tree wrt to the root of the *whole* partition
>> tree. But maybe we want this function to accept one more argument,
>> 'rootoid', the OID of the root table against which to measure the level?
>
> I have another idea. Suppose we just have one function, and that
> function a set of records, and each record contains (1) the OID of a
> table, (2) the OID of the immediate parent or NULL for the root, and
> (3) the level (0 = root, 1 = child, 2 = grandchild, etc.).
>
> So then to get the immediate children you would say:
>
> SELECT * FROM pg_whatever() WHERE level = 1
>
> And to get everything you would just say:
>
> SELECT * FROM pg_whatever();
>
> And if you wanted grandchildren or everything but the root or whatever
> you could just adjust the WHERE clause.
>
> By including the OID of the immediate parent, there's enough
> information for application code to draw an actual graph if it wants,
> which doesn't work so well if you just know the levels.
That's a good idea, thanks.
Actually, by the time I sent the last version of the patch or maybe few
versions before that, I too had started thinking if we shouldn't just have
a SETOF RECORD function like you've outlined here, but wasn't sure of the
fields it should have. (relid, parentid, level) seems like a good start,
or maybe that's just what we need.
I tried to implement such a function. Example usage:
create table q (a int, b int, c int) partition by list (a);
create table q1 partition of q for values in (1) partition by hash (b);
create table q11 partition of q1 for values with (modulus 1, remainder 0)
partition by hash (c);
create table q111 partition of q11 for values with (modulus 1, remainder 0);
create table q2 partition of q for values in (2);
insert into q select i%2+1, i, i from generate_series(1, 1000) i;
select * from pg_partition_children('q');
relid │ parentid │ level
───────┼──────────┼───────
q │ │ 0
q1 │ q │ 1
q2 │ q │ 1
q11 │ q1 │ 2
q111 │ q11 │ 3
(5 rows)
select * from pg_partition_children('q') where level > 0;
relid │ parentid │ level
───────┼──────────┼───────
q1 │ q │ 1
q2 │ q │ 1
q11 │ q1 │ 2
q111 │ q11 │ 3
(4 rows)
select * from pg_partition_children('q') where level = 1;
relid │ parentid │ level
───────┼──────────┼───────
q1 │ q │ 1
q2 │ q │ 1
(2 rows)
select *, pg_relation_size(relid) as size from pg_partition_children('q');
relid │ parentid │ level │ size
───────┼──────────┼───────┼───────
q │ │ 0 │ 0
q1 │ q │ 1 │ 0
q2 │ q │ 1 │ 24576
q11 │ q1 │ 2 │ 0
q111 │ q11 │ 3 │ 24576
(5 rows)
select sum(pg_relation_size(relid)) as size from pg_partition_children('q');
size
───────
49152
(1 row)
select *, pg_relation_size(relid) as size from pg_partition_children('q1');
relid │ parentid │ level │ size
───────┼──────────┼───────┼───────
q1 │ q │ 0 │ 0
q11 │ q1 │ 1 │ 0
q111 │ q11 │ 2 │ 24576
(3 rows)
select *, pg_relation_size(relid) as size from pg_partition_children('q11');
relid │ parentid │ level │ size
───────┼──────────┼───────┼───────
q11 │ q1 │ 0 │ 0
q111 │ q11 │ 1 │ 24576
(2 rows)
select *, pg_relation_size(relid) as size from pg_partition_children('q111');
relid │ parentid │ level │ size
───────┼──────────┼───────┼───────
q111 │ q11 │ 0 │ 24576
(1 row)
Note that the level that's returned for each table is computed wrt the
root table passed to the function and not the actual root partition.
I have updated the patch to include just this one function, its
documentation, and tests.
Regards,
Amit
Вложения
Hi Amit, On 08/03/2018 04:28 AM, Amit Langote wrote: > That's a good idea, thanks. > > Actually, by the time I sent the last version of the patch or maybe few > versions before that, I too had started thinking if we shouldn't just have > a SETOF RECORD function like you've outlined here, but wasn't sure of the > fields it should have. (relid, parentid, level) seems like a good start, > or maybe that's just what we need. > I think there should be a column that identifies leaf partitions (bool isleaf), otherwise it isn't obvious in complex scenarios. > > Note that the level that's returned for each table is computed wrt the > root table passed to the function and not the actual root partition. > If you are given a leaf partition as input, then you will have to keep executing the query until you find the root, and count those. So, I think it should be either be the level to the root, or there should be another column that lists that (rootlevel). Best regards, Jesper
On Fri, Aug 3, 2018 at 8:35 AM, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > If you are given a leaf partition as input, then you will have to keep > executing the query until you find the root, and count those. So, I think it > should be either be the level to the root, or there should be another column > that lists that (rootlevel). I disagree. I think Amit has got the right semantics -- it gives you everything rooted at the partition you name, relative to that root. We could have another function which, given the OID of a partition, returns the topmost parent (or the immediate parent), but I think that if you say "tell me all the partitions of X", it should just tell you about stuff that's under X, regardless of what's over X. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On 08/03/2018 08:59 AM, Robert Haas wrote: > On Fri, Aug 3, 2018 at 8:35 AM, Jesper Pedersen > <jesper.pedersen@redhat.com> wrote: >> If you are given a leaf partition as input, then you will have to keep >> executing the query until you find the root, and count those. So, I think it >> should be either be the level to the root, or there should be another column >> that lists that (rootlevel). > > I disagree. I think Amit has got the right semantics -- it gives you > everything rooted at the partition you name, relative to that root. > We could have another function which, given the OID of a partition, > returns the topmost parent (or the immediate parent), but I think that > if you say "tell me all the partitions of X", it should just tell you > about stuff that's under X, regardless of what's over X. > We had the 2 pg_partition_level() functions and pg_partition_leaf_children() in v8, so it would be good to get those back. Best regards, Jesper
Hi,
On 2018/08/03 21:35, Jesper Pedersen wrote:
> Hi Amit,
>
> On 08/03/2018 04:28 AM, Amit Langote wrote:
>> That's a good idea, thanks.
>>
>> Actually, by the time I sent the last version of the patch or maybe few
>> versions before that, I too had started thinking if we shouldn't just have
>> a SETOF RECORD function like you've outlined here, but wasn't sure of the
>> fields it should have. (relid, parentid, level) seems like a good start,
>> or maybe that's just what we need.
>>
>
> I think there should be a column that identifies leaf partitions (bool
> isleaf), otherwise it isn't obvious in complex scenarios.
Ah, getting isleaf directly from pg_partition_children would be better
than an application figuring that out by itself.
>> Note that the level that's returned for each table is computed wrt the
>> root table passed to the function and not the actual root partition.
>
> If you are given a leaf partition as input, then you will have to keep
> executing the query until you find the root, and count those. So, I think
> it should be either be the level to the root, or there should be another
> column that lists that (rootlevel).
The function pg_partition_children is to get partitions found under a
given root table. If you pass a leaf partition to it, then there is
nothing under, just the leaf partition itself, and its level wrt itself is
0. That's what Robert said too, to which you replied:
On 2018/08/03 22:11, Jesper Pedersen wrote:
> We had the 2 pg_partition_level() functions and
> pg_partition_leaf_children() in v8, so it would be good to get those back.
Do we need a pg_partition_level that expects the individual partition OID
to be passed to it or can we do with the information we get from the
revised pg_partition_children? In earlier revisions,
pg_partition_children returned only the partition OIDs, so we needed to
provide pg_partition_* functions for getting the parent, root parent,
level, etc. separately. I mean to ask if is there a need for having these
functions separately if the revised pg_partition_children already outputs
that information?
pg_partition_leaf_children()'s output can be obtained as follows, after
adding isleaf column to pg_partition_children's output:
select * from pg_partition_children('<root>') where isleaf;
Attached updated patch adds isleaf to pg_partition_children's output.
Thanks,
Amit
Вложения
Hi,
On 08/07/2018 03:32 AM, Amit Langote wrote:
> Do we need a pg_partition_level that expects the individual partition OID
> to be passed to it or can we do with the information we get from the
> revised pg_partition_children? In earlier revisions,
> pg_partition_children returned only the partition OIDs, so we needed to
> provide pg_partition_* functions for getting the parent, root parent,
> level, etc. separately. I mean to ask if is there a need for having these
> functions separately if the revised pg_partition_children already outputs
> that information?
>
I'm thinking of the case where we only have information about a leaf
partition, and we need its root partition and the actual level in the
partition tree.
If we had
SELECT pg_partition_root_parent('leafpart');
and
SELECT pg_partition_level('leafpart');
-- we don't need the pg_partition_level('leafpart', 'parentpart')
function now.
We can use pg_partition_children() for the rest.
> pg_partition_leaf_children()'s output can be obtained as follows, after
> adding isleaf column to pg_partition_children's output:
>
Yes, this is great.
Thanks !
Best regards,
Jesper
On Tue, Aug 7, 2018 at 7:32 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Attached updated patch adds isleaf to pg_partition_children's output.
Hi Amit,
Hmm, I wonder where this garbage is coming from:
6201 select *, pg_relation_size(relid) as size from
pg_partition_children('ptif_test');
6202 ! ERROR: invalid byte sequence for encoding "UTF8": 0x8b
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.8126
--
Thomas Munro
http://www.enterprisedb.com
On Wed, Aug 8, 2018 at 11:21 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Tue, Aug 7, 2018 at 7:32 PM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> Attached updated patch adds isleaf to pg_partition_children's output. > > Hmm, I wonder where this garbage is coming from: partition.c:437:3: error: array index 3 is past the end of the array (which contains 3 elements) [-Werror,-Warray-bounds] That'll do it. Also: partition.c:409:19: error: implicit declaration of function 'get_rel_relkind' is invalid in C99 [-Werror,-Wimplicit-function-declaration] -- Thomas Munro http://www.enterprisedb.com
Thanks Thomas for notifying. On 2018/08/08 20:47, Thomas Munro wrote: > On Wed, Aug 8, 2018 at 11:21 PM, Thomas Munro > <thomas.munro@enterprisedb.com> wrote: >> On Tue, Aug 7, 2018 at 7:32 PM, Amit Langote >> <Langote_Amit_f8@lab.ntt.co.jp> wrote: >>> Attached updated patch adds isleaf to pg_partition_children's output. >> >> Hmm, I wonder where this garbage is coming from: > > partition.c:437:3: error: array index 3 is past the end of the array > (which contains 3 elements) [-Werror,-Warray-bounds] Oops, fixed. > That'll do it. Also: > > partition.c:409:19: error: implicit declaration of function > 'get_rel_relkind' is invalid in C99 > [-Werror,-Wimplicit-function-declaration] Fixed too. Attached updated patch. Thanks, Amit
Вложения
On Thu, Aug 09, 2018 at 01:05:56PM +0900, Amit Langote wrote:
> Attached updated patch.
So, except if I am missing something, what we have here is a patch which
has been debatted quite a bit and has semantics which look nice. Any
objections if we move forward with this patch?
+-- all tables in the tree
+select *, pg_relation_size(relid) as size from
pg_partition_children('ptif_test');
+ relid | parentid | level | isleaf | size
+-------------+------------+-------+--------+-------
+ ptif_test | | 0 | f | 0
+ ptif_test0 | ptif_test | 1 | f | 0
+ ptif_test1 | ptif_test | 1 | f | 0
+ ptif_test2 | ptif_test | 1 | t | 16384
+ ptif_test01 | ptif_test0 | 2 | t | 24576
One thing is that this test depends on the page size. There are already
plan modifications if running the regress tests with a size other than
8kB, but I don't think that we should make that worse, so I would
suggest to replace to use "pg_relation_size(relid) > 0" instead.
I have moved the patch to next CF for now.
--
Michael
Вложения
Hi,
On 2018/10/01 15:03, Michael Paquier wrote:
> On Thu, Aug 09, 2018 at 01:05:56PM +0900, Amit Langote wrote:
>> Attached updated patch.
>
> So, except if I am missing something, what we have here is a patch which
> has been debatted quite a bit and has semantics which look nice.
Thanks.
> Any
> objections if we move forward with this patch?
I wasn't able to respond to some of issues that Jesper brought up with the
approach taken by the latest patch whereby there is no separate
pg_partition_level function. He said that such a function would be useful
to get the information about the individual leaf partitions, but I was no
longer sure of providing such a function separately.
> +-- all tables in the tree
> +select *, pg_relation_size(relid) as size from
> pg_partition_children('ptif_test');
> + relid | parentid | level | isleaf | size
> +-------------+------------+-------+--------+-------
> + ptif_test | | 0 | f | 0
> + ptif_test0 | ptif_test | 1 | f | 0
> + ptif_test1 | ptif_test | 1 | f | 0
> + ptif_test2 | ptif_test | 1 | t | 16384
> + ptif_test01 | ptif_test0 | 2 | t | 24576
>
> One thing is that this test depends on the page size. There are already
> plan modifications if running the regress tests with a size other than
> 8kB, but I don't think that we should make that worse, so I would
> suggest to replace to use "pg_relation_size(relid) > 0" instead.
Might be a good idea, will do.
> I have moved the patch to next CF for now.
Thank you, I'll submit an updated version soon.
Regards,
Amit
On Mon, Oct 01, 2018 at 03:16:32PM +0900, Amit Langote wrote: > I wasn't able to respond to some of issues that Jesper brought up with the > approach taken by the latest patch whereby there is no separate > pg_partition_level function. He said that such a function would be useful > to get the information about the individual leaf partitions, but I was no > longer sure of providing such a function separately. Perhaps that could be debated separately as well? From what I can see what's available would unlock the psql patch which would like to add support for \dP, or show the size of partitions more easily. I am also not completely sure that I see the use-case for pg_partition_level or even pg_partition_root_parent as usually in their schemas users append rather similar relation names to the parent and the children. Or perhaps not? -- Michael
Вложения
On 2018/10/01 15:27, Michael Paquier wrote: > On Mon, Oct 01, 2018 at 03:16:32PM +0900, Amit Langote wrote: >> I wasn't able to respond to some of issues that Jesper brought up with the >> approach taken by the latest patch whereby there is no separate >> pg_partition_level function. He said that such a function would be useful >> to get the information about the individual leaf partitions, but I was no >> longer sure of providing such a function separately. > > Perhaps that could be debated separately as well? From what I can see > what's available would unlock the psql patch which would like to add > support for \dP, or show the size of partitions more easily. Yeah, maybe there is no reason to delay proceeding with pg_partition_children which provides a useful functionality. > I am also > not completely sure that I see the use-case for pg_partition_level or > even pg_partition_root_parent as usually in their schemas users append > rather similar relation names to the parent and the children. Or > perhaps not? We can continue discussing that once we're done dealing with pg_partition_children and then some other patches that are pending due to it such as Pavel's. Thanks, Amit
On Mon, Oct 01, 2018 at 04:27:57PM +0900, Amit Langote wrote:
> Yeah, maybe there is no reason to delay proceeding with
> pg_partition_children which provides a useful functionality.
So, I have been looking at your patch, and there are a couple of things
which could be improved.
Putting the new function pg_partition_children() in partition.c is a
bad idea I think. So instead I think that we should put that in a
different location, say utils/adt/partitionfuncs.c.
+ TupleDescInitEntry(tupdesc, (AttrNumber) 1, "relid",
+ REGCLASSOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 2, "parentid",
+ REGCLASSOID, -1, 0);
REGCLASSOID is used mainly for casting, so instead let's use OIDOID like
any other system function.
+ values[2] = psprintf("%d", level);
+ values[3] = psprintf("%c", relkind == RELKIND_PARTITIONED_TABLE ?
+ 'f' :
+ 't');
Using Datum objects is more elegant in this context.
isleaf is not particularly useful as it can just be fetched with a join
on pg_class/relkind. So I think that we had better remove it.
I have cleaned up a bit tests, removing duplicates and most of the
things which touched the size of relations to have something more
portable.
We could have a flavor using a relation name in input with qualified
names handled properly (see pg_get_viewdef_name for example), not sure
if that's really mandatory so I left that out. I have also added some
comments here and there. The docs could be worded a bit better still.
My result is the patch attached. What do you think?
--
Michael
Вложения
Hi Michael, On 10/2/18 11:37 PM, Michael Paquier wrote: > isleaf is not particularly useful as it can just be fetched with a join > on pg_class/relkind. So I think that we had better remove it. > Removing isleaf would require extra round trips to the server to get that information. So, I think we should keep it. Best regards, Jesper
On Wed, Oct 03, 2018 at 08:12:59AM -0400, Jesper Pedersen wrote:
> Removing isleaf would require extra round trips to the server to get
> that information. So, I think we should keep it.
I don't really get your point about extra round trips with the server,
and getting the same level of information is as simple as a join between
the result set of pg_partition_tree() and pg_class (better to add schema
qualification and aliases to relations by the way):
=# SELECT relid::regclass,
parentrelid::regclass, level,
relkind != 'p' AS isleaf
FROM pg_partition_tree('ptif_test'::regclass), pg_class
WHERE oid = relid;
relid | parentrelid | level | isleaf
-------------+-------------+-------+--------
ptif_test | null | 0 | f
ptif_test0 | ptif_test | 1 | f
ptif_test1 | ptif_test | 1 | f
ptif_test2 | ptif_test | 1 | t
ptif_test01 | ptif_test0 | 2 | t
ptif_test11 | ptif_test1 | 2 | t
(6 rows)
--
Michael
Вложения
On 2018/10/03 12:37, Michael Paquier wrote:
> On Mon, Oct 01, 2018 at 04:27:57PM +0900, Amit Langote wrote:
>> Yeah, maybe there is no reason to delay proceeding with
>> pg_partition_children which provides a useful functionality.
>
> So, I have been looking at your patch, and there are a couple of things
> which could be improved.
Thanks for reviewing and updating the patch.
> Putting the new function pg_partition_children() in partition.c is a
> bad idea I think. So instead I think that we should put that in a
> different location, say utils/adt/partitionfuncs.c.
Okay, sounds like a good idea.
> + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "relid",
> + REGCLASSOID, -1, 0);
> + TupleDescInitEntry(tupdesc, (AttrNumber) 2, "parentid",
> + REGCLASSOID, -1, 0);
> REGCLASSOID is used mainly for casting, so instead let's use OIDOID like
> any other system function.
Check.
> + values[2] = psprintf("%d", level);
> + values[3] = psprintf("%c", relkind == RELKIND_PARTITIONED_TABLE ?
> + 'f' :
> + 't');
> Using Datum objects is more elegant in this context.
Agreed. I think I'd just copied the psprintf code from some other function.
> isleaf is not particularly useful as it can just be fetched with a join
> on pg_class/relkind. So I think that we had better remove it.
That's a bit imposing on the users to know about relkind, but maybe that's
okay.
> I have cleaned up a bit tests, removing duplicates and most of the
> things which touched the size of relations to have something more
> portable.
Thanks for that.
> We could have a flavor using a relation name in input with qualified
> names handled properly (see pg_get_viewdef_name for example), not sure
> if that's really mandatory so I left that out.
Having to always use the typecast (::regclass) may be a bit annoying, but
we can always add that version later.
> I have also added some
> comments here and there. The docs could be worded a bit better still.
>
> My result is the patch attached. What do you think?
I looked at the updated patch and couldn't resist making some changes,
which see in the attached diff file. Also attached is the updated patch.
Thanks,
Amit
Вложения
On 2018/10/04 9:27, Michael Paquier wrote:
> On Wed, Oct 03, 2018 at 08:12:59AM -0400, Jesper Pedersen wrote:
>> Removing isleaf would require extra round trips to the server to get
>> that information. So, I think we should keep it.
>
> I don't really get your point about extra round trips with the server,
> and getting the same level of information is as simple as a join between
> the result set of pg_partition_tree() and pg_class (better to add schema
> qualification and aliases to relations by the way):
> =# SELECT relid::regclass,
> parentrelid::regclass, level,
> relkind != 'p' AS isleaf
> FROM pg_partition_tree('ptif_test'::regclass), pg_class
> WHERE oid = relid;
> relid | parentrelid | level | isleaf
> -------------+-------------+-------+--------
> ptif_test | null | 0 | f
> ptif_test0 | ptif_test | 1 | f
> ptif_test1 | ptif_test | 1 | f
> ptif_test2 | ptif_test | 1 | t
> ptif_test01 | ptif_test0 | 2 | t
> ptif_test11 | ptif_test1 | 2 | t
> (6 rows)
As mentioned in my other reply, that might be considered as asking the
user to know inner details like relkind. Also, if a database has many
partitioned tables with lots of partitions, the pg_class join might get
expensive. OTOH, computing and returning it with other fields of
pg_partition_tree is essentially free.
Thanks,
Amit
On Thu, Oct 04, 2018 at 04:53:02PM +0900, Amit Langote wrote: > As mentioned in my other reply, that might be considered as asking the > user to know inner details like relkind. Also, if a database has many > partitioned tables with lots of partitions, the pg_class join might get > expensive. OTOH, computing and returning it with other fields of > pg_partition_tree is essentially free. So it seems that I am clearly outvoted here ;) Okay, let's do as you folks propose. -- Michael
Вложения
On Thu, Oct 04, 2018 at 05:18:07PM +0900, Michael Paquier wrote: > So it seems that I am clearly outvoted here ;) > > Okay, let's do as you folks propose. And attached is a newer version with this isleaf stuff and the previous feedback from Amit integrated, as long as I recall about it. The version is indented, and tutti-quanti. Does that look fine to everybody here? The CF bot should normally pick up this patch, the previous garbage seen in one of the tests seems to come from the previous implementation which used strings... -- Michael
Вложения
pá 5. 10. 2018 v 7:57 odesílatel Michael Paquier <michael@paquier.xyz> napsal:
On Thu, Oct 04, 2018 at 05:18:07PM +0900, Michael Paquier wrote:
> So it seems that I am clearly outvoted here ;)
>
> Okay, let's do as you folks propose.
And attached is a newer version with this isleaf stuff and the previous
feedback from Amit integrated, as long as I recall about it. The
version is indented, and tutti-quanti. Does that look fine to everybody
here?
looks well
Pavel
The CF bot should normally pick up this patch, the previous garbage seen
in one of the tests seems to come from the previous implementation which
used strings...
--
Michael
On 2018/10/05 14:56, Michael Paquier wrote: > On Thu, Oct 04, 2018 at 05:18:07PM +0900, Michael Paquier wrote: >> So it seems that I am clearly outvoted here ;) >> >> Okay, let's do as you folks propose. > > And attached is a newer version with this isleaf stuff and the previous > feedback from Amit integrated, as long as I recall about it. The > version is indented, and tutti-quanti. Does that look fine to everybody > here? Thanks for making those changes yourself and posting the new version. Can you check the attached diff file for some updates to the documentation part of the patch. Other parts look fine. Thanks, Amit
Вложения
On Fri, Oct 05, 2018 at 03:31:49PM +0900, Amit Langote wrote: > Thanks for making those changes yourself and posting the new version. > > Can you check the attached diff file for some updates to the documentation > part of the patch. Other parts look fine. OK, I merged that into my local branch. From what I can see Mr Robot is happy as well: http://cfbot.cputube.org/amit-langote.html -- Michael
Вложения
On 10/5/18 2:52 AM, Michael Paquier wrote: > On Fri, Oct 05, 2018 at 03:31:49PM +0900, Amit Langote wrote: >> Thanks for making those changes yourself and posting the new version. >> >> Can you check the attached diff file for some updates to the documentation >> part of the patch. Other parts look fine. > > OK, I merged that into my local branch. From what I can see Mr Robot is > happy as well: > http://cfbot.cputube.org/amit-langote.html Looks good. Best regards, Jesper
On Fri, Oct 05, 2018 at 08:22:49AM -0400, Jesper Pedersen wrote: > Looks good. Actually, after sleeping on it, there could be potentially two problems: 1) We don't check the relkind of the relation. For example it is possible to get a tree from an index, which is incorrect. I would suggest to restrain the root relation to be either a relation, a partitioned table or a foreign table. 2) What about the users who can have a look at a tree? Shouldn't we tighten that a bit more? I am sure it is not fine to allow any user to look at what a partition tree looks like, hence only the owner of the root should be able to look at its tree, no? -- Michael
Вложения
On 2018/10/06 15:26, Michael Paquier wrote:
> On Fri, Oct 05, 2018 at 08:22:49AM -0400, Jesper Pedersen wrote:
>> Looks good.
>
> Actually, after sleeping on it, there could be potentially two problems:
> 1) We don't check the relkind of the relation. For example it is
> possible to get a tree from an index, which is incorrect. I would
> suggest to restrain the root relation to be either a relation, a
> partitioned table or a foreign table.
Hmm, how would one find the size of a partitioned index tree if we don't
allow indexes to be passed?
> 2) What about the users who can have a look at a tree? Shouldn't we
> tighten that a bit more? I am sure it is not fine to allow any user to
> look at what a partition tree looks like, hence only the owner of the
> root should be able to look at its tree, no?
Maybe, we should apply same rules as are used when describing a table or
when getting its metadata via other means (pg_relation_size, etc.)?
Afaics, there are no checks performed in that case:
create table foo (a int primary key);
create user mt;
revoke all on foo from mt;
set session authorization mt;
\d foo
Table "public.foo"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ not null │
Indexes:
"foo_pkey" PRIMARY KEY, btree (a)
select pg_relation_size('foo');
pg_relation_size
──────────────────
0
(1 row)
Should we do anything different in this case?
Thanks,
Amit
On 2018/10/06 15:26, Michael Paquier wrote:
> On Fri, Oct 05, 2018 at 08:22:49AM -0400, Jesper Pedersen wrote:
>> Looks good.
>
> Actually, after sleeping on it, there could be potentially two problems:
> 1) We don't check the relkind of the relation. For example it is
> possible to get a tree from an index, which is incorrect. I would
> suggest to restrain the root relation to be either a relation, a
> partitioned table or a foreign table.
Hmm, how would one find the size of a partitioned index tree if we don't
allow indexes to be passed?
> 2) What about the users who can have a look at a tree? Shouldn't we
> tighten that a bit more? I am sure it is not fine to allow any user to
> look at what a partition tree looks like, hence only the owner of the
> root should be able to look at its tree, no?
Maybe, we should apply same rules as are used when describing a table or
when getting its metadata via other means (pg_relation_size, etc.)?
Afaics, there are no checks performed in that case:
create table foo (a int primary key);
create user mt;
revoke all on foo from mt;
set session authorization mt;
\d foo
Table "public.foo"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ not null │
Indexes:
"foo_pkey" PRIMARY KEY, btree (a)
select pg_relation_size('foo');
pg_relation_size
──────────────────
0
(1 row)
Should we do anything different in this case?
Thanks,
Amit
On Tue, Oct 09, 2018 at 05:11:59PM +0900, Amit Langote wrote: > Hmm, how would one find the size of a partitioned index tree if we don't > allow indexes to be passed? pg_total_relation_size() and pg_indexes_size() are designed for that purpose. Anyway, the elements of a partition tree are things which can be directly attached to it, like a table or a foreign table, and not objects which are directly dependent on a given table, like indexes or toast tables. So we have to add some filtering on the relkind. Indexes, partitioned indexes and sequences are three things which cannot be added as partitions. But I think that we had better just make sure that the filtering allows RELKIND_RELATION, RELKIND_PARTITIONED_TABLE and RELKIND_FOREIGN_TABLE relations only. > Maybe, we should apply same rules as are used when describing a table or > when getting its metadata via other means (pg_relation_size, etc.)? > Afaics, there are no checks performed in that case: > > Should we do anything different in this case? Yeah, perhaps we could live without any restriction. There would be still time to tweak that behavior in the v12 development cycle if there are any complaints. -- Michael
Вложения
On 2018/10/09 18:10, Michael Paquier wrote: > On Tue, Oct 09, 2018 at 05:11:59PM +0900, Amit Langote wrote: >> Hmm, how would one find the size of a partitioned index tree if we don't >> allow indexes to be passed? > > pg_total_relation_size() and pg_indexes_size() are designed for that > purpose. Anyway, the elements of a partition tree are things which can > be directly attached to it, like a table or a foreign table, and not > objects which are directly dependent on a given table, like indexes or > toast tables. So we have to add some filtering on the relkind. > Indexes, partitioned indexes and sequences are three things which cannot > be added as partitions. But I think that we had better just make sure > that the filtering allows RELKIND_RELATION, RELKIND_PARTITIONED_TABLE > and RELKIND_FOREIGN_TABLE relations only. I think the way I phrased my question may have been a bit confusing. I was pointing out that just like tables, indexes can now also form their own partition tree. Partitioned indexes, just like partitioned tables, don't have their own storage, so pg_relation_size() cannot be used to obtain their size. We decided that the correct way to get the partitioned table's size is *not* to modify pg_relation_size itself to internally find all partitions and sum their sizes, but to provide a function that returns partitions and tell users to write SQL queries around it to calculate the total size. I'm saying that the new function should be able to be used with partitioned indexes too. Thanks, Amit
On Tue, Oct 09, 2018 at 06:41:59PM +0900, Amit Langote wrote: > Partitioned indexes, just like partitioned tables, don't have their own > storage, so pg_relation_size() cannot be used to obtain their size. We > decided that the correct way to get the partitioned table's size is *not* > to modify pg_relation_size itself to internally find all partitions and > sum their sizes, but to provide a function that returns partitions and > tell users to write SQL queries around it to calculate the total size. > I'm saying that the new function should be able to be used with > partitioned indexes too. I have to admit that I find the concept non-intuitive. A partition tree is a set of partitions based on a root called a partitioned table. A partitioned index is not a partition, it is a specific object which is associated to a partitioned table without any physical on-disk presence. An index is not a partition as well, it is an object associated to one relation. I am not saying that there is no merit in that. I honestly don't know, but being able to list all the partitioned tables and their partition tables looks enough to cover all the grounds discussed, and there is no need to work out more details for a new clone of find_all_inheritors and get_partition_ancestors. -- Michael
Вложения
On 2018/10/09 19:05, Michael Paquier wrote: > On Tue, Oct 09, 2018 at 06:41:59PM +0900, Amit Langote wrote: >> Partitioned indexes, just like partitioned tables, don't have their own >> storage, so pg_relation_size() cannot be used to obtain their size. We >> decided that the correct way to get the partitioned table's size is *not* >> to modify pg_relation_size itself to internally find all partitions and >> sum their sizes, but to provide a function that returns partitions and >> tell users to write SQL queries around it to calculate the total size. >> I'm saying that the new function should be able to be used with >> partitioned indexes too. > > I have to admit that I find the concept non-intuitive. A partition tree > is a set of partitions based on a root called a partitioned table. A > partitioned index is not a partition, it is a specific object which is > associated to a partitioned table without any physical on-disk presence. > An index is not a partition as well, it is an object associated to one > relation. I see it as follows: a partitioned index *does* have partitions and the partitions in that case are index objects, whereas, a partitioned table's partitions are table objects. IOW, I see the word "partition" as describing a relationship, not any specific object or kind of objects. > I am not saying that there is no merit in that. I honestly don't know, > but being able to list all the partitioned tables and their partition > tables looks enough to cover all the grounds discussed, and there is no > need to work out more details for a new clone of find_all_inheritors and > get_partition_ancestors. Sorry if I'm misunderstanding something, but why would we need a new clone? If we don't change pg_partition_tree() to only accept tables (regular/partitioned/foreign tables) as input, then the same code can work for indexes as well. As long as we store partition relationship in pg_inherits, same code should work for both. Thanks, Amit
On Tue, Oct 09, 2018 at 07:20:40PM +0900, Amit Langote wrote:
> Sorry if I'm misunderstanding something, but why would we need a new
> clone? If we don't change pg_partition_tree() to only accept tables
> (regular/partitioned/foreign tables) as input, then the same code can work
> for indexes as well. As long as we store partition relationship in
> pg_inherits, same code should work for both.
Okay, I see what you are coming at. However, please note that even if I
can see the dependencies in pg_inherits for indexes, the patch still
needs some work I am afraid if your intention is to do so:
CREATE TABLE ptif_test (a int, b int) PARTITION BY range (a);
create index ptif_test_i on ptif_test (a);
CREATE TABLE ptif_test0 PARTITION OF ptif_test
FOR VALUES FROM (minvalue) TO (0) PARTITION BY list (b);
=# select relid::regclass, parentrelid::regclass from
pg_partition_tree('ptif_test'::regclass);
relid | parentrelid
------------+-------------
ptif_test | null
ptif_test0 | ptif_test
(2 rows)
=# select relid::regclass, parentrelid::regclass from
pg_partition_tree('ptif_test_i'::regclass);
relid | parentrelid
-------------+-------------
ptif_test_i | null
(1 row)
In this case I would have expected ptif_test0_a_idx to be listed, with
ptif_test_i as a parent.
isleaf is of course wrong if the input is a partitioned index, so more
regression tests to cover those cases would be nice.
--
Michael
Вложения
On Tue, Oct 09, 2018 at 08:17:20PM +0900, Michael Paquier wrote: > isleaf is of course wrong if the input is a partitioned index, so more > regression tests to cover those cases would be nice. Also, the call to find_all_inheritors needs AccessShareLock... NoLock is not secure. -- Michael
Вложения
On 2018/10/09 20:17, Michael Paquier wrote:
> On Tue, Oct 09, 2018 at 07:20:40PM +0900, Amit Langote wrote:
>> Sorry if I'm misunderstanding something, but why would we need a new
>> clone? If we don't change pg_partition_tree() to only accept tables
>> (regular/partitioned/foreign tables) as input, then the same code can work
>> for indexes as well. As long as we store partition relationship in
>> pg_inherits, same code should work for both.
>
> Okay, I see what you are coming at. However, please note that even if I
> can see the dependencies in pg_inherits for indexes, the patch still
> needs some work I am afraid if your intention is to do so:
> CREATE TABLE ptif_test (a int, b int) PARTITION BY range (a);
> create index ptif_test_i on ptif_test (a);
> CREATE TABLE ptif_test0 PARTITION OF ptif_test
> FOR VALUES FROM (minvalue) TO (0) PARTITION BY list (b);
>
> =# select relid::regclass, parentrelid::regclass from
> pg_partition_tree('ptif_test'::regclass);
> relid | parentrelid
> ------------+-------------
> ptif_test | null
> ptif_test0 | ptif_test
> (2 rows)
> =# select relid::regclass, parentrelid::regclass from
> pg_partition_tree('ptif_test_i'::regclass);
> relid | parentrelid
> -------------+-------------
> ptif_test_i | null
> (1 row)
>
> In this case I would have expected ptif_test0_a_idx to be listed, with
> ptif_test_i as a parent.
I was partly wrong in saying that we wouldn't need any changes to support
partitioned indexes here. Actually, the core function
find_inheritance_children wouldn't scan pg_inherits for us if we pass an
(partitioned) index to it, even if the index may have children. That's
because of this quick-return code at the beginning of it:
/*
* Can skip the scan if pg_class shows the relation has never had a
* subclass.
*/
if (!has_subclass(parentrelId))
return NIL;
It appears that we don't set relhassubclass for partitioned indexes (that
is, on parent indexes), so the above the test is useless for indexes.
Maybe, we need to start another thread to discuss whether we should be
manipulating relhassubclass for index partitioning (I'd brought this up in
the context of relispartition, btw [1]). I'm not immediately sure if
setting relhassubclass correctly for indexes will have applications beyond
this one, but it might be something we should let others know so that we
can hear more opinions.
For time being, I've modified that code as follows:
@@ -68,9 +70,11 @@ find_inheritance_children(Oid parentrelId, LOCKMODE
lockmode)
/*
* Can skip the scan if pg_class shows the relation has never had a
- * subclass.
+ * subclass. Since partitioned indexes never have their
+ * relhassubclass set, we cannot skip the scan based on that.
*/
- if (!has_subclass(parentrelId))
+ if (get_rel_relkind(parentrelId) != RELKIND_PARTITIONED_INDEX &&
+ !has_subclass(parentrelId))
return NIL;
> isleaf is of course wrong if the input is a partitioned index, so more
> regression tests to cover those cases would be nice.
Yeah, I fixed that:
@@ -111,7 +111,8 @@ pg_partition_tree(PG_FUNCTION_ARGS)
nulls[1] = true;
/* isleaf */
- values[2] = BoolGetDatum(relkind != RELKIND_PARTITIONED_TABLE);
+ values[2] = BoolGetDatum(relkind != RELKIND_PARTITIONED_TABLE &&
+ relkind != RELKIND_PARTITIONED_INDEX);
On 2018/10/09 20:25, Michael Paquier wrote:
> Also, the call to find_all_inheritors needs AccessShareLock... NoLock
> is not secure.
I had thought about that, but don't remember why I made up my mind about
not taking any lock here. Maybe we should take locks. Fixed.
Attached updated patch. I updated docs and tests to include partitioned
indexes.
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/12085bc4-0bc6-0f3a-4c43-57fe0681772b%40lab.ntt.co.jp
Вложения
On Wed, Oct 10, 2018 at 11:54:48AM +0900, Amit Langote wrote: > I was partly wrong in saying that we wouldn't need any changes to support > partitioned indexes here. Actually, the core function > find_inheritance_children wouldn't scan pg_inherits for us if we pass an > (partitioned) index to it, even if the index may have children. > > It appears that we don't set relhassubclass for partitioned indexes (that > is, on parent indexes), so the above the test is useless for indexes. Aha, so that was it. > Maybe, we need to start another thread to discuss whether we should be > manipulating relhassubclass for index partitioning (I'd brought this up in > the context of relispartition, btw [1]). I'm not immediately sure if > setting relhassubclass correctly for indexes will have applications beyond > this one, but it might be something we should let others know so that we > can hear more opinions. This reminds of https://postgr.es/m/20180226053613.GD6927@paquier.xyz, which has resulted in relhaspkey being removed from pg_class with f66e8bf8. I would much prefer if we actually removed it... Now relhassubclass is more tricky than relhaspkey as it gets used as a fast-exit path for a couple of things: 1) prepunion.c when planning for child relations. 2) plancat.c 2) analyze.c 3) rewriteHandler.c > For time being, I've modified that code as follows: > > @@ -68,9 +70,11 @@ find_inheritance_children(Oid parentrelId, LOCKMODE > lockmode) > > /* > * Can skip the scan if pg_class shows the relation has never had a > - * subclass. > + * subclass. Since partitioned indexes never have their > + * relhassubclass set, we cannot skip the scan based on that. > */ > - if (!has_subclass(parentrelId)) > + if (get_rel_relkind(parentrelId) != RELKIND_PARTITIONED_INDEX && > + !has_subclass(parentrelId)) > return NIL; I don't like living with this hack. What if we simply nuked this fast-path exit all together? The only code path where this could represent a performance issue is RelationBuildPartitionKey(), but we expect a partitioned table to have children anyway, and there will be few cases where partitioned tables have no partitions. -- Michael
Вложения
On Tue, Oct 2, 2018 at 11:37 PM Michael Paquier <michael@paquier.xyz> wrote: > Putting the new function pg_partition_children() in partition.c is a > bad idea I think. So instead I think that we should put that in a > different location, say utils/adt/partitionfuncs.c. > > + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "relid", > + REGCLASSOID, -1, 0); > + TupleDescInitEntry(tupdesc, (AttrNumber) 2, "parentid", > + REGCLASSOID, -1, 0); > REGCLASSOID is used mainly for casting, so instead let's use OIDOID like > any other system function. -1. REGCLASSOID is a lot easier for humans to read and can still be joined to an OID column somewhere if needed. I think we should be striving to use types like regclass more often in system catalogs and views, not less often. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2018-Oct-12, Robert Haas wrote: > I think we should be striving to use types like regclass more often in > system catalogs and views, not less often. +1 -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/10/10 13:01, Michael Paquier wrote: > On Wed, Oct 10, 2018 at 11:54:48AM +0900, Amit Langote wrote: >> I was partly wrong in saying that we wouldn't need any changes to support >> partitioned indexes here. Actually, the core function >> find_inheritance_children wouldn't scan pg_inherits for us if we pass an >> (partitioned) index to it, even if the index may have children. >> >> It appears that we don't set relhassubclass for partitioned indexes (that >> is, on parent indexes), so the above the test is useless for indexes. > > Aha, so that was it. > >> Maybe, we need to start another thread to discuss whether we should be >> manipulating relhassubclass for index partitioning (I'd brought this up in >> the context of relispartition, btw [1]). I'm not immediately sure if >> setting relhassubclass correctly for indexes will have applications beyond >> this one, but it might be something we should let others know so that we >> can hear more opinions. > > This reminds of https://postgr.es/m/20180226053613.GD6927@paquier.xyz, > which has resulted in relhaspkey being removed from pg_class with > f66e8bf8. I would much prefer if we actually removed it... Now > relhassubclass is more tricky than relhaspkey as it gets used as a > fast-exit path for a couple of things: > 1) prepunion.c when planning for child relations. > 2) plancat.c > 2) analyze.c > 3) rewriteHandler.c I'm concerned about the 1st one. Currently in expand_inherited_rtentry(), checking relhassubclass allows us to short-circuit scanning pg_inherits to find out if a table has children. Note that expand_inherited_rtentry() is called for *all* queries that contain a table so that we correctly identify tables that would require inheritance planning. So, removing relhassubclass means a slight (?) degradation for *all* queries. >> For time being, I've modified that code as follows: >> >> @@ -68,9 +70,11 @@ find_inheritance_children(Oid parentrelId, LOCKMODE >> lockmode) >> >> /* >> * Can skip the scan if pg_class shows the relation has never had a >> - * subclass. >> + * subclass. Since partitioned indexes never have their >> + * relhassubclass set, we cannot skip the scan based on that. >> */ >> - if (!has_subclass(parentrelId)) >> + if (get_rel_relkind(parentrelId) != RELKIND_PARTITIONED_INDEX && >> + !has_subclass(parentrelId)) >> return NIL; > > I don't like living with this hack. What if we simply nuked this > fast-path exit all together? The only code path where this could > represent a performance issue is RelationBuildPartitionKey(), but we > expect a partitioned table to have children anyway, and there will be > few cases where partitioned tables have no partitions. As I said above, the price of removing relhassubclass might be a bit steep. So, the other alternative I mentioned before is to set relhassubclass correctly even for indexes if only for pg_partition_tree to be able to use find_inheritance_children unchanged. Thanks, Amit
On Fri, Oct 19, 2018 at 01:05:52PM +0900, Amit Langote wrote: > As I said above, the price of removing relhassubclass might be a bit > steep. So, the other alternative I mentioned before is to set > relhassubclass correctly even for indexes if only for pg_partition_tree to > be able to use find_inheritance_children unchanged. Playing devil's advocate a bit more... Another alternative here would be to remove the fast path using relhassubclass from find_inheritance_children and instead have its callers check for it :) Anyway, it seems that you are right here. Just setting relhassubclass for partitioned indexes feels more natural with what's on HEAD now. Even if I'd like to see all those hypothetical columns in pg_class go away, that cannot happen without a close lookup at the performance impact. -- Michael
Вложения
On 2018/10/19 16:47, Michael Paquier wrote: > On Fri, Oct 19, 2018 at 01:05:52PM +0900, Amit Langote wrote: >> As I said above, the price of removing relhassubclass might be a bit >> steep. So, the other alternative I mentioned before is to set >> relhassubclass correctly even for indexes if only for pg_partition_tree to >> be able to use find_inheritance_children unchanged. > > Playing devil's advocate a bit more... Another alternative here would > be to remove the fast path using relhassubclass from > find_inheritance_children and instead have its callers check for it :) Yeah, we could make it the responsibility of the callers of find_all_inheritors and find_inheritance_children to check relhassubclass as an optimization and remove any reference to relhassubclass from pg_inherits.c. Although we can write such a patch, it seems like it'd be bigger than the patch to ensure the correct value of relhassubclass for indexes, which I just posted on the other thread [1]. > Anyway, it seems that you are right here. Just setting relhassubclass > for partitioned indexes feels more natural with what's on HEAD now. > Even if I'd like to see all those hypothetical columns in pg_class go > away, that cannot happen without a close lookup at the performance > impact. Okay, I updated the patch on this thread. Since the updated patch depends on the correct value of relhassubclass being set for indexes, this patch should be applied on top of the other patch. I've attached here both. Another change I made is something Robert and Alvaro seem to agree about -- to use regclass instead of oid type as input/output columns. Thanks, Amit [1] https://www.postgresql.org/message-id/85d50b48-1b59-ae6c-e984-dd0b2926be3c%40lab.ntt.co.jp
Вложения
On Fri, Oct 19, 2018 at 06:55:09PM +0900, Amit Langote wrote:
> Yeah, we could make it the responsibility of the callers of
> find_all_inheritors and find_inheritance_children to check relhassubclass
> as an optimization and remove any reference to relhassubclass from
> pg_inherits.c. Although we can write such a patch, it seems like it'd be
> bigger than the patch to ensure the correct value of relhassubclass for
> indexes, which I just posted on the other thread [1].
And what is present as patch 0001 on this thread has been committed as
55853d6, so we are good for this part.
>> Anyway, it seems that you are right here. Just setting relhassubclass
>> for partitioned indexes feels more natural with what's on HEAD now.
>> Even if I'd like to see all those hypothetical columns in pg_class go
>> away, that cannot happen without a close lookup at the performance
>> impact.
>
> Okay, I updated the patch on this thread.
Thanks for the new version.
> Since the updated patch depends on the correct value of relhassubclass
> being set for indexes, this patch should be applied on top of the other
> patch. I've attached here both.
- if (!has_subclass(parentrelId))
+ if (get_rel_relkind(parentrelId) != RELKIND_PARTITIONED_INDEX &&
+ !has_subclass(parentrelId))
return NIL;
You don't need this bit anymore, relhassubclass is now set for
partitioned indexes.
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is not a table, a foreign table, or an index",
+ get_rel_name(rootrelid))));
Should this also list "partitioned tables and partitioned indexes"? The
style is heavy, but that maps with what pgstattuple does..
The tests should include also something for a leaf index when fed to
pg_partition_tree() (in order to control the index names you could just
attach an index to a partition after creating it, but I leave that up to
you).
+ <entry><literal><function>pg_partition_tree(<type>oid</type>)</function></literal></entry>
+ <entry><type>setof record</type></entry>
The change to regclass has not been reflected yet in the documentation
and the implementation, because...
> Another change I made is something Robert and Alvaro seem to agree about
> -- to use regclass instead of oid type as input/output columns.
... I am in minority here, it feels lonely ;)
--
Michael
Вложения
On 2018/10/29 12:59, Michael Paquier wrote:
> On Fri, Oct 19, 2018 at 06:55:09PM +0900, Amit Langote wrote:
>> Yeah, we could make it the responsibility of the callers of
>> find_all_inheritors and find_inheritance_children to check relhassubclass
>> as an optimization and remove any reference to relhassubclass from
>> pg_inherits.c. Although we can write such a patch, it seems like it'd be
>> bigger than the patch to ensure the correct value of relhassubclass for
>> indexes, which I just posted on the other thread [1].
>
> And what is present as patch 0001 on this thread has been committed as
> 55853d6, so we are good for this part.
Thank you for that. :)
>>> Anyway, it seems that you are right here. Just setting relhassubclass
>>> for partitioned indexes feels more natural with what's on HEAD now.
>>> Even if I'd like to see all those hypothetical columns in pg_class go
>>> away, that cannot happen without a close lookup at the performance
>>> impact.
>>
>> Okay, I updated the patch on this thread.
>
> Thanks for the new version.
>
>> Since the updated patch depends on the correct value of relhassubclass
>> being set for indexes, this patch should be applied on top of the other
>> patch. I've attached here both.
>
> - if (!has_subclass(parentrelId))
> + if (get_rel_relkind(parentrelId) != RELKIND_PARTITIONED_INDEX &&
> + !has_subclass(parentrelId))
> return NIL;
>
> You don't need this bit anymore, relhassubclass is now set for
> partitioned indexes.
Oh, how did I forget to do that! Removed.
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("\"%s\" is not a table, a foreign table, or an index",
> + get_rel_name(rootrelid))));
> Should this also list "partitioned tables and partitioned indexes"? The
> style is heavy, but that maps with what pgstattuple does..
Hmm, I think we mention the word "partitioned" in the error message only
if partitioning is required to perform an operation but it's absent (for
example, trying to attach partition to a non-partitioned table) or if its
presence prevents certain operation from being performed (for example,
calling pgrowlocks() on a partitioned table). Neither seems true in this
case. One can pass a relation of any of the types mentioned in the above
error message to pg_partition_tree and get some output from it.
> The tests should include also something for a leaf index when fed to
> pg_partition_tree() (in order to control the index names you could just
> attach an index to a partition after creating it, but I leave that up to
> you).
>
> + <entry><literal><function>pg_partition_tree(<type>oid</type>)</function></literal></entry>
> + <entry><type>setof record</type></entry>
>
> The change to regclass has not been reflected yet in the documentation
> and the implementation, because...
>
>> Another change I made is something Robert and Alvaro seem to agree about
>> -- to use regclass instead of oid type as input/output columns.
>
> ... I am in minority here, it feels lonely ;)
I've fixed the documentation to mention regclass as the input type. Also,
I've also modified tests to not use ::regclass.
Thanks,
Amit
Вложения
On Mon, Oct 29, 2018 at 04:08:09PM +0900, Amit Langote wrote: > Hmm, I think we mention the word "partitioned" in the error message only > if partitioning is required to perform an operation but it's absent (for > example, trying to attach partition to a non-partitioned table) or if its > presence prevents certain operation from being performed (for example, > calling pgrowlocks() on a partitioned table). Neither seems true in this > case. One can pass a relation of any of the types mentioned in the above > error message to pg_partition_tree and get some output from it. Okay.. We could always reword the error message if there are any complaints. > I've fixed the documentation to mention regclass as the input type. Also, > I've also modified tests to not use ::regclass. Thanks for the new version and using better index names. I have reviewed and committed the patch, with a couple of things tweaked: - removal of the tests on the size, they don't seem useful as part of showing partition information. - no need to join on pg_class for the test of the normal table. - added some comments and reformulated some other comments. - added a test with a materialized view. - added a test with a NULL input (as that's a strict function). -- Michael
Вложения
On 2018/10/30 10:33, Michael Paquier wrote: > Thanks for the new version and using better index names. I have > reviewed and committed the patch, with a couple of things tweaked: > - removal of the tests on the size, they don't seem useful as part of > showing partition information. > - no need to join on pg_class for the test of the normal table. > - added some comments and reformulated some other comments. > - added a test with a materialized view. > - added a test with a NULL input (as that's a strict function). Thank you! Regards, Amit