Re: Adding support for Default partition in partitioning

Hi,

On Thu, Sep 7, 2017 at 6:27 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> On Wed, Sep 6, 2017 at 5:50 PM, Jeevan Ladhe
> <jeevan(dot)ladhe(at)enterprisedb(dot)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.
>
>
I get your concern here that we are scanning the pg_partitioned_table
syscache
twice when we are building a partition descriptor; first in
RelationBuildPartitionKey() and next in RelationBuildPartitionDesc() when we
call get_default_partition_oid().

To avoid this, I can think of following three different solutions:
1.
Introduce a default partition OID field in PartitionKey structure, and
store the
partdefid while we scan pg_partitioned_table syscache in function
RelationBuildPartitionKey(). RelationBuildPartitionDesc() can later retrieve
this field from PartitionKey.

2.
Return the default OID RelationBuildPartitionKey() , and pass that as a
parameter to
RelationBuildPartitionDesc().

3.
Introduce a out parameter OID to function RelationBuildPartitionKey() which
would store
the partdefid, and pass that as a parameter to RelationBuildPartitionDesc().

I really do not think any of the above solution is very neat and organized
or
intuitive. While I understand that the syscache would be scanned twice if we
don’t fix this, we are not building a new cache here for
pg_partitioned_table,
we are just scanning it. Moreover, if there is a heavy OLTP going on this
partitioned table we could expect that this relation cache is going to be
mostly
there, and RelationBuildPartitionDesc() won’t happen for the same table more
often.

I guess it would be worth getting others(excluding me and Ashutosh)
opinion/views
also here.

> >
> >>
> >> + /* 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 have introduced a new function partdesc_get_defpart_oid() to
retrieve the default oid from the partition descriptor and used it
whereever we have relation partition desc available.
Also, I have renamed the existing function get get_default_partition_oid()
to partition_catalog_get_defpart_oid().

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

I still need to look into this.

Regards,
Jeevan Ladhe