Hi Ashutosh, Please find my feedback inlined.
On Fri, Jul 28, 2017 at 7:00 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe > <jeevan.la...@enterprisedb.com> wrote: > > Hi, > > > > I have rebased the patches on the latest commit. > > > > Thanks for rebasing the patches. The patches apply and compile > cleanly. make check passes. > > Here are some review comments > 0001 patch > Most of this patch is same as 0002 patch posted in thread [1]. I have > extensively reviewed that patch for Amit Langote. Can you please compare > these > two patches and try to address those comments OR just use patch from that > thread? For example, canSkipPartConstraintValidation() is named as > PartConstraintImpliedByRelConstraint() in that patch. OR > + if (scanRel_constr == NULL) > + return false; > + > is not there in that patch since returning false is wrong when > partConstraint > is NULL. I think this patch needs those fixes. Also, this patch set would > need > a rebase when 0001 from that thread gets committed. > > I have renamed the canSkipPartConstraintValidation() to PartConstraintImpliedByRelConstraint() and made other changes applicable per Amit’s patch. This patch also refactors the scanning logic in ATExecAttachPartition() and adds it into a function ValidatePartitionConstraints(), hence I could not use Amit’s patch as it is. Please have a look into the new patch and let me know if it looks fine to you. > 0002 patch > + if (!and_args) > + result = NULL; > Add "NULL, if there are not partition constraints e.g. in case of default > partition as the only partition.". Added. Please check. > This patch avoids calling > validatePartitionConstraints() and hence canSkipPartConstraintValidation() > when > partConstraint is NULL, but patches in [1] introduce more callers of > canSkipPartConstraintValidation() which may pass NULL. So, it's better > that we > handle that case. > Following code added in patch 0001 now should take care of this. + num_check = (constr != NULL) ? constr->num_check : 0; > 0003 patch > + parentRel = heap_open(parentOid, AccessExclusiveLock); > In [2], Amit Langote has given a reason as to why heap_drop_with_catalog() > should not heap_open() the parent relation. But this patch still calls > heap_open() without giving any counter argument. Also I don't see > get_default_partition_oid() using Relation anywhere. If you remove that > heap_open() please remove following heap_close(). > + heap_close(parentRel, NoLock); > As clarified earlier this was addressed in 0004 patch of V24 series. In current set of patches this is now addressed in patch 0003 itself. > > + /* > + * The default partition accepts any non-specified > + * value, hence it should not get a mapped index > while > + * assigning those for non-null datums. > + */ > Instead of "any non-specified value", you may want to use "any value not > specified in the lists of other partitions" or something like that. > Changed the comment. > > + * If this is a NULL, route it to the null-accepting partition. > + * Otherwise, route by searching the array of partition bounds. > You may want to write it as "If this is a null partition key, ..." to > clarify > what's NULL. > Changed the comment. > > + * cur_index < 0 means we could not find a non-default partition > of > + * this parent. cur_index >= 0 means we either found the leaf > + * partition, or the next parent to find a partition of. > + * > + * If we couldn't find a non-default partition check if the > default > + * partition exists, if it does, get its index. > In order to avoid repeating "we couldn't find a ..."; you may want to add > ", > try default partition if one exists." in the first sentence itself. > Sorry, but I am not really sure how this change would make the comment more readable than the current one. > get_default_partition_oid() is defined in this patch and then redefined in > 0004. Let's define it only once, mostly in or before 0003 patch. > get_default_partition_oid() is now defined only once in patch 0003. > > + * partition strategy. Assign the parent strategy to the default > s/parent/parent's/ > Fixed. > > +-- attaching default partition overlaps if the default partition already > exists > +CREATE TABLE def_part PARTITION OF list_parted DEFAULT; > +CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS); > +ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT; > +ERROR: cannot attach a new partition to table "list_parted" having a > default partition > For 0003 patch this testcase is same as the testcase in the next hunk; no > new > partition can be added after default partition. Please add this testcase in > next set of patches. > Though the error message is same, the purpose of testing is different: 1. There cannot be more than one default partition, 2. and other is to test the fact the a new partition cannot be added if the default partition exists. The later test needs to be removed in next patch where we add support for adding new partition even if a default partition exists. > +-- fail > +insert into part_default values ('aa', 2); > May be explain why the insert should fail. "A row, which would fit > other partition, does not fit default partition, even when inserted > directly" > or something like that. I see that many of the tests in that file do not > explain why something should "fail" or be "ok", but may be it's better to > document the reason for better readability and future reference. > Added a comment. +-- check in case of multi-level default partitioned table > s/in/the/ ?. Or you may want to reword it as "default partitioned > partition in > multi-level partitioned table" as there is nothing like "default > partitioned > table". May be we need a testcase where every level of a multi-level > partitioned table has a default partition. > > I have changed the comment as well as added a test scenario where the partition further has a default partition. > +-- drop default, as we need to add some more partitions to test tuple > routing > Should be clubbed with the actual DROP statement? This is needed in patch 0003, as it prevents adding/creating further partitions to parent. This is removed in patch 0004. > +-- Check that addition or removal of any partition is correctly dealt > with by > +-- default partition table when it is being used in cached plan. > Plan of a prepared statement gets cached only after it's executed 5 times. > Before that the statement gets invalidated but there's not cached plan that > gets invalidated. The test is fine here, but in order to test the cached > plan > as mentioned in the comment, you will need to execute the statement 5 times > before executing drop statement. That's probably unnecessary, so just > modify > the comment to say "prepared statements instead of cached plan". > Agree. Fixed. > 0004 patch > The patch adds another column partdefid to catalog pg_partitioned_table. > The > column gives OID of the default partition for a given partitioned table. > This > means that the default partition's OID is stored at two places 1. in the > default partition table's pg_class entry and in pg_partitioned_table. > There is > no way to detect when these two go out of sync. Keeping those two in sync > is > also a maintenance burdern. Given that default partition's OID is required > only > while adding/dropping a partition, which is a less frequent operation, it > won't > hurt to join a few catalogs (pg_inherits and pg_class in this case) to > find out > the default partition's OID. That will be occasional performance hit > worth the otherwise maintenance burden. > To avoid partdefid of pg_partitioned_table going out of sync during any future developments I have added an assert in RelationBuildPartitionDesc() in patch 0003 in V25 series. I believe DBAs are not supposed to alter any catalog tables, hence instead of adding an error, I added an Assert to prevent this breaking during development cycle. We have similar kind of duplications in other catalogs e.g. pg_opfamily, pg_operator etc. Also, per Robert [1], the other route of searching pg_class and pg_inherits is going to cause some syscache bloat. [1] https://www.postgresql.org/message-id/CA%2BTgmobbnamyvii0pRdg9pp_jLHSUvq7u5SiRrVV0tEFFU58Tg%40mail.gmail.com Regards, Jeevan Ladhe