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