Re: [HACKERS] Default Partition for Range

On Mon, Jun 5, 2017 at 1:43 AM, Beena Emerson <memissemerson@gmail.com> wrote:
> The new patch is rebased over default_partition_v18.patch
> [http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315831.html]

I have done the initial review of the patch, I have few comments.


+ 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.

-------------

- PartitionRangeDatum *datum = castNode(PartitionRangeDatum, lfirst(lc));
+ Node   *node = lfirst(lc);
+ PartitionRangeDatum *datum;
+
+ datum = castNode(PartitionRangeDatum, node);

Why do you need to change this?

--------------

+ if (!datums)
+ bound->content[i] = RANGE_DATUM_DEFAULT;
+

Better to check if (datums != NULL) for non boolean types.

-------------
+ 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.

-------------

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