Re: Boolean partitions syntax

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

>> Currently transformPartitionBoundValue() applies eval_const_expressions()
>> by way of calling expression_planner().  However passing to it, say, an
>> expression representing random() is unable to reduce it to a Const because
>> simplify_function/evaluate_function won't compute a mutable function like
>> random().  So, that currently results in an error like this (note that
>> this is after applying Horiguchi-san's latest patch that enables
>> specifying random() as a partition bound in the first place):
>>
>> create table foo_part partition of foo for values in ((random())::int);
>> ERROR:  specified value cannot be cast to type integer for column "a"
>> LINE 1: ...table foo_random partition of foo for values in ((random()):...
>>                                                              ^
>> DETAIL:  The cast requires a non-immutable conversion.
>> HINT:  Try putting the literal value in single quotes.
>>
>> The error is output after the following if check in
>> transformPartitionBoundValue fails:
>>
>>     /* Fail if we don't have a constant (i.e., non-immutable coercion) */
>>     if (!IsA(value, Const))
>>
>> 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:

--- 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 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 */

* 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.

* 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.

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.

Thanks,
Amit