Re: [HACKERS] Default Partition for Range

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.


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.

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)
+ {
+ /*

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com