Re: Boolean partitions syntax

> On Apr 11, 2018, at 4:33 AM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>
> Thank you for the comments.
>
> At Wed, 11 Apr 2018 13:51:55 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<3d0fda29-986c-d970-a22c-b4bd44f56931@lab.ntt.co.jp>
>> Horiguchi-san,
>>
>> Thanks for working on this.
>>
>> On 2018/04/11 13:20, Kyotaro HORIGUCHI wrote:
>>> At Wed, 11 Apr 2018 11:27:17 +0900, Amit Langote wrote:
>>>> On 2018/04/11 10:44, Tom Lane wrote:
>>>>> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
>>>>>> At least partition bound *must* be a constant. Any expression
>>>>>> that can be reduced to a constant at parse time ought to be
>>>>>> accepted but must not be accepted if not.
>>>>>
>>>>> My point is that *any* expression can be reduced to a constant,
>>>>> we just have to do so.
>>>
>>> Agreed in that sense. What was in my mind was something like
>>> column reference, random() falls into reducible category of
>>> course.
>>>
>>> # I regard the change in gram.y is regarded as acceptable as a
>>> # direction, so I'll continue to working on this.
>>
>> I haven't yet reviewed the grammar changes in detail yet...
>
>>>> I think what Tom is proposing here, instead of bailing out upon
>>>> eval_const_expressions() failing to simplify the input expression to a
>>>> Const, is to *invoke the executor* to evaluate the expression, like the
>>>> optimizer does in evaluate_expr, and cook up a Const with whatever comes
>>>> out to store it into the catalog (that is, in relpartbound).
>>>
>>> Yes. In the attached I used evaluate_expr by making it non-static
>>> function. a_expr used instead of partbound_datum is changed to
>>> u_expr, which is the same with range bounds.
>>>
>>>> =# create table c1 partition of p for values in (random() * 100);
>>>> CREATE TABLE
>>>> =# \d c1
>>> ...
>>>> Partition of: p FOR VALUES IN (97)
>>
>> I looked at the non-gram.y portions of the patch for now as I was also
>> playing with this.  Some comments on your patch:
>>
>> * You missed adding a break here for the EXPR_KIND_PARTITION_EXPRESSION case
>>
>>         case EXPR_KIND_PARTITION_EXPRESSION:
>>             err = _("window functions are not allowed in partition key
>> expressions");
>> +        case EXPR_KIND_PARTITION_BOUNDS:
>> +            err = _("window functions are not allowed in partition bounds");
>>             break;
>>
>> So, the following is the wrong error message that you probably failed to
>> notice:
>
> Oops! Fixed along with another one. I haven't looked the
> difference so seriously.
>
>> --- a/src/test/regress/expected/create_table.out
>> +++ b/src/test/regress/expected/create_table.out
>> @@ -308,7 +308,7 @@ CREATE TABLE partitioned (
>>     a int,
>>     b int
>> ) PARTITION BY RANGE ((avg(a) OVER (PARTITION BY b)));
>> -ERROR:  window functions are not allowed in partition key expressions
>> +ERROR:  window functions are not allowed in partition bounds
>
> I felt a bit uneasy to saw that in the very corner of my mind..
>
>> * I think the new ParseExprKind you added should omit the last "S", that
>> is, name it EXPR_KIND_PARTITION_BOUND, because these are expressions to
>> represent individual bound values.  And so adjust the comment to say "bound".
>>
>> +    EXPR_KIND_PARTITION_BOUNDS,     /* partition bounds value */
>
> Agreed.
>
>> * When looking at the changes to transformPartitionBoundValue, I noticed
>> that there is no new argument Oid colTypColl
>>
>> static Const *
>> -transformPartitionBoundValue(ParseState *pstate, A_Const *con,
>> +transformPartitionBoundValue(ParseState *pstate, Node *val,
>>                              const char *colName, Oid colType, int32
>> colTypmod)
>>
>> that's because you seem to pass the expression's type, typmod, and typcoll
>> to the newly added call to evaluate_expr.  I wonder if we should really
>> pass the partition key specified values here.  We already receive first
>> two from the caller.
>
> I overlooked that the value (Expr) is already coerced at the
> time. Added collation handling and this would be back-patchable.
>
> I'm not sure how we should handle collate in this case but the
> patch rejects bound values with non-default collation if it is
> different from partition key.
>
> Collation check works
>
> =# create table p (a text collate "de_DE") partition by (a)
> =# create table c1 partition of p for values in (('a' collate "ja_JP"));
> ERROR:  collation mismatch between partition key expression (12622) and partition bound value (12684)
> LINE 1: create table c1 partition of p for values in (('a' collate "...
>
>> * In the changes to create_table.out
>>
>> @@ -450,13 +450,9 @@ CREATE TABLE part_1 PARTITION OF list_parted FOR
>> VALUES IN ('1');
>> CREATE TABLE part_2 PARTITION OF list_parted FOR VALUES IN (2);
>> CREATE TABLE part_null PARTITION OF list_parted FOR VALUES IN (null);
>> CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
>> -ERROR:  syntax error at or near "int"
>> -LINE 1: ... fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
>> -                                                              ^
>> +ERROR:  partition "fail_part" would overlap partition "part_1"
>> CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
>> -ERROR:  syntax error at or near "::"
>> -LINE 1: ...fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
>> -                                                                ^
>> +ERROR:  partition "fail_part" would overlap partition "part_1"
>>
>> How about just remove the two tests that now get the overlap error.
>
> Removed.
>
>> Also,
>>
>> @@ -490,12 +486,10 @@ CREATE TABLE moneyp (
>>     a money
>> ) PARTITION BY LIST (a);
>> CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
>> -ERROR:  specified value cannot be cast to type money for column "a"
>> -LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
>> -                                                                   ^
>> -DETAIL:  The cast requires a non-immutable conversion.
>> -HINT:  Try putting the literal value in single quotes.
>> +CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
>> +CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12,
>> '99')::int);
>> CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
>> +ERROR:  relation "moneyp_10" already exists
>>
>> Remove the command that causes overlap error, or simply just remove the
>> whole moneyp test, as its purpose was to exercise the code that's now removed.
>
> Removed. And added tests for collation handling. (and
> partbound_datum_list is fixed.)

I experimented with this patch a bit. First, I could not build it:

parse_expr.c:1853:8: error: use of undeclared identifier 'EXPR_KIND_PARTITION_BOUNDS'; did you mean
      'EXPR_KIND_PARTITION_BOUND'?
                case EXPR_KIND_PARTITION_BOUNDS:
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
                     EXPR_KIND_PARTITION_BOUND
../../../src/include/parser/parse_node.h:73:2: note: 'EXPR_KIND_PARTITION_BOUND' declared here
        EXPR_KIND_PARTITION_BOUND,      /* partition bounds value */
        ^
parse_expr.c:3480:8: error: use of undeclared identifier 'EXPR_KIND_PARTITION_BOUNDS'; did you mean
      'EXPR_KIND_PARTITION_BOUND'?
                case EXPR_KIND_PARTITION_BOUNDS:
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
                     EXPR_KIND_PARTITION_BOUND
../../../src/include/parser/parse_node.h:73:2: note: 'EXPR_KIND_PARTITION_BOUND' declared here
        EXPR_KIND_PARTITION_BOUND,      /* partition bounds value */
        ^
2 errors generated.

The attached patch fixes the error.

I ran the following cases:

Case #1: My Original Test Case

    CREATE TABLE records (
        id int GENERATED BY DEFAULT AS IDENTITY NOT NULL,
        record_date date NOT NULL,
        record_text text,
        archived bool NOT NULL DEFAULT FALSE
    ) PARTITION BY LIST(archived);

    CREATE TABLE records_archive
    PARTITION OF records
    FOR VALUES IN (TRUE);

    CREATE TABLE records_active
    PARTITION OF records
    FOR VALUES IN (FALSE);

Everything created like a charm.

Case #2: random()

    CREATE TABLE oddity (
        id int GENERATED BY DEFAULT AS IDENTITY NOT NULL,
        random_filter int
    ) PARTITION BY LIST(random_filter);

    CREATE TABLE oddity_random
    PARTITION OF oddity
    FOR VALUES IN ((random() * 100)::int);

I did a \d+ on oddity and:

    partitions=# \d+ oddity
    (truncated)
    Partition key: LIST (random_filter)
    Partitions: oddity_random FOR VALUES IN (19)

So this appears to behave as described above.

Attached is the patch with the fix for the build.  This is the first time I’m attaching
a patch for the core server, so apologizes if I missed up the formatting.