Re: [POC] hash partitioning

On Fri, May 12, 2017 at 8:08 AM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2017/05/12 11:20, Robert Haas wrote:
>> On Thu, May 11, 2017 at 10:15 PM, Amit Langote
>> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> On 2017/05/12 10:42, Robert Haas wrote:
>>>> On Thu, May 11, 2017 at 12:02 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>>>>> We need to add PARTITION_STRATEGY_HASH as well, we don't support NULL
>>>>> for hash also, right?
>>>>
>>>> I think it should.
>>>>
>>>> Actually, I think that not supporting nulls for range partitioning may
>>>> have been a fairly bad decision.
>>>
>>> I think the relevant discussion concluded [1] that way, because we
>>> couldn't decide which interface to provide for specifying where NULLs are
>>> placed or because we decided to think about it later.
>>
>> Yeah, but I have a feeling that marking the columns NOT NULL is going
>> to make it really hard to support that in the future when we get the
>> syntax hammered out. If it had only affected the partition
>> constraints that'd be different.
>
> So, adding keycol IS NOT NULL (like we currently do for expressions) in
> the implicit partition constraint would be more future-proof than
> generating an actual catalogued NOT NULL constraint on the keycol? I now
> tend to think it would be better. Directly inserting into a range
> partition with a NULL value for a column currently generates a "null value
> in column \"%s\" violates not-null constraint" instead of perhaps more
> relevant "new row for relation \"%s\" violates partition constraint".
> That said, we *do* document the fact that a NOT NULL constraint is added
> on range key columns, but we might as well document instead that we don't
> currently support routing tuples with NULL values in the partition key
> through a range-partitioned table and so NULL values cause error.

in get_partition_for_tuple() we have
if (key->strategy == PARTITION_STRATEGY_RANGE)
{
/* Disallow nulls in the range partition key of the tuple */
for (i = 0; i < key->partnatts; i++)
if (isnull[i])
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("range partition key of row contains null")));
}

Instead of throwing an error here, we should probably return -1 and
let the error be ""no partition of relation \"%s\" found for row",
which is the real error, not having a partition which can accept NULL.
If in future we decide to support NULL values in partition keys, we
need to just remove above code from get_partition_for_tuple() and
everything will work as is. I am assuming that we don't add any
implicit/explicit NOT NULL constraint right now.

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