Re: [HACKERS] Adding support for Default partition in partitioning - Mailing list pgsql-hackers
From | Ashutosh Bapat |
---|---|
Subject | Re: [HACKERS] Adding support for Default partition in partitioning |
Date | |
Msg-id | CAFjFpRcBystGW-zAQt8S6uCLJM2ymwiW1ho2OrbiexVg98DW4A@mail.gmail.com Whole thread Raw |
In response to | Re: [HACKERS] Adding support for Default partition in partitioning (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>) |
Responses |
Re: [HACKERS] Adding support for Default partition in partitioning
(Robert Haas <robertmhaas@gmail.com>)
Re: [HACKERS] Adding support for Default partition in partitioning (Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>) |
List | pgsql-hackers |
Some more comments on the latest set of patches. In heap_drop_with_catalog(), we heap_open() the parent table to get the default partition OID, if any. If the relcache doesn't have an entry for the parent, this means that the entry will be created, only to be invalidated at the end of the function. If there is no default partition, this all is completely unnecessary. We should avoid heap_open() in this case. This also means that get_default_partition_oid() should not rely on the relcache entry, but should growl through pg_inherit to find the default partition. In get_qual_for_list(), if the table has only default partition, it won't have any boundinfo. In such a case the default partition's constraint would come out as (NOT ((a IS NOT NULL) AND (a = ANY (ARRAY[]::integer[])))). The empty array looks odd and may be we spend a few CPU cycles executing ANY on an empty array. We have the same problem with a partition containing only NULL value. So, may be this one is not that bad. Please add a testcase to test addition of default partition as the first partition. get_qual_for_list() allocates the constant expressions corresponding to the datums in CacheMemoryContext while constructing constraints for a default partition. We do not do this for other partitions. We may not be constructing the constraints for saving in the cache. For example, ATExecAttachPartition constructs the constraints for validation. In such a case, this code will unnecessarily clobber the cache memory. generate_partition_qual() copies the partition constraint in the CacheMemoryContext. + if (spec->is_default) + { + result = list_make1(make_ands_explicit(result)); + result = list_make1(makeBoolExpr(NOT_EXPR, result, -1)); + } If the "result" is an OR expression, calling make_ands_explicit() on it would create AND(OR(...)) expression, with an unnecessary AND. We want to avoid that? + if (cur_index < 0 && (partition_bound_has_default(partdesc->boundinfo))) + cur_index = partdesc->boundinfo->default_index; + The partition_bound_has_default() check is unnecessary since we check for cur_index < 0 next anyway. + * + * Given the parent relation checks if it has default partition, and if it + * does exist returns its oid, otherwise returns InvalidOid. + */ May be reworded as "If the given relation has a default partition, this function returns the OID of the default partition. Otherwise it returns InvalidOid." +Oid +get_default_partition_oid(Relation parent) +{ + PartitionDesc partdesc = RelationGetPartitionDesc(parent); + + if (partdesc->boundinfo && partition_bound_has_default(partdesc->boundinfo)) + return partdesc->oids[partdesc->boundinfo->default_index]; + + return InvalidOid; +} An unpartitioned table would not have partdesc set set. So, this function will segfault if we pass an unpartitioned table. Either Assert that partdesc should exist or check for its NULL-ness. + defaultPartOid = get_default_partition_oid(rel); + if (OidIsValid(defaultPartOid)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("there exists a default partition for table \"%s\", cannot attach a new partition", + RelationGetRelationName(rel)))); + Should be done before heap_open on the table being attached. If we are not going to attach the partition, there's no point in instantiating its relcache. The comment in heap_drop_with_catalog() should mention why we lock the default partition before locking the table being dropped. extern List *preprune_pg_partitions(PlannerInfo *root, RangeTblEntry *rte, Index rti, Node *quals,LOCKMODE lockmode); -#endif /* PARTITION_H */ Unnecessary hunk. On Thu, Jun 15, 2017 at 12:31 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > Oops, I meant to send one more comment. > > On 2017/06/15 15:48, Amit Langote wrote: >> BTW, I noticed the following in 0002 > + errmsg("there exists a default partition for table \"%s\", cannot > add a new partition", > > This error message style seems novel to me. I'm not sure about the best > message text here, but maybe: "cannot add new partition to table \"%s\" > with default partition" > > Note that the comment applies to both DefineRelation and > ATExecAttachPartition. > > Thanks, > Amit > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
pgsql-hackers by date: