Re: Adding support for Default partition in partitioning

On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> 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.

I am *entirely* unconvinced by this line of argument. I think we want
to open the relation the first time we touch it and pass the Relation
around thereafter. Anything else is prone to accidentally failing to
have the relation locked early enough, or looking up the OID in the
relcache multiple times.

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

I think that one is probably worth fixing.

> Please add a testcase to test addition of default partition as the first
> partition.

That seems like a good idea, too.

> 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));
> + }

Clearly we do not want things to end up across multiple contexts. We
should ensure that anything linked from the relcache entry ends up in
CacheMemoryContext, but we must be careful not to allocate anything
else in there, because CacheMemoryContext is never reset.

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

I'm not sure it's worth the trouble.

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

No, because we should take the lock before examining any properties of
the table.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company