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