Re: Adding support for Default partition in partitioning

Hi Robert,

> 0005:
> > Extend default partitioning support to allow addition of new partitions.
>
> + if (spec->is_default)
> + {
> + /* Default partition cannot be added if there already
> exists one. */
> + if (partdesc->nparts > 0 &&
> partition_bound_has_default(boundinfo))
> + {
> + with = boundinfo->default_index;
> + ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("partition \"%s\"
> conflicts with existing default partition \"%s\"",
> + relname,
> get_rel_name(partdesc->oids[with])),
> + parser_errposition(pstate,
> spec->location)));
> + }
> +
> + return;
> + }
>
> I generally think it's good to structure the code so as to minimize
> the indentation level. In this case, if you did if (partdesc->nparts
> == 0 || !partition_bound_has_default(boundinfo)) return; first, then
> the rest of it could be one level less indented. Also, perhaps it
> would be clearer to test boundinfo == NULL rather than
> partdesc->nparts == 0, assuming they are equivalent.

I think even with this change there will be one level of indentation
needed for throwing the error, as the error is to be thrown only if
there exists a default partition.

>
>
- * We must also lock the default partition, for the same
> reasons explained
> - * in heap_drop_with_catalog().
> + * We must lock the default partition, for the same reasons
> explained in
> + * DefineRelation().
>
> I don't really see the point of this change. Whichever earlier patch
> adds this code could include or omit the word "also" as appropriate,
> and then this patch wouldn't need to change it.
>
>
Actually the change is made because if the difference in the function name.
I will remove ‘also’ from the first patch itself.

> > 0007:
> > This patch introduces code to check if the scanning of default partition
> > child
> > can be skipped if it's constraints are proven.
>
> If I understand correctly, this is actually a completely separate
> feature not intrinsically related to default partitioning.

I don't see this as a new feature, since scanning the default partition
will be introduced by this series of patches only, and rather than a
feature this can be classified as a completeness of default skip
validation logic. Your thoughts?

Regards,
Jeevan Ladhe