On 2016/08/17 14:33, Ashutosh Bapat wrote:
>> +relid_is_partition(Oid relid)
>> +{
>> + return SearchSysCacheExists1(PARTRELID, ObjectIdGetDatum(relid));
>> +}
>>
>> This is used in a lot of places, and the overhead of checking it in
>> all of those places is not necessarily nil. Syscache lookups aren't
>> free. What if we didn't create a new catalog for this and instead
>> just added a relpartitionbound attribute to pg_class? It seems a bit
>> silly to have a whole extra catalog to store one extra column...
>>
> It looks like in most of the places where this function is called it's
> using relid_is_partition(RelationGetRelid(rel)). Instead probably we should
> check existence of rd_partdesc or rd_partkey within Relation() and make
> sure that those members are always set for a partitioned table. That will
> avoid cache lookup and may give better performance.
It seems you are talking about a *partitioned* relation here, whereas
relid_is_partition() is to trying to check if a relation is *partition* by
looking up the pg_partition catalog (or the associated cache). For the
former, the test you suggest or rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE test is enough.
I am slightly tempted to eliminate the pg_partition catalog and associated
syscache altogether and add a column to pg_class as Robert suggested.
That way, all relid_is_partition() calls will be replaced by
rel->rd_partbound != NULL check. But one potential problem with that
approach is that now whenever a parent relation is opened, all the
partition relations must be opened to get the partbound value (to form the
PartitionDesc to be stored in parent relation's rd_partdesc). Whereas
currently, we just look up the pg_partition catalog (or the associated
cache) for every partition and that gets us the partbound.
> That brings up another question. Can we have rd_partdesc non null and
> rd_partkey null or vice-versa. If not, should we club those into a single
> structure like Partition (similar to Relation)?
It's true that rd_partkey and rd_partdesc are both either NULL or
non-NULL, so combining them into a single struct is an idea worth considering.
Thanks,
Amit
