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

Reply via email to