Re: Default Partition for Range

Hello Dilip,

On Mon, Jun 5, 2017 at 8:44 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> On Mon, Jun 5, 2017 at 1:43 AM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
>> The new patch is rebased over default_partition_v18.patch
>> [http://www.mail-archive.com/pgsql-hackers(at)postgresql(dot)org/msg315831.html]
>
> I have done the initial review of the patch, I have few comments.
>
Thank you.
>
> + if ((lower->content[0] == RANGE_DATUM_DEFAULT))
> + {
> + if (partition_bound_has_default(partdesc->boundinfo))
> + {
> + overlap = true;
> + with = partdesc->boundinfo->default_index;
> + }
>
> I think we can change if ((lower->content[0] == RANGE_DATUM_DEFAULT))
> check to if (spec->is_default) that way it will be consistent with the
> check in the PARTITION_STRATEGY_LIST. Or we can move this complete
> check outside of the switch.

I have now moved the is_default check for both list and range outside
the switch case.

>
> -------------
>
> - PartitionRangeDatum *datum = castNode(PartitionRangeDatum, lfirst(lc));
> + Node *node = lfirst(lc);
> + PartitionRangeDatum *datum;
> +
> + datum = castNode(PartitionRangeDatum, node);
>
> Why do you need to change this?

I forgot to remove it.
It was needed for previous version of patch but no longer needed and
hence reverted this change.

>
> --------------
>
> + if (!datums)
> + bound->content[i] = RANGE_DATUM_DEFAULT;
> +
>
> Better to check if (datums != NULL) for non boolean types.

done.

>
> -------------
> + if (content1[i] == RANGE_DATUM_DEFAULT ||
> + content2[i] == RANGE_DATUM_DEFAULT)
> + {
> + if (content1[i] == content2[i])
> + return 0;
> + else if (content1[i] == RANGE_DATUM_DEFAULT)
> + return -1;
>
> I don't see any comments why default partition should be considered
> smallest in the index comparison. For negative infinity, it's pretty
> obvious by the enum name itself.

Default could be lowest or highest, no specific reason for putting it lowest.
I have not added any comments in this version.

--

Beena Emerson

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