Re: Boolean partitions syntax

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

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center