Re: Default Partition for Range

On Wed, May 31, 2017 at 5:53 PM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
> Hello,
>
> PFA the updated patch.
> Dependent patch default_partition_v17.patch [1]
> This patch adds regression tests and removes the separate function to
> get default partition qual.
>
>
> On Mon, May 29, 2017 at 3:22 PM, Jeevan Ladhe
> <jeevan(dot)ladhe(at)enterprisedb(dot)com> wrote:
>> Hi Beena,
>>
>> I went through your patch, and here are some of my comments:
>>
>> - For generating a qual for default range partition, instead of scanning for
>> all
>> the children and collecting all the boundspecs, how about creating and
>> negating
>> an expression from the lists of lowerdatums and upperdatums in boundinfo.
>>
>
> Unlike list, range partition can be for multiple columns and the
> expressions get complicated. I have used the same logic of looping
> through different partitions and negating the ORed expr. However, this
> is now done under get_qual_for_range.
>
>
>> - Wrong comment:
>> + int default_index; /* Index of the default list partition. */
>
> Comment is made generic in the dependent patch.
>
>>
>> - Suggested by Robert earlier on default list partitioning thread, instead
>> of
>> abbreviating is_def/found_def use is(found)_default etc.
>
> corrected.
>
>>
>> - unrelated change:
>> - List *all_parts;
>> + List *all_parts;
>>
>
> undone.
>
>> - typo: partiton should be partition, similar typo at other places too.
>> + * A non-default range partiton table does not currently allow partition
>> keys
>>
>
> rectified.
>
>> - Useless hunk for this patch:
>> - Oid defid;
>> + Oid defid;
>
> undone.
>
>>
>> - better to use IsA here, instead of doing explicit check:
>> + bound->content[i] = (datum->type == T_DefElem)
>> + ? RANGE_DATUM_DEFAULT
>
> Modified
>
>>
>> - It is better to use head of either lowerdatums or upperdatums list to
>> verify
>> if its a default partition and get rid of the constant PARTITION_DEFAULT
>> altogether.
>>
>
> modified this part as necessary.
>
>
>> - In the function get_qual_from_partbound() is_def is always going to be
>> false
>> for range partition, the function get_qual_for_range can be directly passed
>> false instead.
>>
>> - Following comment for function get_qual_for_range_default() implies that
>> this
>> function returns bool, but the actually it returns a List.
>> + *
>> + * If DEFAULT is the only partiton for the table then this returns TRUE.
>> + *
>>
> Updated.
>
> [1] http://www.mail-archive.com/pgsql-hackers(at)postgresql(dot)org/msg315573.html
>

Hi Beena,

I had a look at the patch from the angle of aesthetics and there are a
few cosmetic changes I might suggest. Please have a look at the
attached patch and if you agree with those changes you may merge it on
your patch. The patch also fixes a couple of more spelling mistakes
unrelated to this patch.

--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/