Re: [HACKERS] Multi column range partition table

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 — 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