Re: [HACKERS] Default Partition for Range

On Mon, Jul 3, 2017 at 8:00 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Fri, Jun 30, 2017 at 11:56 AM, Beena Emerson <memissemerson@gmail.com> wrote:
>> Hello Dilip,
>>
>> On Wed, Jun 21, 2017 at 6:27 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>> On Tue, Jun 20, 2017 at 6:57 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>>> This is basically crashing in RelationBuildPartitionDesc, so I think
>
>>
>> Thank you for your review and analysis.
>>
>> I have updated the patch.
>> - bound->content is set to RANGE_DATUM_DEFAULT for each of the keys
>> and not just the first one.
>> - Improve the way of handling DEFAULT bounds in RelationBuildPartitionDesc,
>>
>> There is a test for multiple column range in alter_table.sql
>
> Thanks for update patch,  After this fix segmentation fault is solved.
>
> I have some more comments.
>
> 1.
>
> lower = make_one_range_bound(key, -1, spec->lowerdatums, true);
>   upper = make_one_range_bound(key, -1, spec->upperdatums, false);
>
> + /* Default case is not handled here */
> + if (spec->is_default)
> + break;
> +
>
> I think we can move default check just above the make range bound it
> will avoid unnecessary processing.
>

Removed the check here. Default is checked beforehand.

>
> 2.
> RelationBuildPartitionDesc
> {
> ....
>
>        /*
> * If either of them has infinite element, we can't equate
> * them.  Even when both are infinite, they'd have
> * opposite signs, because only one of cur and prev is a
> * lower bound).
> */
> if (cur->content[j] != RANGE_DATUM_FINITE ||
>   prev->content[j] != RANGE_DATUM_FINITE)
> {
> is_distinct = true;
> break;
> }
> After default range partitioning patch the above comment doesn't sound
> very accurate and
> can lead to the confusion, now here we are not only considering
> infinite bounds but
> there is also a possibility that prev bound can be DEFAULT, so
> comments should clearly
> mention that.

Modified.

>
> 3.
>
> + bound->datums = datums ? (Datum *) palloc0(key->partnatts * sizeof(Datum))
> + : NULL;
>   bound->content = (RangeDatumContent *) palloc0(key->partnatts *
>     sizeof(RangeDatumContent));
>   bound->lower = lower;
>
>   i = 0;
> +
> + /* datums are NULL for default */
> + if (datums == NULL)
> + for (i = 0; i < key->partnatts; i++)
> + bound->content[i] = RANGE_DATUM_DEFAULT;
>
> To me, it will look cleaner if we keep bound->content=NULL for
> default, instead of allocating memory
> and initializing each element,  But it's just a suggestions and you
> can decide whichever way
> seems better to you.  Then the other places e.g.
> + if (cur->content[i] == RANGE_DATUM_DEFAULT)
> + {
> + /*
>
> will change like
>
> + if (cur->content == NULL)
> + {
> + /*

I disagree. We use the content value RANGE_DATUM_DEFAULT during
comparing in partition_rbound_cmp and the code will not be very
intuiative if we use NULL instead of DEFAULT.


-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company