Re: partition tree inspection functions

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