At Wed, 11 Apr 2018 11:27:17 +0900, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote in <1810b14f-3cd7-aff5-8358-c225c0231...@lab.ntt.co.jp> > 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. > 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) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index ed6b680ed8..cfb0984100 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -152,8 +152,6 @@ static Node *substitute_actual_parameters(Node *expr, int nargs, List *args, static Node *substitute_actual_parameters_mutator(Node *node, substitute_actual_parameters_context *context); static void sql_inline_error_callback(void *arg); -static Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, - Oid result_collation); static Query *substitute_actual_srf_parameters(Query *expr, int nargs, List *args); static Node *substitute_actual_srf_parameters_mutator(Node *node, @@ -4842,7 +4840,7 @@ sql_inline_error_callback(void *arg) * We use the executor's routine ExecEvalExpr() to avoid duplication of * code and ensure we get the same result as the executor would get. */ -static Expr * +Expr * evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, Oid result_collation) { diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index dd0c26c11b..02979a3533 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -180,6 +180,8 @@ static Node *makeXmlExpr(XmlExprOp op, char *name, List *named_args, static List *mergeTableFuncParameters(List *func_args, List *columns); static TypeName *TableFuncTypeName(List *columns); static RangeVar *makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner); +static Node *makePartRangeDatum(PartitionRangeDatumKind kind, Node *value, + int location); static void SplitColQualList(List *qualList, List **constraintList, CollateClause **collClause, core_yyscan_t yyscanner); @@ -472,7 +474,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type <node> columnDef columnOptions %type <defelt> def_elem reloption_elem old_aggr_elem operator_def_elem %type <node> def_arg columnElem where_clause where_or_current_clause - a_expr b_expr c_expr AexprConst indirection_el opt_slice_bound + a_expr u_expr b_expr b0_expr c_expr c0_expr AexprConst indirection_el opt_slice_bound columnref in_expr having_clause func_table xmltable array_expr ExclusionWhereClause operator_def_arg %type <list> rowsfrom_item rowsfrom_list opt_col_def_list @@ -585,7 +587,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type <partelem> part_elem %type <list> part_params %type <partboundspec> PartitionBoundSpec -%type <node> partbound_datum PartitionRangeDatum +%type <node> PartitionRangeDatum %type <list> hash_partbound partbound_datum_list range_datum_list %type <defelt> hash_partbound_elem @@ -2804,15 +2806,9 @@ hash_partbound: } ; -partbound_datum: - Sconst { $$ = makeStringConst($1, @1); } - | NumericOnly { $$ = makeAConst($1, @1); } - | NULL_P { $$ = makeNullAConst(@1); } - ; - partbound_datum_list: - partbound_datum { $$ = list_make1($1); } - | partbound_datum_list ',' partbound_datum + u_expr { $$ = list_make1($1); } + | partbound_datum_list ',' a_expr { $$ = lappend($1, $3); } ; @@ -2825,33 +2821,18 @@ range_datum_list: PartitionRangeDatum: MINVALUE { - PartitionRangeDatum *n = makeNode(PartitionRangeDatum); - - n->kind = PARTITION_RANGE_DATUM_MINVALUE; - n->value = NULL; - n->location = @1; - - $$ = (Node *) n; + $$ = makePartRangeDatum(PARTITION_RANGE_DATUM_MINVALUE, + NULL, @1); } | MAXVALUE { - PartitionRangeDatum *n = makeNode(PartitionRangeDatum); - - n->kind = PARTITION_RANGE_DATUM_MAXVALUE; - n->value = NULL; - n->location = @1; - - $$ = (Node *) n; + $$ = makePartRangeDatum(PARTITION_RANGE_DATUM_MAXVALUE, + NULL, @1); } - | partbound_datum + | u_expr { - PartitionRangeDatum *n = makeNode(PartitionRangeDatum); - - n->kind = PARTITION_RANGE_DATUM_VALUE; - n->value = $1; - n->location = @1; - - $$ = (Node *) n; + $$ = makePartRangeDatum(PARTITION_RANGE_DATUM_VALUE, + $1, @1); } ; @@ -13478,9 +13459,20 @@ a_expr: c_expr { $$ = $1; } * cause trouble in the places where b_expr is used. For simplicity, we * just eliminate all the boolean-keyword-operator productions from b_expr. */ -b_expr: c_expr - { $$ = $1; } - | b_expr TYPECAST Typename +b_expr: c_expr { $$ = $1; } + | b0_expr { $$ = $1; } + ; + +/* + * u_expr is a subset of b_expr so as to be usable along with + * unreserved keywords. + */ +u_expr: c0_expr { $$ = $1; } + | b0_expr { $$ = $1; } + ; + +/* common part of b_expr and u_expr */ +b0_expr: b_expr TYPECAST Typename { $$ = makeTypeCast($1, $3, @2); } | '+' b_expr %prec UMINUS { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "+", NULL, $2, @1); } @@ -13554,7 +13546,11 @@ b_expr: c_expr * ambiguity to the b_expr syntax. */ c_expr: columnref { $$ = $1; } - | AexprConst { $$ = $1; } + | c0_expr { $$ = $1; } + ; + +/* common part of c_expr and u_expr */ +c0_expr: AexprConst { $$ = $1; } | PARAM opt_indirection { ParamRef *p = makeNode(ParamRef); @@ -16275,6 +16271,18 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner) return r; } +static Node * +makePartRangeDatum(PartitionRangeDatumKind kind, Node *value, int location) +{ + PartitionRangeDatum *n = makeNode(PartitionRangeDatum); + + n->kind = kind; + n->value = value; + n->location = location; + + return (Node *) n; +} + /* Separate Constraint nodes from COLLATE clauses in a ColQualList */ static void SplitColQualList(List *qualList, diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index 0307738946..61f59f7527 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -506,6 +506,12 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr) else err = _("grouping operations are not allowed in EXECUTE parameters"); + case EXPR_KIND_PARTITION_BOUNDS: + if (isAgg) + err = _("aggregate functions are not allowed in partition bounds"); + else + err = _("grouping operations are not allowed in partition bounds"); + break; case EXPR_KIND_TRIGGER_WHEN: if (isAgg) @@ -908,6 +914,8 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc, break; 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; case EXPR_KIND_CALL_ARGUMENT: err = _("window functions are not allowed in CALL arguments"); diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 38fbe3366f..a7f3d86f75 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -1850,6 +1850,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink) case EXPR_KIND_CALL_ARGUMENT: err = _("cannot use subquery in CALL argument"); break; + case EXPR_KIND_PARTITION_BOUNDS: + err = _("cannot use subquery in partition bounds"); + break; /* * There is intentionally no default: case here, so that the @@ -3474,6 +3477,8 @@ ParseExprKindName(ParseExprKind exprKind) return "WHEN"; case EXPR_KIND_PARTITION_EXPRESSION: return "PARTITION BY"; + case EXPR_KIND_PARTITION_BOUNDS: + return "partition bounds"; case EXPR_KIND_CALL_ARGUMENT: return "CALL"; case EXPR_KIND_MERGE_WHEN_AND: diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index 615aee6d15..a39e26dd76 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2306,6 +2306,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location) case EXPR_KIND_PARTITION_EXPRESSION: err = _("set-returning functions are not allowed in partition key expressions"); break; + case EXPR_KIND_PARTITION_BOUNDS: + err = _("set-returning functions are not allowed in partition bounds"); + break; case EXPR_KIND_CALL_ARGUMENT: err = _("set-returning functions are not allowed in CALL arguments"); break; diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index f9f9904bad..05b24e7f69 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -48,6 +48,7 @@ #include "miscadmin.h" #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" +#include "optimizer/clauses.h" #include "optimizer/planner.h" #include "parser/analyze.h" #include "parser/parse_clause.h" @@ -138,7 +139,7 @@ static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column); static void setSchemaName(char *context_schema, char **stmt_schema_name); static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd); static void validateInfiniteBounds(ParseState *pstate, List *blist); -static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con, +static Const *transformPartitionBoundValue(ParseState *pstate, Node *con, const char *colName, Oid colType, int32 colTypmod); @@ -3674,12 +3675,12 @@ transformPartitionBound(ParseState *pstate, Relation parent, result_spec->listdatums = NIL; foreach(cell, spec->listdatums) { - A_Const *con = castNode(A_Const, lfirst(cell)); + Node *expr = (Node *)lfirst (cell); Const *value; ListCell *cell2; bool duplicate; - value = transformPartitionBoundValue(pstate, con, + value = transformPartitionBoundValue(pstate, expr, colname, coltype, coltypmod); /* Don't add to the result if the value is a duplicate */ @@ -3740,7 +3741,6 @@ transformPartitionBound(ParseState *pstate, Relation parent, char *colname; Oid coltype; int32 coltypmod; - A_Const *con; Const *value; /* Get the column's name in case we need to output an error */ @@ -3761,8 +3761,8 @@ transformPartitionBound(ParseState *pstate, Relation parent, if (ldatum->value) { - con = castNode(A_Const, ldatum->value); - value = transformPartitionBoundValue(pstate, con, + value = transformPartitionBoundValue(pstate, + ldatum->value, colname, coltype, coltypmod); if (value->constisnull) @@ -3775,8 +3775,8 @@ transformPartitionBound(ParseState *pstate, Relation parent, if (rdatum->value) { - con = castNode(A_Const, rdatum->value); - value = transformPartitionBoundValue(pstate, con, + value = transformPartitionBoundValue(pstate, + rdatum->value, colname, coltype, coltypmod); if (value->constisnull) @@ -3845,13 +3845,13 @@ validateInfiniteBounds(ParseState *pstate, List *blist) * Transform one constant in a partition bound spec */ static Const * -transformPartitionBoundValue(ParseState *pstate, A_Const *con, +transformPartitionBoundValue(ParseState *pstate, Node *val, const char *colName, Oid colType, int32 colTypmod) { Node *value; /* Make it into a Const */ - value = (Node *) make_const(pstate, &con->val, con->location); + value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUNDS); /* Coerce to correct type */ value = coerce_to_target_type(pstate, @@ -3867,21 +3867,28 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg("specified value cannot be cast to type %s for column \"%s\"", format_type_be(colType), colName), - parser_errposition(pstate, con->location))); + parser_errposition(pstate, exprLocation(val)))); /* Simplify the expression, in case we had a coercion */ if (!IsA(value, Const)) value = (Node *) expression_planner((Expr *) value); - /* Fail if we don't have a constant (i.e., non-immutable coercion) */ + /* Eval if we still don't have a constant (i.e., non-immutable coercion) */ if (!IsA(value, Const)) - ereport(ERROR, - (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("specified value cannot be cast to type %s for column \"%s\"", - format_type_be(colType), colName), - errdetail("The cast requires a non-immutable conversion."), - errhint("Try putting the literal value in single quotes."), - parser_errposition(pstate, con->location))); + { + Oid result_type; + int32 result_typmod; + Oid result_col; + + /* Collect result characteristics */ + result_type = exprType(value); + result_typmod = exprTypmod(value); + result_col = exprCollation(value); + + value = (Node *)evaluate_expr((Expr *) value, result_type, + result_typmod, result_col); + Assert(IsA(value, Const)); + } return (Const *) value; } diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h index ba4fa4b68b..4b1a5b96f8 100644 --- a/src/include/optimizer/clauses.h +++ b/src/include/optimizer/clauses.h @@ -85,4 +85,6 @@ extern Node *estimate_expression_value(PlannerInfo *root, Node *node); extern Query *inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte); +extern Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, + Oid result_collation); #endif /* CLAUSES_H */ diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h index 3fd2151ccb..5eccf5078a 100644 --- a/src/include/parser/parse_node.h +++ b/src/include/parser/parse_node.h @@ -70,6 +70,7 @@ typedef enum ParseExprKind EXPR_KIND_TRIGGER_WHEN, /* WHEN condition in CREATE TRIGGER */ EXPR_KIND_POLICY, /* USING or WITH CHECK expr in policy */ EXPR_KIND_PARTITION_EXPRESSION, /* PARTITION BY expression */ + EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */ EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */ } ParseExprKind; diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index e724439037..c1ead46a7d 100644 --- 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 CREATE TABLE partitioned ( a int ) PARTITION BY LIST ((a LIKE (SELECT 1))); @@ -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" -- syntax does not allow empty list of values for list partitions CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN (); ERROR: syntax error at or near ")" @@ -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 DROP TABLE moneyp; -- immutable cast should work, though CREATE TABLE bigintp ( diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index 235bef13dc..a626bfe659 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -458,6 +458,8 @@ CREATE TABLE moneyp ( a money ) PARTITION BY LIST (a); CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10); +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'); DROP TABLE moneyp;