Hello Dilip, On Mon, Jun 5, 2017 at 8:44 PM, Dilip Kumar <[email protected]> wrote: > On Mon, Jun 5, 2017 at 1:43 AM, Beena Emerson <[email protected]> wrote: >> The new patch is rebased over default_partition_v18.patch >> [http://www.mail-archive.com/[email protected]/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
default_range_partition_v4.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
