Hi Ashutosh,
On Thu, Aug 17, 2017 at 3:41 PM, Jeevan Ladhe <jeevan.la...@enterprisedb.com
> wrote:
> 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
>
>
You can see your comments addressed as above in patch series v25 here[1].
[1]
https://www.postgresql.org/message-id/CAOgcT0NwqnavYtu-QM-DAZ6N%3DwTiqKgy83WwtO2x94LSLZ1-Sw%40mail.gmail.com
Regards,
Jeevan Ladhe
