Re: Adding support for Default partition in partitioning

Hi Ashutosh,

I have tried to address your comments in the V22 set of patches[1].
Please find my feedback inlined on your comments.

On Thu, Jun 15, 2017 at 10:24 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.
>

Instead of reading the defaultOid from cache, as suggested by Amit here[2],
now
I have stored this in pg_partition_table, and reading it from there.

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

> Please add a testcase to test addition of default partition as the first
> partition.
>
> Added this in insert.sql rather than create_table.sql, as the purpose here
is to test if default being a first partition accepts any values for the key
including null.

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

Removed CacheMemoryContext.
I thought once the partition qual is generated, it should be in remain in
the memory context, but when it is needed, it is indirectly taken care by
generate_partition_qual() in following code:

/* Save a copy in the relcache */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
rel->rd_partcheck = copyObject(result);
MemoryContextSwitchTo(oldcxt);

>
> + 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?
>
>
Actually the OR expression here is generated using a call to makeBoolExpr(),
which returns a single expression node, and if this is passed to
make_ands_explicit(), it checks if the list length is node, returns the
initial
node itself, and hence AND(OR(...)) kind of expression is not generated
here.

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

> + *
> + * 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."
>
> I have reworded this to:
"If the given relation has a default partition return the OID of the default
partition, otherwise return 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.
>

Fixed.

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

Fixed.

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

I could not locate this hunk.

Regards,
Jeevan Ladhe

Refs:
[1] https://www.postgresql.org/message-id/CAOgcT0OARciE2X%
2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/35d68d49-555f-421a-99f8-185a44d085a4%40lab.ntt.co.jp