On Wed, Sep 6, 2017 at 5:50 PM, Jeevan Ladhe <jeevan.la...@enterprisedb.com> wrote: > >> >> I am wondering whether we could avoid call to get_default_partition_oid() >> in >> the above block, thus avoiding a sys cache lookup. The sys cache search >> shouldn't be expensive since the cache should already have that entry, but >> still if we can avoid it, we save some CPU cycles. The default partition >> OID is >> stored in pg_partition_table catalog, which is looked up in >> RelationGetPartitionKey(), a function which precedes >> RelationGetPartitionDesc() >> everywhere. What if that RelationGetPartitionKey() also returns the >> default >> partition OID and the common caller passes it to >> RelationGetPartitionDesc()?. > > > The purpose here is to cross check the relid with partdefid stored in > catalog > pg_partitioned_table, though its going to be the same in the parents cache, > I > think its better that we retrieve it from the catalog syscache. > Further, RelationGetPartitionKey() is a macro and not a function, so > modifying > the existing simple macro for this reason does not sound a good idea to me. > Having said this I am open to ideas here.
Sorry, I meant RelationBuildPartitionKey() and RelationBuildPartitionDesc() instead of RelationGetPartitionKey() and RelationGetPartitionDesc() resp. > >> >> + /* A partition cannot be attached if there exists a default partition >> */ >> + defaultPartOid = get_default_partition_oid(RelationGetRelid(rel)); >> + if (OidIsValid(defaultPartOid)) >> + ereport(ERROR, >> + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), >> + errmsg("cannot attach a new partition to table >> \"%s\" having a default partition", >> + RelationGetRelationName(rel)))); >> get_default_partition_oid() searches the catalogs, which is not needed >> when we >> have relation descriptor of the partitioned table (to which a new >> partition is >> being attached). You should get the default partition OID from partition >> descriptor. That will be cheaper. > > > Something like following can be done here: > /* A partition cannot be attached if there exists a default partition */ > if (partition_bound_has_default(rel->partdesc->boundinfo)) > ereport(ERROR, > (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > errmsg("cannot attach a new partition to table \"%s\" > having a default partition", > RelationGetRelationName(rel)))); > > But, partition_bound_has_default() is defined in partition.c and not in > partition.h. This is done that way because boundinfo is not available in > partition.h. Further, this piece of code is removed in next patch where we > extend default partition support to add/attach partition even when default > partition exists. So, to me I don’t see much of the correction issue here. If the code is being removed, I don't think we should sweat too much about it. Sorry for the noise. > > Another way to get around this is, we can define another version of > get_default_partition_oid() something like > get_default_partition_oid_from_parent_rel() > in partition.c which looks around in relcache instead of catalog and returns > the > oid of default partition, or get_default_partition_oid() accepts both parent > OID, > and parent ‘Relation’ rel, if rel is not null look into relcahce and return, > else search from catalog using OID. I think we should define a function to return default partition OID from partition descriptor and make it extern. Define a wrapper which accepts Relation and returns calls this function to get default partition OID from partition descriptor. The wrapper will be called only on an open Relation, wherever it's available. > >> I haven't gone through the full patch yet, so there may be more >> comments here. There is some duplication of code in >> check_default_allows_bound() and ValidatePartitionConstraints() to >> scan the children of partition being validated. The difference is that >> the first one scans the relations in the same function and the second >> adds them to work queue. May be we could use >> ValidatePartitionConstraints() to scan the relation or add to the >> queue based on some input flag may be wqueue argument itself. But I >> haven't thought through this completely. Any thoughts? > > > check_default_allows_bound() is called only from DefineRelation(), > and not for alter command. I am not really sure how can we use > work queue for create command. No, we shouldn't use work queue for CREATE command. We should extract the common code into a function to be called from check_default_allows_bound() and ValidatePartitionConstraints(). To that function we pass a flag (or the work queue argument itself), which decides whether to add a work queue item or scan the relation directly. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers