Re: Boolean partitions syntax

> On Apr 11, 2018, at 4:33 AM, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> Thank you for the comments.
>
> At Wed, 11 Apr 2018 13:51:55 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <3d0fda29-986c-d970-a22c-b4bd44f56931(at)lab(dot)ntt(dot)co(dot)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(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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.