Hi Beena,

On Thu, Aug 17, 2017 at 4:59 PM, Beena Emerson <memissemer...@gmail.com>

> PFA the patch rebased over v25 patches of default list partition [1]

Thanks for rebasing.

Range partition review:

There are lot of changes in RelationBuildPartitionDesc(). It was hard to
understand why these changes are needed for default partition. I did not
any explanation for the same, may be I am missing some discussion? Only
when I looked into it carefully I could understand that these changes are
nothing but optimization for identifying the distinct bounds.
I think it is better to keep the patch for code optimisation/simplification
in a
separate patch. I have separated the patch(0001) in attached patches for

+ * For default partition, it returns the negation of the constraints of all
+ * the other partitions.
+ *
+ * If we end up with an empty result list, we return NULL.

We can rephrase the comment as below:

"For default partition, function returns the negation of the constraints of
the other partitions. If default is the only partition then returns NULL."

@@ -2396,6 +2456,8 @@ make_one_range_bound(PartitionKey key, int index,
List *datums, bool lower)
    ListCell   *lc;
    int         i;

+   Assert(datums != NULL);
    bound = (PartitionRangeBound *) palloc0(sizeof(PartitionRangeBound));
    bound->index = index;
    bound->datums = (Datum *) palloc0(key->partnatts * sizeof(Datum));

I am not really convinced, why should we have above Assert.

 static List *
-get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec)
+get_qual_for_range(Relation parent, PartitionBoundSpec *spec,
+                  bool for_default)

The addition and the way flag ‘for_default’ is being used is very confusing.
At this moment I am not able to think about a alternate solution to the
way you have used this flag. But will try and see if I can come up with
an alternate approach.

I still need to look at the test, and need to do some manual testing. Will
update here with any findings.

0001: Refactoring/simplification of code in RelationBuildPartitionDesc(),
0002: implementation of default range partition by Beena.


Attachment: 0001-Refactor-RelationBuildPartitionDesc.patch
Description: Binary data

Attachment: 0002-Add-support-for-default-partition-for-range.patch
Description: Binary data

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to