Horton

Thread

Commits

  1. Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition bounds.

  2. Clarify the contract of partition_rbound_cmp().

  3. Simplify the logic checking new range partition bounds.

  1. Multi column range partition table

    Amul Sul <sulamul@gmail.com> — 2017-06-22T11:48:28Z

    Hi,

While working on the another patch, I came across the case where
I need an auto generated partition for a mutil-column range partitioned
table having following range bound:

PARTITION p1 FROM  (UNBOUNDED, UNBOUNDED) TO (10, 10)
PARTITION p2 FROM  (10, 10)  TO (10, UNBOUNDED)
PARTITION p3 FROM  (10, UNBOUNDED) TO (20, 10)
PARTITION p4 FROM (20, 10) TO (20, UNBOUNDED)
PARTITION p5 FROM (20, UNBOUNDED) TO (UNBOUNDED, UNBOUNDED)

In this, a lower bound of the partition is an upper bound of the
previous partition.

While trying to create p3 partition with (10, UNBOUNDED) to (20, 10) bound,
got an overlap partition error.

Here is the SQL to reproduced this error:

CREATE TABLE range_parted ( i1 int,  i2 int ) PARTITION BY RANGE (i1, i2);
CREATE TABLE p1 PARTITION OF range_parted FOR VALUES FROM (UNBOUNDED,
UNBOUNDED) TO (10, 10);
CREATE TABLE p2 PARTITION OF range_parted FOR VALUES FROM (10, 10) TO
(10, UNBOUNDED);
CREATE TABLE p3   PARTITION OF tab1 FOR VALUES FROM (10, UNBOUNDED) TO (20, 10);

ERROR:  partition "p3" would overlap partition "tab1_p_10_10"

This happened because of UNBOUNDED handling, where it is a negative infinite
if it is in FROM clause.  Wondering can't we explicitly treat this as
a positive infinite value, can we?

Thoughts/Comments?

Regards,
Amul

  2. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-06-23T01:28:21Z

    On 2017/06/22 20:48, amul sul wrote:
> Hi,
> 
> While working on the another patch, I came across the case where
> I need an auto generated partition for a mutil-column range partitioned
> table having following range bound:
> 
> PARTITION p1 FROM  (UNBOUNDED, UNBOUNDED) TO (10, 10)
> PARTITION p2 FROM  (10, 10)  TO (10, UNBOUNDED)
> PARTITION p3 FROM  (10, UNBOUNDED) TO (20, 10)
> PARTITION p4 FROM (20, 10) TO (20, UNBOUNDED)
> PARTITION p5 FROM (20, UNBOUNDED) TO (UNBOUNDED, UNBOUNDED)
> 
> In this, a lower bound of the partition is an upper bound of the
> previous partition.
> 
> While trying to create p3 partition with (10, UNBOUNDED) to (20, 10) bound,
> got an overlap partition error.
> 
> Here is the SQL to reproduced this error:
> 
> CREATE TABLE range_parted ( i1 int,  i2 int ) PARTITION BY RANGE (i1, i2);
> CREATE TABLE p1 PARTITION OF range_parted FOR VALUES FROM (UNBOUNDED,
> UNBOUNDED) TO (10, 10);
> CREATE TABLE p2 PARTITION OF range_parted FOR VALUES FROM (10, 10) TO
> (10, UNBOUNDED);
> CREATE TABLE p3   PARTITION OF tab1 FOR VALUES FROM (10, UNBOUNDED) TO (20, 10);
> 
> ERROR:  partition "p3" would overlap partition "tab1_p_10_10"
> 
> This happened because of UNBOUNDED handling, where it is a negative infinite
> if it is in FROM clause.  Wondering can't we explicitly treat this as
> a positive infinite value, can we?

No, we cannot.  What would be greater than (or equal to) +infinite?
Nothing.  So, even if you will want p3 to accept (10, 9890148), it won't
because 9890148 is not >= +infinite.  It will accept only the rows where
the first column is > 10 (second column is not checked in that case).

You will have to define p3 as follows:

CREATE TABLE p3 PARTITION OF tab1 FOR VALUES FROM (11, UNBOUNDED) TO (20, 10);

It's fine to use the previous partition's upper bound as the lower bound
of the current partition, if the former does contain an UNBOUNDED value,
because whereas a finite value divides the range into two parts (assigned
to the two partitions respectively), an UNBOUNDED value does not.  The
latter represents an abstract end of the range (either on the positive
side or the negative).

Does that make sense?

Thanks,
Amit

  3. Re: Multi column range partition table

    Amul Sul <sulamul@gmail.com> — 2017-06-23T04:42:48Z

    On Fri, Jun 23, 2017 at 6:58 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/06/22 20:48, amul sul wrote:
>> Hi,
>>
>> While working on the another patch, I came across the case where
>> I need an auto generated partition for a mutil-column range partitioned
>> table having following range bound:
>>
>> PARTITION p1 FROM  (UNBOUNDED, UNBOUNDED) TO (10, 10)
>> PARTITION p2 FROM  (10, 10)  TO (10, UNBOUNDED)
>> PARTITION p3 FROM  (10, UNBOUNDED) TO (20, 10)
>> PARTITION p4 FROM (20, 10) TO (20, UNBOUNDED)
>> PARTITION p5 FROM (20, UNBOUNDED) TO (UNBOUNDED, UNBOUNDED)
>>
>> In this, a lower bound of the partition is an upper bound of the
>> previous partition.
>>
>> While trying to create p3 partition with (10, UNBOUNDED) to (20, 10) bound,
>> got an overlap partition error.
>>
>> Here is the SQL to reproduced this error:
>>
>> CREATE TABLE range_parted ( i1 int,  i2 int ) PARTITION BY RANGE (i1, i2);
>> CREATE TABLE p1 PARTITION OF range_parted FOR VALUES FROM (UNBOUNDED,
>> UNBOUNDED) TO (10, 10);
>> CREATE TABLE p2 PARTITION OF range_parted FOR VALUES FROM (10, 10) TO
>> (10, UNBOUNDED);
>> CREATE TABLE p3   PARTITION OF tab1 FOR VALUES FROM (10, UNBOUNDED) TO (20, 10);
>>
>> ERROR:  partition "p3" would overlap partition "tab1_p_10_10"
>>
>> This happened because of UNBOUNDED handling, where it is a negative infinite
>> if it is in FROM clause.  Wondering can't we explicitly treat this as
>> a positive infinite value, can we?
>
> No, we cannot.  What would be greater than (or equal to) +infinite?
> Nothing.  So, even if you will want p3 to accept (10, 9890148), it won't
> because 9890148 is not >= +infinite.  It will accept only the rows where
> the first column is > 10 (second column is not checked in that case).
>
> You will have to define p3 as follows:
>
> CREATE TABLE p3 PARTITION OF tab1 FOR VALUES FROM (11, UNBOUNDED) TO (20, 10);
>
What if the partition key column is FLOAT ?

Regards,
Amul

  4. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-06-23T06:02:04Z

    On 2017/06/23 13:42, amul sul wrote:
> On Fri, Jun 23, 2017 at 6:58 AM, Amit Langote wrote:
>> On 2017/06/22 20:48, amul sul wrote:
>>> This happened because of UNBOUNDED handling, where it is a negative infinite
>>> if it is in FROM clause.  Wondering can't we explicitly treat this as
>>> a positive infinite value, can we?
>>
>> No, we cannot.  What would be greater than (or equal to) +infinite?
>> Nothing.  So, even if you will want p3 to accept (10, 9890148), it won't
>> because 9890148 is not >= +infinite.  It will accept only the rows where
>> the first column is > 10 (second column is not checked in that case).
>>
>> You will have to define p3 as follows:
>>
>> CREATE TABLE p3 PARTITION OF tab1 FOR VALUES FROM (11, UNBOUNDED) TO (20, 10);
>>
> What if the partition key column is FLOAT ?

I would say use a value such that the btfloat4cmp (or btfloat8cmp) will
tell it to be greater than 10.

Of course, we can't write what I just said in the user-level
documentation, because the fact that we use system- or user-defined btree
comparison proc (btfloat4/8cmp) for partitioning may be irrelevant to the
users.  Although, we do mention in the documentation that we use btree
operator class specified semantics for partitioning.  In any case,
defining your partitioning or indexing on raw float type column(s) is
prone to semantic caveats, I'd think.

Also, there was interesting exchange on this topic during the patch
development [1].  Excerpt:

"Same for ranges of floating-point numbers, which are also probably an
unlikely candidate for a partitioning key anyway."

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoaucSqQ%3DdJFhaojSpb1706MQYo1Tfn_3tWv6CVWhAOdrQ%40mail.gmail.com

  5. Re: Multi column range partition table

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-23T07:01:04Z

    On Fri, Jun 23, 2017 at 6:58 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/06/22 20:48, amul sul wrote:
>> Hi,
>>
>> While working on the another patch, I came across the case where
>> I need an auto generated partition for a mutil-column range partitioned
>> table having following range bound:
>>
>> PARTITION p1 FROM  (UNBOUNDED, UNBOUNDED) TO (10, 10)
>> PARTITION p2 FROM  (10, 10)  TO (10, UNBOUNDED)
>> PARTITION p3 FROM  (10, UNBOUNDED) TO (20, 10)
>> PARTITION p4 FROM (20, 10) TO (20, UNBOUNDED)
>> PARTITION p5 FROM (20, UNBOUNDED) TO (UNBOUNDED, UNBOUNDED)
>>
>> In this, a lower bound of the partition is an upper bound of the
>> previous partition.
>>
>> While trying to create p3 partition with (10, UNBOUNDED) to (20, 10) bound,
>> got an overlap partition error.
>>
>> Here is the SQL to reproduced this error:
>>
>> CREATE TABLE range_parted ( i1 int,  i2 int ) PARTITION BY RANGE (i1, i2);
>> CREATE TABLE p1 PARTITION OF range_parted FOR VALUES FROM (UNBOUNDED,
>> UNBOUNDED) TO (10, 10);
>> CREATE TABLE p2 PARTITION OF range_parted FOR VALUES FROM (10, 10) TO
>> (10, UNBOUNDED);
>> CREATE TABLE p3   PARTITION OF tab1 FOR VALUES FROM (10, UNBOUNDED) TO (20, 10);
>>
>> ERROR:  partition "p3" would overlap partition "tab1_p_10_10"
>>
>> This happened because of UNBOUNDED handling, where it is a negative infinite
>> if it is in FROM clause.  Wondering can't we explicitly treat this as
>> a positive infinite value, can we?

The way we have designed our syntax, we don't have a way to tell that
p3 comes after p2 and they have no gap between those. But I don't
think that's your question. What you are struggling with is a way to
specify a lower bound (10, +infinity) so that anything with i1 > 10
would go to partition 3.

>
> No, we cannot.  What would be greater than (or equal to) +infinite?
> Nothing.  So, even if you will want p3 to accept (10, 9890148), it won't
> because 9890148 is not >= +infinite.  It will accept only the rows where
> the first column is > 10 (second column is not checked in that case).
>
> You will have to define p3 as follows:
>
> CREATE TABLE p3 PARTITION OF tab1 FOR VALUES FROM (11, UNBOUNDED) TO (20, 10);

That's not exactly same as specifying (10, +infinity) in case i1 is a
float. A user can not precisely tell what would be the acceptable
value just greater than 10.

An UNBOUNDED in the lower bound is always considered as -infinity for
that data type. There is no way to specify a lower bound which has
+infinity in it. +infinite as a lower bounds for the first key may not
make sense (although that means that the partition will always be
empty), but it does make sense for keys after the first as Amul has
explained below.

The question is do we have support for that and if not, will we
consider it for v10 or v11 and how.

>
> It's fine to use the previous partition's upper bound as the lower bound
> of the current partition, if the former does contain an UNBOUNDED value,
> because whereas a finite value divides the range into two parts (assigned
> to the two partitions respectively), an UNBOUNDED value does not.  The
> latter represents an abstract end of the range (either on the positive
> side or the negative).

Not exactly for second key onwards.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  6. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-06-23T08:00:09Z

    On 23 June 2017 at 08:01, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> The way we have designed our syntax, we don't have a way to tell that
> p3 comes after p2 and they have no gap between those. But I don't
> think that's your question. What you are struggling with is a way to
> specify a lower bound (10, +infinity) so that anything with i1 > 10
> would go to partition 3.
>

I think actually there is a fundamental problem here, which arises
because UNBOUNDED has 2 different meanings depending on context, and
thus it is not possible in general to specify the start of one range
to be equal to the end of the previous range, as is necessary to get
contiguous non-overlapping ranges.

Note that this isn't just a problem for floating point datatypes
either, it also applies to other types such as strings. For example,
given a partition over (text, int) types defined with the following
values:

  FROM ('a', UNBOUNDED) TO ('b', UNBOUNDED)

which is equivalent to

  FROM ('a', -INFINITY) TO ('b', +INFINITY)

where should the next range start?

Even if you were to find a way to specify "the next string after 'b'",
it wouldn't exactly be pretty. The problem is that the above partition
corresponds to "all the strings starting with 'a', plus the string
'b', which is pretty ugly. A neater way to define the pair of ranges
in this case would be:

  FROM ('a', -INFINITY) TO ('b', -INFINITY)
  FROM ('b', -INFINITY) TO ('c', -INFINITY)

since then all strings starting with 'a' would fall into the first
partition and all the strings starting with 'b' would fall into the
second one.

Currently, when there are 2 partition columns, the partition
constraint is defined as

  (a is not null) and (b is not null)
  and
  (a > al or (a = al and b >= bl))
  and
  (a < au or (a = au and b < bu))

if the upper bound bu were allowed to be -INFINITY (something that
should probably be forbidden unless the previous column's upper bound
were finite), then this would simplify to

  (a is not null) and (b is not null)
  and
  (a > al or (a = al and b >= bl))
  and
  (a < au)

and in the example above, where al is -INFINITY, it would further simplify to

  (a is not null) and (b is not null)
  and
  (a >= al)
  and
  (a < au)

There would also be a similar simplification possible if the lower
bound of a partition column were allowed to be +INFINITY.

So, I think that having UNBOUNDED represent both -INFINITY and
+INFINITY depending on context is a design flaw, and that we need to
allow both -INFINITY and +INFINITY as upper and lower bounds (provided
they are preceded by a column with a finite bound). I think that, in
general, that's the only way to allow contiguous non-overlapping
partitions to be defined on multiple columns.

Regards,
Dean

  7. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-06-30T08:06:32Z

    On 2017/06/23 17:00, Dean Rasheed wrote:
> On 23 June 2017 at 08:01, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> The way we have designed our syntax, we don't have a way to tell that
>> p3 comes after p2 and they have no gap between those. But I don't
>> think that's your question. What you are struggling with is a way to
>> specify a lower bound (10, +infinity) so that anything with i1 > 10
>> would go to partition 3.
> 
> I think actually there is a fundamental problem here, which arises
> because UNBOUNDED has 2 different meanings depending on context, and
> thus it is not possible in general to specify the start of one range
> to be equal to the end of the previous range, as is necessary to get
> contiguous non-overlapping ranges.

Okay, I thought about this a bit more and I think I realize that this
arbitrary-sounding restriction of allowing only -infinity in FROM and
+infinity in TO limits the usefulness of the feature to specify infinite
bounds at all.

> Note that this isn't just a problem for floating point datatypes
> either, it also applies to other types such as strings. For example,
> given a partition over (text, int) types defined with the following
> values:
> 
>   FROM ('a', UNBOUNDED) TO ('b', UNBOUNDED)
> 
> which is equivalent to
> 
>   FROM ('a', -INFINITY) TO ('b', +INFINITY)
> 
> where should the next range start?
> 
> Even if you were to find a way to specify "the next string after 'b'",
> it wouldn't exactly be pretty. The problem is that the above partition
> corresponds to "all the strings starting with 'a', plus the string
> 'b', which is pretty ugly. A neater way to define the pair of ranges
> in this case would be:
> 
>   FROM ('a', -INFINITY) TO ('b', -INFINITY)
>   FROM ('b', -INFINITY) TO ('c', -INFINITY)
> 
> since then all strings starting with 'a' would fall into the first
> partition and all the strings starting with 'b' would fall into the
> second one.

I agree that a valid use case like the one above is awkward to express
currently.

> Currently, when there are 2 partition columns, the partition
> constraint is defined as
> 
>   (a is not null) and (b is not null)
>   and
>   (a > al or (a = al and b >= bl))
>   and
>   (a < au or (a = au and b < bu))
> 
> if the upper bound bu were allowed to be -INFINITY (something that
> should probably be forbidden unless the previous column's upper bound
> were finite), then this would simplify to
> 
>   (a is not null) and (b is not null)
>   and
>   (a > al or (a = al and b >= bl))
>   and
>   (a < au)
> 
> and in the example above, where al is -INFINITY, it would further simplify to
> 
>   (a is not null) and (b is not null)
>   and
>   (a >= al)
>   and
>   (a < au)
> 
> There would also be a similar simplification possible if the lower
> bound of a partition column were allowed to be +INFINITY.

Yep.

> So, I think that having UNBOUNDED represent both -INFINITY and
> +INFINITY depending on context is a design flaw, and that we need to
> allow both -INFINITY and +INFINITY as upper and lower bounds (provided
> they are preceded by a column with a finite bound). I think that, in
> general, that's the only way to allow contiguous non-overlapping
> partitions to be defined on multiple columns.

Alright, I spent some time implementing a patch to allow specifying
-infinity and +infinity in arbitrary ways.  Of course, it prevents
nonsensical inputs with appropriate error messages.

When implementing the same, I initially thought that the only grammar
modification required is to allow specifying a sign before the unbounded
keyword, but thought it sounded strange to call the actual bound values
-unbounded and +unbounded.  While the keyword "unbounded" describes the
property of being unbounded, actual values are really -infinity and
+infinity.  So, I decided to instead modify the grammar to accept
-infinity and +infinity in the FROM and TO lists.  The sign is optional
and in its absence, infinity in FROM means -infinity and vice versa.  This
decision may be seen as controversial, now that we are actually in beta,
if we decide to go with this patch at all.

Some adjustments were required in the logic in partition.c that depended
on the old assumption that all infinite values in the lower bound meant
-infinity and vice versa.  That includes get_qual_for_range() being able
to simplify the partition constraint as Dean mentioned in his email.

When testing the patch, I realized that the current code in
check_new_partition_bound() that checks for range partition overlap had a
latent bug that resulted in false positives for the new cases that the new
less restrictive syntax allowed.  I spent some time simplifying that code
while also fixing the aforementioned bug.  It's implemented in the
attached patch 0001.

0002 is the patch that implements the new syntax.

It's possible that this won't be considered a PG 10 open item but a new
feature and so PG 11 material, as Ashutosh also wondered.

Thanks,
Amit

  8. Re: Multi column range partition table

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-30T09:04:13Z

    On Fri, Jun 30, 2017 at 1:36 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> Alright, I spent some time implementing a patch to allow specifying
> -infinity and +infinity in arbitrary ways.  Of course, it prevents
> nonsensical inputs with appropriate error messages.

I don't think -infinity and +infinity are the right terms. For a
string or character data type there is no -infinity and +infinity.
Similarly for enums. We need to extend UNBOUNDED somehow to indicate
the end of a given type in the given direction. I thought about
UNBOUNDED LEFT/RIGHT but then whether LEFT indicates -ve side or +side
would cause confusion. Also LEFT/RIGHT may work for a single
dimensional datatype but not for multi-dimensional spaces. How about
MINIMUM/MAXIMUM or UNBOUNDED MIN/MAX to indicate the extremities.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  9. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-02T16:59:53Z

    On 30 June 2017 at 09:06, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> When testing the patch, I realized that the current code in
> check_new_partition_bound() that checks for range partition overlap had a
> latent bug that resulted in false positives for the new cases that the new
> less restrictive syntax allowed.  I spent some time simplifying that code
> while also fixing the aforementioned bug.  It's implemented in the
> attached patch 0001.
>

I haven't had time to look at 0002 yet, but looking at 0001, I'm not
convinced that this really represents much of a simplification, but I
do prefer the way it now consistently reports the first overlapping
partition in the error message.

I'm not entirely convinced by this change either:

-                        if (equal || off1 != off2)
+                        if (off2 > off1 + 1 || ((off2 == off1 + 1) && !equal))

Passing probe_is_bound = true to partition_bound_bsearch() will I
think cause it to return equal = false when the upper bound of one
partition equals the lower bound of another, so relying on the "equal"
flag here seems a bit dubious. I think I can just about convince
myself that it works, but not for the reasons stated in the comments.

It also seems unnecessary for this code to be doing 2 binary searches.
I think a better simplification would be to just do one binary search
to find the gap that the lower bound fits in, and then test the upper
bound of the new partition against the lower bound of the next
partition (if there is one), as in the attached patch.

Regards,
Dean

  10. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-02T17:15:15Z

    On 30 June 2017 at 10:04, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Fri, Jun 30, 2017 at 1:36 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>
>> Alright, I spent some time implementing a patch to allow specifying
>> -infinity and +infinity in arbitrary ways.  Of course, it prevents
>> nonsensical inputs with appropriate error messages.
>
> I don't think -infinity and +infinity are the right terms. For a
> string or character data type there is no -infinity and +infinity.
> Similarly for enums. We need to extend UNBOUNDED somehow to indicate
> the end of a given type in the given direction. I thought about
> UNBOUNDED LEFT/RIGHT but then whether LEFT indicates -ve side or +side
> would cause confusion. Also LEFT/RIGHT may work for a single
> dimensional datatype but not for multi-dimensional spaces. How about
> MINIMUM/MAXIMUM or UNBOUNDED MIN/MAX to indicate the extremities.
>

Yes, I think you're right. Also, some datatypes include values that
are equal to +/-infinity, which would then behave differently from
unbounded as range bounds, so it wouldn't be a good idea to overload
that term.

My first thought was UNBOUNDED ABOVE/BELOW, because that matches the
terminology already in use of upper and lower bounds.

Regards,
Dean

  11. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-07-03T02:32:15Z

    Hi Dean,

Thanks a lot for the review.

On 2017/07/03 1:59, Dean Rasheed wrote:
> On 30 June 2017 at 09:06, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> When testing the patch, I realized that the current code in
>> check_new_partition_bound() that checks for range partition overlap had a
>> latent bug that resulted in false positives for the new cases that the new
>> less restrictive syntax allowed.  I spent some time simplifying that code
>> while also fixing the aforementioned bug.  It's implemented in the
>> attached patch 0001.
>>
> 
> I haven't had time to look at 0002 yet, but looking at 0001, I'm not
> convinced that this really represents much of a simplification, but I
> do prefer the way it now consistently reports the first overlapping
> partition in the error message.
> 
> I'm not entirely convinced by this change either:
> 
> -                        if (equal || off1 != off2)
> +                        if (off2 > off1 + 1 || ((off2 == off1 + 1) && !equal))
> 
> Passing probe_is_bound = true to partition_bound_bsearch() will I
> think cause it to return equal = false when the upper bound of one
> partition equals the lower bound of another, so relying on the "equal"
> flag here seems a bit dubious. I think I can just about convince
> myself that it works, but not for the reasons stated in the comments.

You are right.  What's actually happening in the case where I was thinking
equal would be set to true is that off2 ends up being equal to off1, so
the second arm of that || is not checked at all.

> It also seems unnecessary for this code to be doing 2 binary searches.
> I think a better simplification would be to just do one binary search
> to find the gap that the lower bound fits in, and then test the upper
> bound of the new partition against the lower bound of the next
> partition (if there is one), as in the attached patch.

I agree.  The patch looks good to me.

Thanks again.

Regards,
Amit

  12. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-07-03T05:00:11Z

    On 2017/07/03 2:15, Dean Rasheed wrote:
> On 30 June 2017 at 10:04, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> On Fri, Jun 30, 2017 at 1:36 PM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>
>>> Alright, I spent some time implementing a patch to allow specifying
>>> -infinity and +infinity in arbitrary ways.  Of course, it prevents
>>> nonsensical inputs with appropriate error messages.
>>
>> I don't think -infinity and +infinity are the right terms. For a
>> string or character data type there is no -infinity and +infinity.
>> Similarly for enums. We need to extend UNBOUNDED somehow to indicate
>> the end of a given type in the given direction. I thought about
>> UNBOUNDED LEFT/RIGHT but then whether LEFT indicates -ve side or +side
>> would cause confusion. Also LEFT/RIGHT may work for a single
>> dimensional datatype but not for multi-dimensional spaces. How about
>> MINIMUM/MAXIMUM or UNBOUNDED MIN/MAX to indicate the extremities.
>>
> 
> Yes, I think you're right. Also, some datatypes include values that
> are equal to +/-infinity, which would then behave differently from
> unbounded as range bounds, so it wouldn't be a good idea to overload
> that term.

Agree with you both that using (+/-) infinity may not be a good idea after
all.

> My first thought was UNBOUNDED ABOVE/BELOW, because that matches the
> terminology already in use of upper and lower bounds.

I was starting to like the Ashutosh's suggested UNBOUNDED MIN/MAX syntax,
but could you clarify your comment that ABOVE/BELOW is the terminology
already in use of upper and lower bounds?  I couldn't find ABOVE/BELOW in
our existing syntax anywhere that uses the upper/lower bound notion, so
was confused a little bit.

Also, I assume UNBOUNDED ABOVE signifies positive infinity and vice versa.

Thanks,
Amit

  13. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-07-03T06:10:14Z

    On 2017/07/03 14:00, Amit Langote wrote:
> On 2017/07/03 2:15, Dean Rasheed wrote:
>> On 30 June 2017 at 10:04, Ashutosh Bapat
>> <ashutosh.bapat@enterprisedb.com> wrote:
>>> On Fri, Jun 30, 2017 at 1:36 PM, Amit Langote
>>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>>
>>>> Alright, I spent some time implementing a patch to allow specifying
>>>> -infinity and +infinity in arbitrary ways.  Of course, it prevents
>>>> nonsensical inputs with appropriate error messages.
>>>
>>> I don't think -infinity and +infinity are the right terms. For a
>>> string or character data type there is no -infinity and +infinity.
>>> Similarly for enums. We need to extend UNBOUNDED somehow to indicate
>>> the end of a given type in the given direction. I thought about
>>> UNBOUNDED LEFT/RIGHT but then whether LEFT indicates -ve side or +side
>>> would cause confusion. Also LEFT/RIGHT may work for a single
>>> dimensional datatype but not for multi-dimensional spaces. How about
>>> MINIMUM/MAXIMUM or UNBOUNDED MIN/MAX to indicate the extremities.
>>>
>>
>> Yes, I think you're right. Also, some datatypes include values that
>> are equal to +/-infinity, which would then behave differently from
>> unbounded as range bounds, so it wouldn't be a good idea to overload
>> that term.
> 
> Agree with you both that using (+/-) infinity may not be a good idea after
> all.
> 
>> My first thought was UNBOUNDED ABOVE/BELOW, because that matches the
>> terminology already in use of upper and lower bounds.
> 
> I was starting to like the Ashutosh's suggested UNBOUNDED MIN/MAX syntax,
> but could you clarify your comment that ABOVE/BELOW is the terminology
> already in use of upper and lower bounds?  I couldn't find ABOVE/BELOW in
> our existing syntax anywhere that uses the upper/lower bound notion, so
> was confused a little bit.
> 
> Also, I assume UNBOUNDED ABOVE signifies positive infinity and vice versa.

Anyway, here's the revised version of the syntax patch that implements
ABOVE/BELOW extension to UNBOUNDED specification.

0001 is the patch that Dean posted [1] as a replacement for what I earlier
posted for simplifying range partition overlap check.

0002 is the UNBOUNDED syntax extension patch.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAEZATCVcBCBZsMcHj37TF%2BdcsjCtKZdZ_FAaJjaFMvfoXRqZMg%40mail.gmail.com

  14. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-03T08:36:19Z

    On 3 July 2017 at 06:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/07/03 2:15, Dean Rasheed wrote:
>> My first thought was UNBOUNDED ABOVE/BELOW, because that matches the
>> terminology already in use of upper and lower bounds.
>
> I was starting to like the Ashutosh's suggested UNBOUNDED MIN/MAX syntax,
> but could you clarify your comment that ABOVE/BELOW is the terminology
> already in use of upper and lower bounds?  I couldn't find ABOVE/BELOW in
> our existing syntax anywhere that uses the upper/lower bound notion, so
> was confused a little bit.
>

I just meant that the words "above" and "below" more closely match the
already-used terms "upper" and "lower" for the bounds, so that
terminology seemed more consistent, e.g. "UNBOUNDED ABOVE" => no upper
bound.


> Also, I assume UNBOUNDED ABOVE signifies positive infinity and vice versa.
>

Right.

I'm not particularly wedded to that terminology. I always find naming
things hard, so if anyone can think of anything better, let's hear it.

The bigger question is do we want this for PG10? If so, time is
getting tight. My feeling is that we do, because otherwise we'd be
changing the syntax in PG11 of a feature only just released in PG10,
and I think the current syntax is flawed, so it would be better not to
have it in any public release. I'd feel better hearing from the
original committer though.

Meanwhile, I'll continue trying to review the latest patches...

Regards,
Dean

  15. Re: Multi column range partition table

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-07-03T08:43:55Z

    On Mon, Jul 3, 2017 at 2:06 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 3 July 2017 at 06:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/07/03 2:15, Dean Rasheed wrote:
>>> My first thought was UNBOUNDED ABOVE/BELOW, because that matches the
>>> terminology already in use of upper and lower bounds.
>>
>> I was starting to like the Ashutosh's suggested UNBOUNDED MIN/MAX syntax,
>> but could you clarify your comment that ABOVE/BELOW is the terminology
>> already in use of upper and lower bounds?  I couldn't find ABOVE/BELOW in
>> our existing syntax anywhere that uses the upper/lower bound notion, so
>> was confused a little bit.
>>
>
> I just meant that the words "above" and "below" more closely match the
> already-used terms "upper" and "lower" for the bounds, so that
> terminology seemed more consistent, e.g. "UNBOUNDED ABOVE" => no upper
> bound.
>
>
>> Also, I assume UNBOUNDED ABOVE signifies positive infinity and vice versa.
>>
>
> Right.
>
> I'm not particularly wedded to that terminology. I always find naming
> things hard, so if anyone can think of anything better, let's hear it.

Yet another option: UNBOUNDED UPPER/LOWER.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  16. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-07-03T09:32:24Z

    Hi Dean,

On 2017/07/03 17:36, Dean Rasheed wrote:
> On 3 July 2017 at 06:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/07/03 2:15, Dean Rasheed wrote:
>>> My first thought was UNBOUNDED ABOVE/BELOW, because that matches the
>>> terminology already in use of upper and lower bounds.
>>
>> I was starting to like the Ashutosh's suggested UNBOUNDED MIN/MAX syntax,
>> but could you clarify your comment that ABOVE/BELOW is the terminology
>> already in use of upper and lower bounds?  I couldn't find ABOVE/BELOW in
>> our existing syntax anywhere that uses the upper/lower bound notion, so
>> was confused a little bit.
>>
> 
> I just meant that the words "above" and "below" more closely match the
> already-used terms "upper" and "lower" for the bounds, so that
> terminology seemed more consistent, e.g. "UNBOUNDED ABOVE" => no upper
> bound.
>
>> Also, I assume UNBOUNDED ABOVE signifies positive infinity and vice versa.
>>
> 
> Right.

I see, thanks for clarifying.

> I'm not particularly wedded to that terminology. I always find naming
> things hard, so if anyone can think of anything better, let's hear it.
> 
> The bigger question is do we want this for PG10? If so, time is
> getting tight. My feeling is that we do, because otherwise we'd be
> changing the syntax in PG11 of a feature only just released in PG10,
> and I think the current syntax is flawed, so it would be better not to
> have it in any public release. I'd feel better hearing from the
> original committer though.

The way I have extended the syntax in the posted patch, ABOVE/BELOW (or
whatever we decide instead) are optional.  UNBOUNDED without the
ABOVE/BELOW specifications implicitly means UNBOUNDED ABOVE if in FROM and
vice versa, which seems to me like sensible default behavior and what's
already present in PG 10.

Do you think ABOVE/BELOW shouldn't really be optional?

> Meanwhile, I'll continue trying to review the latest patches...

I had forgotten to update the CREATE TABLE documentation in 0002 to
reflect the syntax extension.  Fixed in the attached latest patch.

Thanks,
Amit

  17. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-04T07:49:05Z

    On 3 July 2017 at 10:32, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/07/03 17:36, Dean Rasheed wrote:
>> The bigger question is do we want this for PG10? If so, time is
>> getting tight. My feeling is that we do, because otherwise we'd be
>> changing the syntax in PG11 of a feature only just released in PG10,
>> and I think the current syntax is flawed, so it would be better not to
>> have it in any public release. I'd feel better hearing from the
>> original committer though.
>
> The way I have extended the syntax in the posted patch, ABOVE/BELOW (or
> whatever we decide instead) are optional.  UNBOUNDED without the
> ABOVE/BELOW specifications implicitly means UNBOUNDED ABOVE if in FROM and
> vice versa, which seems to me like sensible default behavior and what's
> already present in PG 10.
>
> Do you think ABOVE/BELOW shouldn't really be optional?
>

Hmm, I'm not so sure about that.

The more I think about this, the more I think that the current design
is broken, and that introducing UNBOUNDED ABOVE/BELOW is just a
sticking plaster to cover that up. Yes, it allows nicer multi-column
ranges to be defined, as demonstrated upthread. But, it also allows
some pretty counterintuitive things like making the lower bound
exclusive and the upper bound inclusive.

I think that's actually the real problem with the current design. If I
have a single-column partition like

  (col) FROM (x) TO (y)

it's pretty clear that's a simple range, inclusive at the lower end
and exclusive at the upper end:

  (x) <= (col) < (y)

If I now make that a 2-column partition, but leave the second column
unbounded:

  (col1,col2) FROM (x,UNBOUNDED) TO (y,UNBOUNDED)

my initial expectation would have been for that to mean the same
thing, i.e.,

  (x) <= (col1) < (y)

but that only happens if "UNBOUNDED" means negative infinity in both
places. That then starts to give the sort of desirable properties
you'd expect, like using the same expression for the lower bound of
one partition as the upper bound of another makes the two partitions
contiguous.

But of course, that's not exactly a pretty design either, because then
you'd be saying that UNBOUNDED means positive infinity if it's the
upper bound of the first column, and negative infinity if it's the
lower bound of the first column or either bound of any other column.

Another aspect of the current design I don't like is that you have to
keep repeating UNBOUNDED [ABOVE/BELOW], for each of the rest of the
columns in the bound, and anything else is an error. That's a pretty
verbose way of saying "the rest of the columns are unbounded".

So the more I think about this, the more I think that a cleaner design
would be as follows:

1). Don't allow UNBOUNDED, except in the first column, where it can
    keep it's current meaning.

2). Allow the partition bounds to have fewer columns than the
    partition definition, and have that mean the same as it would have
    meant if you were partitioning by that many columns. So, for
    example, if you were partitioning by (col1,col2), you'd be allowed
    to define a partition like so:

      FROM (x) TO (y)

    and it would mean

      x <= col1 < y

    Or you'd be able to define a partition like

      FROM (x1,x2) TO (y)

    which would mean

      (col1 > x1) OR (col1 = x1 AND col2 >= x2) AND col1 < y

3). Don't allow any value after UNBOUNDED (i.e., only specify
    UNBOUNDED once in a partition bound).


This design has a few neat properties:

- Lower bounds are always inclusive and upper bounds are always
  exclusive.

- If the expression for the lower bound of one partition is the same
  as the expression for the upper bound of another, the 2 partitions
  are contiguous, making it easy to define a covering set of
  partitions.

- It's much easier to understand what a bound of "(x)" means than
  "(x,UNBOUNDED [ABOVE/BELOW])"

- It's much less verbose, and there's no needless repetition.


Of course, it's pretty late in the day to be proposing this kind of
redesign, but I fear that if we don't tackle it now, it will just be
harder to deal with in the future.

Actually, a quick, simple hacky implementation might be to just fill
in any omitted values in a partition bound with negative infinity
internally, and when printing a bound, omit any values after an
infinite value. But really, I think we'd want to tidy up the
implementation, and I think a number of things would actually get much
simpler. For example, get_qual_for_range() could simply stop when it
reached the end of the list of values for the bound, and it wouldn't
need to worry about an unbounded value following a bounded one.

Thoughts?

Regards,
Dean

  18. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-07-05T09:43:13Z

    Hi Dean,

On 2017/07/04 16:49, Dean Rasheed wrote:
> On 3 July 2017 at 10:32, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/07/03 17:36, Dean Rasheed wrote:
>>> The bigger question is do we want this for PG10? If so, time is
>>> getting tight. My feeling is that we do, because otherwise we'd be
>>> changing the syntax in PG11 of a feature only just released in PG10,
>>> and I think the current syntax is flawed, so it would be better not to
>>> have it in any public release. I'd feel better hearing from the
>>> original committer though.
>>
>> The way I have extended the syntax in the posted patch, ABOVE/BELOW (or
>> whatever we decide instead) are optional.  UNBOUNDED without the
>> ABOVE/BELOW specifications implicitly means UNBOUNDED ABOVE if in FROM and
>> vice versa, which seems to me like sensible default behavior and what's
>> already present in PG 10.
>>
>> Do you think ABOVE/BELOW shouldn't really be optional?
>>
> 
> Hmm, I'm not so sure about that.
> 
> The more I think about this, the more I think that the current design
> is broken, and that introducing UNBOUNDED ABOVE/BELOW is just a
> sticking plaster to cover that up. Yes, it allows nicer multi-column
> ranges to be defined, as demonstrated upthread. But, it also allows
> some pretty counterintuitive things like making the lower bound
> exclusive and the upper bound inclusive.

Yes, I kind of got that impression from the example, but wasn't able to
reach the same conclusion as yours that it stems from the underlying
design issues; I thought we'd just have to document them as caveats, but
that doesn't really sound nice.  Thanks for pointing that out.

> I think that's actually the real problem with the current design. If I
> have a single-column partition like
> 
>   (col) FROM (x) TO (y)
> 
> it's pretty clear that's a simple range, inclusive at the lower end
> and exclusive at the upper end:
> 
>   (x) <= (col) < (y)
> 
> If I now make that a 2-column partition, but leave the second column
> unbounded:
> 
>   (col1,col2) FROM (x,UNBOUNDED) TO (y,UNBOUNDED)
> 
> my initial expectation would have been for that to mean the same
> thing, i.e.,
> 
>   (x) <= (col1) < (y)
> 
> but that only happens if "UNBOUNDED" means negative infinity in both
> places. That then starts to give the sort of desirable properties
> you'd expect, like using the same expression for the lower bound of
> one partition as the upper bound of another makes the two partitions
> contiguous.
> 
> But of course, that's not exactly a pretty design either, because then
> you'd be saying that UNBOUNDED means positive infinity if it's the
> upper bound of the first column, and negative infinity if it's the
> lower bound of the first column or either bound of any other column.

Initially, I didn't understand the part where you said FROM (x, UNBOUNDED)
TO (y, UNBOUNDED) would mean the same thing as (x) <= (col1) < (y),
because row comparison logic that underlying multi-column range partition
key comparisons appears to me to contradict the same.  But, maybe it's
thinking about the implementation details like this that's clouding my
judgement about the correctness or the intuitiveness of the current design.

> Another aspect of the current design I don't like is that you have to
> keep repeating UNBOUNDED [ABOVE/BELOW], for each of the rest of the
> columns in the bound, and anything else is an error. That's a pretty
> verbose way of saying "the rest of the columns are unbounded".
>
> So the more I think about this, the more I think that a cleaner design
> would be as follows:
> 
> 1). Don't allow UNBOUNDED, except in the first column, where it can
>     keep it's current meaning.
> 
> 2). Allow the partition bounds to have fewer columns than the
>     partition definition, and have that mean the same as it would have
>     meant if you were partitioning by that many columns. So, for
>     example, if you were partitioning by (col1,col2), you'd be allowed
>     to define a partition like so:
> 
>       FROM (x) TO (y)
> 
>     and it would mean
> 
>       x <= col1 < y
> 
>     Or you'd be able to define a partition like
> 
>       FROM (x1,x2) TO (y)
> 
>     which would mean
> 
>       (col1 > x1) OR (col1 = x1 AND col2 >= x2) AND col1 < y
> 
> 3). Don't allow any value after UNBOUNDED (i.e., only specify
>     UNBOUNDED once in a partition bound).

I assume we don't need the ability of specifying ABOVE/BELOW in this design.

In retrospect, that sounds like something that was implemented in the
earlier versions of the patch, whereby there was no ability to specify
UNBOUNDED on a per-column basis.  So the syntax was:

FROM { (x [, ...]) | UNBOUNDED } TO { (y [, ...]) | UNBOUNDED }

But, it was pointed out to me [1] that that doesn't address the use case,
for example, where part1 goes up to (10, 10) and part2 goes from (10, 10)
up to (10, unbounded).

The new design will limit the usage of unbounded range partitions at the
tail ends.

> This design has a few neat properties:
> 
> - Lower bounds are always inclusive and upper bounds are always
>   exclusive.
> 
> - If the expression for the lower bound of one partition is the same
>   as the expression for the upper bound of another, the 2 partitions
>   are contiguous, making it easy to define a covering set of
>   partitions.
> 
> - It's much easier to understand what a bound of "(x)" means than
>   "(x,UNBOUNDED [ABOVE/BELOW])"
> 
> - It's much less verbose, and there's no needless repetition.

They all sound good to me.

> Of course, it's pretty late in the day to be proposing this kind of
> redesign, but I fear that if we don't tackle it now, it will just be
> harder to deal with in the future.
> 
> Actually, a quick, simple hacky implementation might be to just fill
> in any omitted values in a partition bound with negative infinity
> internally, and when printing a bound, omit any values after an
> infinite value. But really, I think we'd want to tidy up the
> implementation, and I think a number of things would actually get much
> simpler. For example, get_qual_for_range() could simply stop when it
> reached the end of the list of values for the bound, and it wouldn't
> need to worry about an unbounded value following a bounded one.
> 
> Thoughts?

I cooked up a patch for the "hacky" implementation for now, just as you
described in the above paragraph.  Will you be willing to give it a look?
I will also think about the non-hacky way of implementing this.

0001 is your patch to tidy up check_new_partition_bound()  (must be
applied before 0002)

0002 is the patch to implement the range partition syntax redesign that
you outlined above

Thanks again.

Regards,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoYJcUTcN7vVgg54GHtffH11JJWYZnfF4KiRxjV-iaACQg%40mail.gmail.com

  19. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-05T14:18:49Z

    On 5 July 2017 at 10:43, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> So the more I think about this, the more I think that a cleaner design
>> would be as follows:
>>
>> 1). Don't allow UNBOUNDED, except in the first column, where it can
>>     keep it's current meaning.
>>
>> 2). Allow the partition bounds to have fewer columns than the
>>     partition definition, and have that mean the same as it would have
>>     meant if you were partitioning by that many columns. So, for
>>     example, if you were partitioning by (col1,col2), you'd be allowed
>>     to define a partition like so:
>>
>>       FROM (x) TO (y)
>>
>>     and it would mean
>>
>>       x <= col1 < y
>>
>>     Or you'd be able to define a partition like
>>
>>       FROM (x1,x2) TO (y)
>>
>>     which would mean
>>
>>       (col1 > x1) OR (col1 = x1 AND col2 >= x2) AND col1 < y
>>
>> 3). Don't allow any value after UNBOUNDED (i.e., only specify
>>     UNBOUNDED once in a partition bound).
>
> I assume we don't need the ability of specifying ABOVE/BELOW in this design.
>

Yes that's right.


> In retrospect, that sounds like something that was implemented in the
> earlier versions of the patch, whereby there was no ability to specify
> UNBOUNDED on a per-column basis.  So the syntax was:
>
> FROM { (x [, ...]) | UNBOUNDED } TO { (y [, ...]) | UNBOUNDED }
>

Yes, that's where I ended up too.


> But, it was pointed out to me [1] that that doesn't address the use case,
> for example, where part1 goes up to (10, 10) and part2 goes from (10, 10)
> up to (10, unbounded).
>
> The new design will limit the usage of unbounded range partitions at the
> tail ends.
>

True, but I don't think that's really a problem. When the first column
is a discrete type, an upper bound of (10, unbounded) can be rewritten
as (11) in the new design. When it's a continuous type, e.g. floating
point, it can no longer be represented, because (10.0, unbounded)
really means (col1 <= 10.0). But we've already decided not to support
anything other than inclusive lower bounds and exclusive upper bounds,
so allowing this upper bound goes against that design choice.


>> Of course, it's pretty late in the day to be proposing this kind of
>> redesign, but I fear that if we don't tackle it now, it will just be
>> harder to deal with in the future.
>>
>> Actually, a quick, simple hacky implementation might be to just fill
>> in any omitted values in a partition bound with negative infinity
>> internally, and when printing a bound, omit any values after an
>> infinite value. But really, I think we'd want to tidy up the
>> implementation, and I think a number of things would actually get much
>> simpler. For example, get_qual_for_range() could simply stop when it
>> reached the end of the list of values for the bound, and it wouldn't
>> need to worry about an unbounded value following a bounded one.
>>
>> Thoughts?
>
> I cooked up a patch for the "hacky" implementation for now, just as you
> described in the above paragraph.  Will you be willing to give it a look?
> I will also think about the non-hacky way of implementing this.
>

OK, I'll take a look.

Meanwhile, I already had a go at the "non-hacky" implementation (WIP
patch attached). The more I worked on it, the simpler things got,
which I think is a good sign.

Part-way through, I realised that the PartitionRangeDatum Node type is
no longer needed, because each bound value is now necessarily finite,
so the lowerdatums and upperdatums lists in a PartitionBoundSpec can
now be made into lists of Const nodes, making them match the
listdatums field used for LIST partitioning, and then a whole lot of
related code gets simplified.

It needed a little bit more code in partition.c to track individual
bound sizes, but there were a number of other places that could be
simplified, so overall this represents a reduction in the code size
and complexity.

It's not complete (e.g., no doc updates yet), but it passes all the
tests, and so far seems to work as I would expect.

Regards,
Dean

  20. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-05T17:07:37Z

    On 5 July 2017 at 10:43, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> In retrospect, that sounds like something that was implemented in the
> earlier versions of the patch, whereby there was no ability to specify
> UNBOUNDED on a per-column basis.  So the syntax was:
>
> FROM { (x [, ...]) | UNBOUNDED } TO { (y [, ...]) | UNBOUNDED }
>
> But, it was pointed out to me [1] that that doesn't address the use case,
> for example, where part1 goes up to (10, 10) and part2 goes from (10, 10)
> up to (10, unbounded).
>

[Reading that other thread]

It's a reasonable point that our syntax is quite different from
Oracle's, and doing this takes it even further away, and removes
support for things that they do support.

For the record, Oracle allows things like the following:

DROP TABLE t1;
CREATE TABLE t1 (a NUMBER, b NUMBER, c NUMBER)
PARTITION BY RANGE (a,b,c)
  (PARTITION t1p1 VALUES LESS THAN (1,2,3),
   PARTITION t1p2 VALUES LESS THAN (2,3,4),
   PARTITION t1p3 VALUES LESS THAN (3,MAXVALUE,5),
   PARTITION t1p4 VALUES LESS THAN (4,MAXVALUE,6)
  );

INSERT INTO t1 VALUES(1,2,3);
INSERT INTO t1 VALUES(2,3,4);
INSERT INTO t1 VALUES(3,4,5);
INSERT INTO t1 VALUES(3.01,4,5);
INSERT INTO t1 VALUES(4,5,10);

COLUMN subobject_name FORMAT a20;
SELECT a, b, c, subobject_name
  FROM t1, user_objects o
 WHERE o.data_object_id = dbms_rowid.rowid_object(t1.ROWID)
 ORDER BY a,b,c;

         A          B          C SUBOBJECT_NAME
---------- ---------- ---------- --------------------
         1          2          3 T1P2
         2          3          4 T1P3
         3          4          5 T1P3
      3.01          4          5 T1P4
         4          5         10 T1P4


So they use MAXVALUE instead of UNBOUNDED for an upper bound, which is
more explicit. They don't have an equivalent MINVALUE, but it's
arguably not necessary, since the first partition's lower bound is
implicitly unbounded.

With this syntax they don't need to worry about gaps or overlaps
between partitions, which is nice, but arguably less flexible.

They're also more lax about allowing finite values after MAXVALUE, and
they document the fact that any value after a MAXVALUE is ignored.

I don't think their scheme provides any way to define a partition of
the above table that would hold all rows for which a < some value.

So if we were to go for maximum flexibility and compatibility with
Oracle, then perhaps what we would do is more like the original idea
of UNBOUNDED ABOVE/BELOW, except call them MINVALUE and MAXVALUE,
which conveniently are already unreserved keywords, as well as being
much shorter. Plus, we would also relax the constraint about having
finite values after MINVALUE/MAXVALUE.

I think I'll go play around with that idea to see what it looks like
in practice. Your previous patch already does much of that, and is far
less invasive.

Regards,
Dean

  21. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-07-06T02:14:47Z

    Hi Dean,

On 2017/07/05 23:18, Dean Rasheed wrote:
> On 5 July 2017 at 10:43, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> In retrospect, that sounds like something that was implemented in the
>> earlier versions of the patch, whereby there was no ability to specify
>> UNBOUNDED on a per-column basis.  So the syntax was:
>>
>> FROM { (x [, ...]) | UNBOUNDED } TO { (y [, ...]) | UNBOUNDED }
>>
> Yes, that's where I ended up too.

I see.

>> But, it was pointed out to me [1] that that doesn't address the use case,
>> for example, where part1 goes up to (10, 10) and part2 goes from (10, 10)
>> up to (10, unbounded).
>>
>> The new design will limit the usage of unbounded range partitions at the
>> tail ends.
> 
> True, but I don't think that's really a problem. When the first column
> is a discrete type, an upper bound of (10, unbounded) can be rewritten
> as (11) in the new design. When it's a continuous type, e.g. floating
> point, it can no longer be represented, because (10.0, unbounded)
> really means (col1 <= 10.0). But we've already decided not to support
> anything other than inclusive lower bounds and exclusive upper bounds,
> so allowing this upper bound goes against that design choice.

Yes.

>>> Of course, it's pretty late in the day to be proposing this kind of
>>> redesign, but I fear that if we don't tackle it now, it will just be
>>> harder to deal with in the future.
>>>
>>> Actually, a quick, simple hacky implementation might be to just fill
>>> in any omitted values in a partition bound with negative infinity
>>> internally, and when printing a bound, omit any values after an
>>> infinite value. But really, I think we'd want to tidy up the
>>> implementation, and I think a number of things would actually get much
>>> simpler. For example, get_qual_for_range() could simply stop when it
>>> reached the end of the list of values for the bound, and it wouldn't
>>> need to worry about an unbounded value following a bounded one.
>>>
>>> Thoughts?
>>
>> I cooked up a patch for the "hacky" implementation for now, just as you
>> described in the above paragraph.  Will you be willing to give it a look?
>> I will also think about the non-hacky way of implementing this.
> 
> OK, I'll take a look.

Thanks a lot for your time.

> Meanwhile, I already had a go at the "non-hacky" implementation (WIP
> patch attached). The more I worked on it, the simpler things got,
> which I think is a good sign.

It definitely looks good to me.  I was thinking of more or less the same
approach, but couldn't have done as clean a job as you've done with your
patch.

> Part-way through, I realised that the PartitionRangeDatum Node type is
> no longer needed, because each bound value is now necessarily finite,
> so the lowerdatums and upperdatums lists in a PartitionBoundSpec can
> now be made into lists of Const nodes, making them match the
> listdatums field used for LIST partitioning, and then a whole lot of
> related code gets simplified.

Yeah, seems that way.

> It needed a little bit more code in partition.c to track individual
> bound sizes, but there were a number of other places that could be
> simplified, so overall this represents a reduction in the code size
> and complexity.

Sounds reasonable.

> It's not complete (e.g., no doc updates yet), but it passes all the
> tests, and so far seems to work as I would expect.

Thanks a lot for working on it.

Regards,
Amit

  22. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-06T09:30:21Z

    On 5 July 2017 at 10:43, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> 0001 is your patch to tidy up check_new_partition_bound()  (must be
> applied before 0002)
>

I pushed this first patch, simplifying check_new_partition_bound() for
range partitions, since it seemed like a good simplification, but note
that I don't think that was actually the cause of the latent bug you
saw upthread.

I think the real issue was in partition_rbound_cmp() -- normally, if
the upper bound of one partition coincides with the lower bound of
another, that function would report the upper bound as the smaller
one, but that logic breaks if any of the bound values are infinite,
since then it will exit early, returning 0, without ever comparing the
"lower" flags on the bounds.

I'm tempted to push a fix for that independently, since it's a bug
waiting to happen, even though it's not possible to hit it currently.

Regards,
Dean

  23. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-06T19:55:15Z

    On 5 July 2017 at 18:07, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> So if we were to go for maximum flexibility and compatibility with
> Oracle, then perhaps what we would do is more like the original idea
> of UNBOUNDED ABOVE/BELOW, except call them MINVALUE and MAXVALUE,
> which conveniently are already unreserved keywords, as well as being
> much shorter. Plus, we would also relax the constraint about having
> finite values after MINVALUE/MAXVALUE.
>

So I know that I have flip-flopped a few times on this now, but I'm
now starting to think that this approach, replacing UNBOUNDED with
MINVALUE and MAXVALUE is the best way to go, along with permitting
finite values after MINVALUE/MAXVALUE.

This gives the greatest flexibility, it's not too verbose, and it
makes it easy to define contiguous sets of partitions just by making
the lower bound of one match the upper bound of another.

With this approach, any partition bounds that Oracle allows are also
valid in PostgreSQL, not that I would normally give too much weight to
that, but it is I think quite a nice syntax. Of course, we also
support things that Oracle doesn't allow, such as MINVALUE and gaps
between partitions.

Parts of the patch are similar to your UNBOUNDED ABOVE/BELOW patch,
but there are a number of differences -- most notably, I replaced the
"infinite" boolean flag on PartitionRangeDatum with a 3-value enum and
did away with all the DefElem nodes and the associated special string
constants being copied and compared.


However, this is also an incompatible syntax change, and any attempt
to support both the old and new syntaxes is likely to be messy, so we
really need to get consensus on whether this is the right thing to do,
and whether it *can* be done now for PG10.

Regards,
Dean

  24. Re: Multi column range partition table

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-07-06T20:04:46Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> However, this is also an incompatible syntax change, and any attempt
> to support both the old and new syntaxes is likely to be messy, so we
> really need to get consensus on whether this is the right thing to do,
> and whether it *can* be done now for PG10.

FWIW, I'd much rather see us get it right the first time than release
PG10 with a syntax that we'll regret later.  I do not think that beta2,
or even beta3, is too late for such a change.

I'm not taking a position on whether this proposal is actually better
than what we have.  But if there's a consensus that it is, we should
go ahead and do it, not worry that it's too late.

			regards, tom lane

  25. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-06T20:24:07Z

    On 6 July 2017 at 21:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> However, this is also an incompatible syntax change, and any attempt
>> to support both the old and new syntaxes is likely to be messy, so we
>> really need to get consensus on whether this is the right thing to do,
>> and whether it *can* be done now for PG10.
>
> FWIW, I'd much rather see us get it right the first time than release
> PG10 with a syntax that we'll regret later.  I do not think that beta2,
> or even beta3, is too late for such a change.
>
> I'm not taking a position on whether this proposal is actually better
> than what we have.  But if there's a consensus that it is, we should
> go ahead and do it, not worry that it's too late.
>

OK, thanks. That's good to know.

Regards,
Dean

  26. Re: Multi column range partition table

    Joe Conway <mail@joeconway.com> — 2017-07-06T21:43:44Z

    On 07/06/2017 01:24 PM, Dean Rasheed wrote:
> On 6 July 2017 at 21:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>>> However, this is also an incompatible syntax change, and any attempt
>>> to support both the old and new syntaxes is likely to be messy, so we
>>> really need to get consensus on whether this is the right thing to do,
>>> and whether it *can* be done now for PG10.
>>
>> FWIW, I'd much rather see us get it right the first time than release
>> PG10 with a syntax that we'll regret later.  I do not think that beta2,
>> or even beta3, is too late for such a change.
>>
>> I'm not taking a position on whether this proposal is actually better
>> than what we have.  But if there's a consensus that it is, we should
>> go ahead and do it, not worry that it's too late.
>>
> 
> OK, thanks. That's good to know.

I agree we should get this right the first time and I also agree with
Dean's proposal, so I guess I'm a +2

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  27. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-07-07T01:06:18Z

    On 2017/07/06 18:30, Dean Rasheed wrote:
> On 5 July 2017 at 10:43, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> 0001 is your patch to tidy up check_new_partition_bound()  (must be
>> applied before 0002)
>>
> 
> I pushed this first patch, simplifying check_new_partition_bound() for
> range partitions, since it seemed like a good simplification, but note
> that I don't think that was actually the cause of the latent bug you
> saw upthread.

I like how simple check_new_partition_bound() has now become.

> I think the real issue was in partition_rbound_cmp() -- normally, if
> the upper bound of one partition coincides with the lower bound of
> another, that function would report the upper bound as the smaller
> one, but that logic breaks if any of the bound values are infinite,
> since then it will exit early, returning 0, without ever comparing the
> "lower" flags on the bounds.
> 
> I'm tempted to push a fix for that independently, since it's a bug
> waiting to happen, even though it's not possible to hit it currently.

Oops, you're right.  Thanks for the fix.

Regards,
Amit

  28. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-07-07T02:21:28Z

    On 2017/07/07 4:55, Dean Rasheed wrote:
> On 5 July 2017 at 18:07, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> So if we were to go for maximum flexibility and compatibility with
>> Oracle, then perhaps what we would do is more like the original idea
>> of UNBOUNDED ABOVE/BELOW, except call them MINVALUE and MAXVALUE,
>> which conveniently are already unreserved keywords, as well as being
>> much shorter. Plus, we would also relax the constraint about having
>> finite values after MINVALUE/MAXVALUE.
>>
> So I know that I have flip-flopped a few times on this now, but I'm
> now starting to think that this approach, replacing UNBOUNDED with
> MINVALUE and MAXVALUE is the best way to go, along with permitting
> finite values after MINVALUE/MAXVALUE.

Sure.

> This gives the greatest flexibility, it's not too verbose, and it
> makes it easy to define contiguous sets of partitions just by making
> the lower bound of one match the upper bound of another.
> 
> With this approach, any partition bounds that Oracle allows are also
> valid in PostgreSQL, not that I would normally give too much weight to
> that, but it is I think quite a nice syntax. Of course, we also
> support things that Oracle doesn't allow, such as MINVALUE and gaps
> between partitions.

Agreed.  MINVALUE/MAXVALUE seems like a good way forward.

> Parts of the patch are similar to your UNBOUNDED ABOVE/BELOW patch,
> but there are a number of differences -- most notably, I replaced the
> "infinite" boolean flag on PartitionRangeDatum with a 3-value enum and
> did away with all the DefElem nodes and the associated special string
> constants being copied and compared.

That's better.

> However, this is also an incompatible syntax change, and any attempt
> to support both the old and new syntaxes is likely to be messy, so we
> really need to get consensus on whether this is the right thing to do,
> and whether it *can* be done now for PG10.

+1 to releasing this syntax in PG 10.

The patch looks generally good, although I found and fixed some minor
issues (typos and such).  Please find attached the updated patch.

Thanks,
Amit

  29. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-07T09:30:46Z

    On 7 July 2017 at 03:21, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> The patch looks generally good, although I found and fixed some minor
> issues (typos and such).  Please find attached the updated patch.
>

Thanks for the review. Those changes all look good. I also see that I
missed an example in the docs at the bottom of the CREATE TABLE page,
so I'll go update that.

Regards,
Dean

  30. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-09T07:42:32Z

    On 6 July 2017 at 22:43, Joe Conway <mail@joeconway.com> wrote:
> I agree we should get this right the first time and I also agree with
> Dean's proposal, so I guess I'm a +2
>

On 7 July 2017 at 03:21, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> +1 to releasing this syntax in PG 10.
>

So, that's 3 votes in favour of replacing UNBOUNDED with
MINVALUE/MAXVALUE for range partition bounds in PG 10. Not a huge
consensus, but no objections either. Any one else have an opinion?

Robert, have you been following this thread?

I was thinking of pushing this later today, in time for beta2.

Regards,
Dean

  31. Re: Multi column range partition table

    Noah Misch <noah@leadboat.com> — 2017-07-11T06:32:28Z

    On Sun, Jul 09, 2017 at 08:42:32AM +0100, Dean Rasheed wrote:
> On 6 July 2017 at 22:43, Joe Conway <mail@joeconway.com> wrote:
> > I agree we should get this right the first time and I also agree with
> > Dean's proposal, so I guess I'm a +2
> >
> 
> On 7 July 2017 at 03:21, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> > +1 to releasing this syntax in PG 10.
> >
> 
> So, that's 3 votes in favour of replacing UNBOUNDED with
> MINVALUE/MAXVALUE for range partition bounds in PG 10. Not a huge
> consensus, but no objections either. Any one else have an opinion?
> 
> Robert, have you been following this thread?
> 
> I was thinking of pushing this later today, in time for beta2.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

  32. Re: Multi column range partition table

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-07-11T12:29:45Z

    On Sun, Jul 9, 2017 at 1:12 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 6 July 2017 at 22:43, Joe Conway <mail@joeconway.com> wrote:
>> I agree we should get this right the first time and I also agree with
>> Dean's proposal, so I guess I'm a +2
>>
>
> On 7 July 2017 at 03:21, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> +1 to releasing this syntax in PG 10.
>>
>
> So, that's 3 votes in favour of replacing UNBOUNDED with
> MINVALUE/MAXVALUE for range partition bounds in PG 10. Not a huge
> consensus, but no objections either. Any one else have an opinion?
>

+     <para>
+      Also note that some element types, such as <literal>timestamp</>,
+      have a notion of "infinity", which is just another value that can
+      be stored. This is different from <literal>MINVALUE</> and
+      <literal>MAXVALUE</>, which are not real values that can be stored,
+      but rather they are ways of saying the value is unbounded.
+      <literal>MAXVALUE</> can be thought of as being greater than any
+      other value, including "infinity" and <literal>MINVALUE</> as being
+      less than any other value, including "minus infinity". Thus the range
+      <literal>FROM ('infinity') TO (MAXVALUE)</> is not an empty range; it
+      allows precisely one value to be stored &mdash; the timestamp
+      "infinity".
      </para>

The description in this paragraph seems to be attaching intuitive
meaning of word "unbounded" to MAXVALUE and MINVALUE, which have
different intuitive meanings of themselves. Not sure if that's how we
should describe MAXVALUE/MINVALUE.

Most of the patch seems to be replacing "content" with "kind",
RangeDatumContent with PartitionRangeDatumKind  and RANGE_DATUM_FINITE
with PARTITION_RANGE_DATUM_VALUE. But those changes in name don't seem
to be adding much value to the patch. Replacing RANGE_DATUM_NEG_INF
and RANGE_DATUM_POS_INF with PARTITION_RANGE_DATUM_MINVALUE and
PARTITION_RANGE_DATUM_MAXVALUE looks like a good change in line with
MINVALUE/MAXVALUE change. May be we should reuse the previous
variables, enum type name and except those two, so that the total
change introduced by the patch is minimal.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  33. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-11T19:24:53Z

    On 11 July 2017 at 13:29, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> +     <para>
> +      Also note that some element types, such as <literal>timestamp</>,
> +      have a notion of "infinity", which is just another value that can
> +      be stored. This is different from <literal>MINVALUE</> and
> +      <literal>MAXVALUE</>, which are not real values that can be stored,
> +      but rather they are ways of saying the value is unbounded.
> +      <literal>MAXVALUE</> can be thought of as being greater than any
> +      other value, including "infinity" and <literal>MINVALUE</> as being
> +      less than any other value, including "minus infinity". Thus the range
> +      <literal>FROM ('infinity') TO (MAXVALUE)</> is not an empty range; it
> +      allows precisely one value to be stored &mdash; the timestamp
> +      "infinity".
>       </para>
>
> The description in this paragraph seems to be attaching intuitive
> meaning of word "unbounded" to MAXVALUE and MINVALUE, which have
> different intuitive meanings of themselves. Not sure if that's how we
> should describe MAXVALUE/MINVALUE.
>

I'm not sure I understand your point. MINVALUE and MAXVALUE do mean
unbounded below and above respectively. This paragraph is just making
the point that that isn't the same as -/+ infinity.


> Most of the patch seems to be replacing "content" with "kind",
> RangeDatumContent with PartitionRangeDatumKind  and RANGE_DATUM_FINITE
> with PARTITION_RANGE_DATUM_VALUE. But those changes in name don't seem
> to be adding much value to the patch. Replacing RANGE_DATUM_NEG_INF
> and RANGE_DATUM_POS_INF with PARTITION_RANGE_DATUM_MINVALUE and
> PARTITION_RANGE_DATUM_MAXVALUE looks like a good change in line with
> MINVALUE/MAXVALUE change. May be we should reuse the previous
> variables, enum type name and except those two, so that the total
> change introduced by the patch is minimal.
>

No, this isn't just renaming that other enum. It's about replacing the
boolean "infinite" flag on PartitionRangeDatum with something that can
properly enumerate the 3 kinds of PartitionRangeDatum that are allowed
(and, as noted above "finite"/"infinite isn't the right terminology
either). Putting that new enum in parsenodes.h makes it globally
available, wherever the PartitionRangeDatum structure is used. A
side-effect of that change is that the old RangeDatumContent enum that
was local to partition.c is no longer needed.

RangeDatumContent wouldn't be a good name for a globally visible enum
of this kind because that name fails to link it to partitioning in any
way, and could easily be confused as having something to do with RTEs
or range types. Also, the term "content" is more traditionally used in
the Postgres sources for a field *holding* content, rather than a
field specifying the *kind* of content. On the other hand, you'll note
that the term "kind" is by far the most commonly used term for naming
this kind of enum, and any matching fields.

IMO, code consistency and readability takes precedence over keeping
patch sizes down.

Regards,
Dean

  34. Re: Multi column range partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-07-12T01:56:22Z

    On 2017/07/12 4:24, Dean Rasheed wrote:
> On 11 July 2017 at 13:29, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote:
>> Most of the patch seems to be replacing "content" with "kind",
>> RangeDatumContent with PartitionRangeDatumKind  and RANGE_DATUM_FINITE
>> with PARTITION_RANGE_DATUM_VALUE. But those changes in name don't seem
>> to be adding much value to the patch. Replacing RANGE_DATUM_NEG_INF
>> and RANGE_DATUM_POS_INF with PARTITION_RANGE_DATUM_MINVALUE and
>> PARTITION_RANGE_DATUM_MAXVALUE looks like a good change in line with
>> MINVALUE/MAXVALUE change. May be we should reuse the previous
>> variables, enum type name and except those two, so that the total
>> change introduced by the patch is minimal.
>>
> 
> No, this isn't just renaming that other enum. It's about replacing the
> boolean "infinite" flag on PartitionRangeDatum with something that can
> properly enumerate the 3 kinds of PartitionRangeDatum that are allowed
> (and, as noted above "finite"/"infinite isn't the right terminology
> either). Putting that new enum in parsenodes.h makes it globally
> available, wherever the PartitionRangeDatum structure is used. A
> side-effect of that change is that the old RangeDatumContent enum that
> was local to partition.c is no longer needed.
> 
> RangeDatumContent wouldn't be a good name for a globally visible enum
> of this kind because that name fails to link it to partitioning in any
> way, and could easily be confused as having something to do with RTEs
> or range types. Also, the term "content" is more traditionally used in
> the Postgres sources for a field *holding* content, rather than a
> field specifying the *kind* of content. On the other hand, you'll note
> that the term "kind" is by far the most commonly used term for naming
> this kind of enum, and any matching fields.
> 
> IMO, code consistency and readability takes precedence over keeping
> patch sizes down.

I agree with Dean here that the new global PartitionRangeDatumKind enum is
an improvement over the previous infinite flag in the parse node plus the
partition.c local RangeDatumContent enum for all the reasons he mentioned.

Thanks,
Amit

  35. Re: Multi column range partition table

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-07-12T09:46:13Z

    On Wed, Jul 12, 2017 at 12:54 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 11 July 2017 at 13:29, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> +     <para>
>> +      Also note that some element types, such as <literal>timestamp</>,
>> +      have a notion of "infinity", which is just another value that can
>> +      be stored. This is different from <literal>MINVALUE</> and
>> +      <literal>MAXVALUE</>, which are not real values that can be stored,
>> +      but rather they are ways of saying the value is unbounded.
>> +      <literal>MAXVALUE</> can be thought of as being greater than any
>> +      other value, including "infinity" and <literal>MINVALUE</> as being
>> +      less than any other value, including "minus infinity". Thus the range
>> +      <literal>FROM ('infinity') TO (MAXVALUE)</> is not an empty range; it
>> +      allows precisely one value to be stored &mdash; the timestamp
>> +      "infinity".
>>       </para>
>>
>> The description in this paragraph seems to be attaching intuitive
>> meaning of word "unbounded" to MAXVALUE and MINVALUE, which have
>> different intuitive meanings of themselves. Not sure if that's how we
>> should describe MAXVALUE/MINVALUE.
>>
>
> I'm not sure I understand your point. MINVALUE and MAXVALUE do mean
> unbounded below and above respectively. This paragraph is just making
> the point that that isn't the same as -/+ infinity.
>

What confuses me and probably users is something named min/max"value"
is not a value but something lesser or greater than any other "value".
The paragraph above explains that <literal>FROM ('infinity') TO
(MAXVALUE)</> implies a partition with only infinity value in there.
What would be the meaning of <literal>FROM (MINVALUE) TO ('minus
infinity')</>, would that be allowed? What would it contain esp. when
the upper bounds are always exclusive?

>
>> Most of the patch seems to be replacing "content" with "kind",
>> RangeDatumContent with PartitionRangeDatumKind  and RANGE_DATUM_FINITE
>> with PARTITION_RANGE_DATUM_VALUE. But those changes in name don't seem
>> to be adding much value to the patch. Replacing RANGE_DATUM_NEG_INF
>> and RANGE_DATUM_POS_INF with PARTITION_RANGE_DATUM_MINVALUE and
>> PARTITION_RANGE_DATUM_MAXVALUE looks like a good change in line with
>> MINVALUE/MAXVALUE change. May be we should reuse the previous
>> variables, enum type name and except those two, so that the total
>> change introduced by the patch is minimal.
>>
>
> No, this isn't just renaming that other enum. It's about replacing the
> boolean "infinite" flag on PartitionRangeDatum with something that can
> properly enumerate the 3 kinds of PartitionRangeDatum that are allowed
> (and, as noted above "finite"/"infinite isn't the right terminology
> either).

Right. I think we need that change.

> Putting that new enum in parsenodes.h makes it globally
> available, wherever the PartitionRangeDatum structure is used. A
> side-effect of that change is that the old RangeDatumContent enum that
> was local to partition.c is no longer needed.

Hmm, I failed to notice the changes in _out, _equal, _read functions.
The downside is that enum can not be used for anything other than
partitioning. But I can not imagine where will we use it though.

>
> RangeDatumContent wouldn't be a good name for a globally visible enum
> of this kind because that name fails to link it to partitioning in any
> way, and could easily be confused as having something to do with RTEs
> or range types. Also, the term "content" is more traditionally used in
> the Postgres sources for a field *holding* content, rather than a
> field specifying the *kind* of content. On the other hand, you'll note
> that the term "kind" is by far the most commonly used term for naming
> this kind of enum, and any matching fields.

Ok.

>
> IMO, code consistency and readability takes precedence over keeping
> patch sizes down.

No doubt about that.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  36. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-13T08:19:28Z

    On 12 July 2017 at 10:46, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Wed, Jul 12, 2017 at 12:54 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> On 11 July 2017 at 13:29, Ashutosh Bapat
>>> The description in this paragraph seems to be attaching intuitive
>>> meaning of word "unbounded" to MAXVALUE and MINVALUE, which have
>>> different intuitive meanings of themselves. Not sure if that's how we
>>> should describe MAXVALUE/MINVALUE.
>>>
>> I'm not sure I understand your point. MINVALUE and MAXVALUE do mean
>> unbounded below and above respectively. This paragraph is just making
>> the point that that isn't the same as -/+ infinity.
>>
> What confuses me and probably users is something named min/max"value"
> is not a value but something lesser or greater than any other "value".

Ah OK, I see what you're saying.

It's worth noting though that, after a little looking around, I found
that Oracle, MySQL and DB2 all use MINVALUE/MAXVALUE for unbounded
range partitions (although in the case of Oracle and MySQL, they
currently only support specifying upper bounds, and only use MAXVALUE
at the moment).

So MINVALUE/MAXVALUE are likely to be familiar to at least some people
coming from other databases. Of course, for those other databases, the
surrounding syntax for creating partitioned tables is completely
different, but at least this makes the bounds themselves portable (our
supported set of bounds will be a superset of those supported by
Oracle and MySQL, and I think the same as those supported by DB2).

I also personally quite like those terms, because they're nice and
concise, and it's pretty obvious which is which.

Regards,
Dean

  37. Re: Multi column range partition table

    Robert Haas <robertmhaas@gmail.com> — 2017-07-14T05:12:26Z

    On Sun, Jul 9, 2017 at 2:42 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 6 July 2017 at 22:43, Joe Conway <mail@joeconway.com> wrote:
>> I agree we should get this right the first time and I also agree with
>> Dean's proposal, so I guess I'm a +2
>
> On 7 July 2017 at 03:21, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> +1 to releasing this syntax in PG 10.
>
> So, that's 3 votes in favour of replacing UNBOUNDED with
> MINVALUE/MAXVALUE for range partition bounds in PG 10. Not a huge
> consensus, but no objections either. Any one else have an opinion?
>
> Robert, have you been following this thread?

Uh, no.  Sorry.  I agree that it's a big problem that (10, UNBOUNDED)
interpreted as a maximum value means first_column <= 10 and when
interpreted as a minimum value means first_column >= 10, because those
things aren't opposites of each other.  I guess the proposal here
would make (10, MAXVALUE) as a maximum value mean first_column <= 10
and as a minimum would mean first_column > 10, and contrariwise for
MINVALUE.  That seems to restore the intended design principle of the
system, which is good, but...

...originally, Amit proposed to attach a postfix INCLUSIVE or
EXCLUSIVE to each bound specification, and this does feel like a bit
of a back door to the same place, kinda.  A partition defined to run
from (10, MAXVALUE) TO (11, MAXVALUE) is a lot like a partition
defined to run from (10) EXCLUSIVE to (11) EXCLUSIVE.  And if we
eventually decide to allow that, then what will be the difference
between a partition which starts at (10, MAXVALUE) EXCLUSIVE and one
which starts from (10, MAXVALUE) INCLUSIVE?

I haven't thought through this well enough to be sure that there's any
problem with what is being proposed, and I definitely don't have a
better solution off the top of my head, but I feel slightly nervous.

Apologies again for the slow response - will update again by Monday.

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

  38. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-16T10:40:55Z

    On 14 July 2017 at 06:12, Robert Haas <robertmhaas@gmail.com> wrote:
> I agree that it's a big problem that (10, UNBOUNDED)
> interpreted as a maximum value means first_column <= 10 and when
> interpreted as a minimum value means first_column >= 10, because those
> things aren't opposites of each other.  I guess the proposal here
> would make (10, MAXVALUE) as a maximum value mean first_column <= 10
> and as a minimum would mean first_column > 10, and contrariwise for
> MINVALUE.  That seems to restore the intended design principle of the
> system, which is good

Right. So in general, when using MINVALUE/MAXVALUE for the 2nd column
of a 2-column partitioning scheme, the partition constraints simplify
as follows:

  FROM (x, MINVALUE) => col1 >= x
  FROM (x, MAXVALUE) => col1 > x

  TO (x, MINVALUE)   => col1 < x
  TO (x, MAXVALUE)   => col1 <= x

which restores the property that one partition can be made contiguous
with another by having the upper bounds of one partition equal to the
lower bounds of the other.

Note that the choice of MINVALUE or MAXVALUE only affects whether the
constraint on the previous column is inclusive or exclusive. That's
quite different from what an INCLUSIVE/EXCLUSIVE flag would do.


>, but...
>
> ...originally, Amit proposed to attach a postfix INCLUSIVE or
> EXCLUSIVE to each bound specification, and this does feel like a bit
> of a back door to the same place, kinda.  A partition defined to run
> from (10, MAXVALUE) TO (11, MAXVALUE) is a lot like a partition
> defined to run from (10) EXCLUSIVE to (11) EXCLUSIVE.  And if we
> eventually decide to allow that, then what will be the difference
> between a partition which starts at (10, MAXVALUE) EXCLUSIVE and one
> which starts from (10, MAXVALUE) INCLUSIVE?

The INCLUSIVE/EXCLUSIVE flag would apply to the constraint as a whole:

  FROM (x, y) INCLUSIVE => (col1, col2) >= (x, y)
  FROM (x, y) EXCLUSIVE => (col1, col2) > (x, y)

  TO (x, y) INCLUSIVE   => (col1, col2) <= (x, y)
  TO (x, y) EXCLUSIVE   => (col1, col2) < (x, y)

which, when expanded out, actually only affects the constraint on the
final column, and then only in the case where all the other columns
are equal to the partition bound value:

  FROM (x, y) INCLUSIVE => col1 > x OR (col1 = x AND col2 >= y)
  FROM (x, y) EXCLUSIVE => col1 > x OR (col1 = x AND col2 > y)

  TO (x, y) INCLUSIVE   => col1 < x OR (col2 = x AND col2 <= y)
  TO (x, y) EXCLUSIVE   => col1 < x OR (col2 = x AND col2 < y)

So while MINVALUE/MAXVALUE makes a particular column unbounded
below/above, and as a side-effect can influence the inclusivity of the
preceding column, INCLUSIVE/EXCLUSIVE affects the inclusivity of the
final column (something that MINVALUE/MAXVALUE cannot do).

MINVALUE/MAXVALUE takes precedence, in the sense that if the bound on
any column is MINVALUE/MAXVALUE, that column and any later columns are
unbounded and no longer appear in the partition constraint expression,
and so any INCLUSIVE/EXCLUSIVE flag would have no effect. That seems
pretty intuitive to me -- "unbounded inclusive" is no different from
"unbounded exclusive".

Technically, anything that can be done using INCLUSIVE/EXCLUSIVE can
also be done using using MINVALUE/MAXVALUE, by artificially adding
another partitioning column and making it unbounded above/below, but
that would really just be a hack, and it (artificially adding an extra
column) would be unnecessary if we added INCLUSIVE/EXCLUSIVE support
in a later release. Thus, I think the 2 features would complement each
other quite nicely.

Regards,
Dean

  39. Re: Multi column range partition table

    Robert Haas <robertmhaas@gmail.com> — 2017-07-17T15:34:45Z

    On Sun, Jul 16, 2017 at 6:40 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Technically, anything that can be done using INCLUSIVE/EXCLUSIVE can
> also be done using using MINVALUE/MAXVALUE, by artificially adding
> another partitioning column and making it unbounded above/below, but
> that would really just be a hack, and it (artificially adding an extra
> column) would be unnecessary if we added INCLUSIVE/EXCLUSIVE support
> in a later release. Thus, I think the 2 features would complement each
> other quite nicely.

OK, works for me.  I'm not really keen about the MINVALUE/MAXVALUE
syntax -- it's really +/- infinity, not a value at all -- but I
haven't got a better proposal and yours at least has the virtue of
perhaps being familiar to those who know about Oracle.

Do you want to own this open item, then?

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

  40. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-17T16:37:13Z

    On 17 July 2017 at 16:34, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sun, Jul 16, 2017 at 6:40 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> Technically, anything that can be done using INCLUSIVE/EXCLUSIVE can
>> also be done using using MINVALUE/MAXVALUE, by artificially adding
>> another partitioning column and making it unbounded above/below, but
>> that would really just be a hack, and it (artificially adding an extra
>> column) would be unnecessary if we added INCLUSIVE/EXCLUSIVE support
>> in a later release. Thus, I think the 2 features would complement each
>> other quite nicely.
>
> OK, works for me.  I'm not really keen about the MINVALUE/MAXVALUE
> syntax -- it's really +/- infinity, not a value at all -- but I
> haven't got a better proposal and yours at least has the virtue of
> perhaps being familiar to those who know about Oracle.
>

Cool. Sounds like we've reached a consensus, albeit with some
reservations around the fact that MINVALUE/MAXVALUE aren't actually
values, despite their names.

+/- infinity *are* values for some datatypes such as timestamps, so it
had to be something different from that, and MINVALUE/MAXVALUE are
quite short and simple, and match the syntax used by 3 other
databases.


> Do you want to own this open item, then?
>

OK.

I need to give the patch another read-through, and then I'll aim to
push it sometime in the next few days.

Regards,
Dean

  41. Re: Multi column range partition table

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-07-21T17:34:57Z

    On 17 July 2017 at 17:37, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 17 July 2017 at 16:34, Robert Haas <robertmhaas@gmail.com> wrote:
>> Do you want to own this open item, then?
>>
> OK.
>
> I need to give the patch another read-through, and then I'll aim to
> push it sometime in the next few days.
>

Committed.

Regards,
Dean