Re: partition tree inspection functions

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