Re: partition tree inspection functions

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Jeevan Ladhe <jeevan(dot)ladhe(at)enterprisedb(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: partition tree inspection functions
Date: 2018-10-04 07:49:31
Message-ID: 6a73165a-72d9-f9b9-fad8-18d718d8464f@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
michael_v12_to_v13.diff text/plain 3.6 KB
partition-tree-func-v13.patch text/plain 10.1 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Ants Aasma 2018-10-04 07:49:40 Re: Skylake-S warning
Previous Message Amit Langote 2018-10-04 06:59:36 Re: executor relation handling