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. 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.". 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. 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); + /* + * 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. + * 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. + * 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. 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. + * partition strategy. Assign the parent strategy to the default s/parent/parent's/ +-- 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. +-- 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. +-- 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. +-- drop default, as we need to add some more partitions to test tuple routing Should be clubbed with the actual DROP statement? +-- 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". 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. I haven't reviewed next two patches, but those patches depend upon some of the comments above. So, it's better to consider these comments before looking at those patches. [1] https://www.postgresql.org/message-id/cee32590-68a7-8b56-5213-e07d9b8ab...@lab.ntt.co.jp [2] https://www.postgresql.org/message-id/35d68d49-555f-421a-99f8-185a44d08...@lab.ntt.co.jp -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers