Re: partition tree inspection functions - Mailing list pgsql-hackers

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