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.gd6...@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
