On 2018/10/29 12:59, Michael Paquier wrote:
> On Fri, Oct 19, 2018 at 06:55:09PM +0900, Amit Langote wrote:
>> Yeah, we could make it the responsibility of the callers of
>> find_all_inheritors and find_inheritance_children to check relhassubclass
>> as an optimization and remove any reference to relhassubclass from
>> pg_inherits.c. Although we can write such a patch, it seems like it'd be
>> bigger than the patch to ensure the correct value of relhassubclass for
>> indexes, which I just posted on the other thread [1].
>
> And what is present as patch 0001 on this thread has been committed as
> 55853d6, so we are good for this part.
Thank you for that. :)
>>> Anyway, it seems that you are right here. Just setting relhassubclass
>>> for partitioned indexes feels more natural with what's on HEAD now.
>>> Even if I'd like to see all those hypothetical columns in pg_class go
>>> away, that cannot happen without a close lookup at the performance
>>> impact.
>>
>> Okay, I updated the patch on this thread.
>
> Thanks for the new version.
>
>> Since the updated patch depends on the correct value of relhassubclass
>> being set for indexes, this patch should be applied on top of the other
>> patch. I've attached here both.
>
> - if (!has_subclass(parentrelId))
> + if (get_rel_relkind(parentrelId) != RELKIND_PARTITIONED_INDEX &&
> + !has_subclass(parentrelId))
> return NIL;
>
> You don't need this bit anymore, relhassubclass is now set for
> partitioned indexes.
Oh, how did I forget to do that! Removed.
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("\"%s\" is not a table, a foreign table, or an index",
> + get_rel_name(rootrelid))));
> Should this also list "partitioned tables and partitioned indexes"? The
> style is heavy, but that maps with what pgstattuple does..
Hmm, I think we mention the word "partitioned" in the error message only
if partitioning is required to perform an operation but it's absent (for
example, trying to attach partition to a non-partitioned table) or if its
presence prevents certain operation from being performed (for example,
calling pgrowlocks() on a partitioned table). Neither seems true in this
case. One can pass a relation of any of the types mentioned in the above
error message to pg_partition_tree and get some output from it.
> The tests should include also something for a leaf index when fed to
> pg_partition_tree() (in order to control the index names you could just
> attach an index to a partition after creating it, but I leave that up to
> you).
>
> + <entry><literal><function>pg_partition_tree(<type>oid</type>)</function></literal></entry>
> + <entry><type>setof record</type></entry>
>
> The change to regclass has not been reflected yet in the documentation
> and the implementation, because...
>
>> Another change I made is something Robert and Alvaro seem to agree about
>> -- to use regclass instead of oid type as input/output columns.
>
> ... I am in minority here, it feels lonely ;)
I've fixed the documentation to mention regclass as the input type. Also,
I've also modified tests to not use ::regclass.
Thanks,
Amit
