Re: [HACKERS] [POC] hash partitioning - Mailing list pgsql-hackers

On 2017/05/12 14:24, Ashutosh Bapat wrote:
> On Fri, May 12, 2017 at 8:08 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/05/12 11:20, Robert Haas wrote:
>>> 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.

We *do* actually, for real columns:

create table p (a int) partition by range (a);
\d p             Table "public.p"Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------a      | integer |           | not null |
Partition key: RANGE (a)

For expression keys, we emit IS NOT NULL as part of the implicit partition
constraint.  The above check for NULL is really for the expressions,
because if any simple columns of the key contain NULL, they will fail the
NOT NULL constraint itself (with that error message).  As I said in my
previous message, I'm thinking that emitting IS NOT NULL as part of the
implicit partition constraint might be better instead of adding it as a
NOT NULL constraint, that is, for the simple column keys; we already do
that for the expression keys for which we cannot add the NOT NULL
constraint anyway.

The way things are currently, error messages generated when a row with
NULL in the range partition key is *directly* into the partition looks a
bit inconsistent, depending on whether the target key is a simple column
or expression:

create table p (a int, b int) partition by range (a, abs(b));
create table p1 partition of p for values from (1, 1) to (1, 10);

insert into p1 values (NULL, NULL);
ERROR:  null value in column "a" violates not-null constraint
DETAIL:  Failing row contains (null, null).

insert into p1 values (1, NULL);
ERROR:  new row for relation "p1" violates partition constraint
DETAIL:  Failing row contains (1, null).

It would be nice if both said "violates partition constraint".

BTW, note that this is independent of your suggestion to emit "partition
not found" message instead of the "no NULLs allowed in the range partition
key" message, which seems fine to me to implement.

Thanks,
Amit