Обсуждение: using expression syntax for partition bounds (was: Re: Booleanpartitions syntax)
using expression syntax for partition bounds (was: Re: Booleanpartitions syntax)
От
Amit Langote
Дата:
(patch and discussion for PG 12) On 2018/04/22 1:28, Tom Lane wrote: > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes: >> [ v8-0001-Allow-generalized-expression-syntax-for-partition.patch ] > > I find what you did to a_expr here to be pretty horrid. Thanks for the review. > I think what you should do is lose the partbound_datum and > PartitionRangeDatum productions altogether, replacing those with just > a_expr, as in the attached grammar-only patch. This would result in > needing to identify MINVALUE and MAXVALUE during parse analysis, since > the grammar would just treat them as ColId expressions. But since we're > not intending to ever allow any actual column references in partition > expressions, I don't see any harm in allowing parse analysis to treat > ColumnRefs containing those names as meaning the special items. I have to agree this is better. > This is a little bit grotty, in that both MINVALUE and "minvalue" would > be recognized as the keyword, but it's sure a lot less messy than what's > there now. And IIRC there are some other places where we're a bit > squishy about the difference between identifiers and keywords, too. Hmm, yes. I tried to update the patch to do things that way. I'm going to create a new entry in the next CF titled "generalized expression syntax for partition bounds" and add the patch there. Thanks, Amit
Вложения
Re: using expression syntax for partition bounds (was: Re: Booleanpartitions syntax)
От
Amit Langote
Дата:
On 2018/04/23 11:37, Amit Langote wrote: > I tried to update the patch to do things that way. I'm going to create a > new entry in the next CF titled "generalized expression syntax for > partition bounds" and add the patch there. Tweaked the commit message to credit all the authors. Thanks, Amit
Вложения
Thanks. I have almost missed this.
At Mon, 23 Apr 2018 11:44:14 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<e8c770eb-a850-6645-8310-f3eaea3a72fd@lab.ntt.co.jp>
> On 2018/04/23 11:37, Amit Langote wrote:
> > I tried to update the patch to do things that way. I'm going to create a
> > new entry in the next CF titled "generalized expression syntax for
> > partition bounds" and add the patch there.
>
> Tweaked the commit message to credit all the authors.
+ any variable-free expression (subqueries, window functions, aggregate
+ functions, and set-returning functions are not allowed). The data type
+ of the partition bound expression must match the data type of the
+ corresponding partition key column.
Parititioning value is slimiar to column default expression in
the sense that it appeas within a DDL. The description for
DEFAULT is:
| The value is any variable-free expression (subqueries and
| cross-references to other columns in the current table are not
| allowed)
It actually refuses aggregates, window functions and SRFs but it
is not mentioned. Whichever we choose, they ought to have the
similar description.
> /*
> * Strip any top-level COLLATE clause, as they're inconsequential.
> * XXX - Should we add a WARNING here?
> */
> while (IsA(value, CollateExpr))
> value = (Node *) ((CollateExpr *) value)->arg;
Ok, I'll follow Tom's suggestion but collate is necessarilly
appears in this shape. For example ('a' collate "de_DE") || 'b')
has the collate node under the top ExprOp and we get a complaint
like following with it.
> ERROR: unrecognized node type: 123
123 is T_CollateExpr. The expression "value" (mmm..) reuires
eval_const_expressions to eliminate CollateExprs. It requires
assign_expr_collations to retrieve value's collation but we don't
do that since we ignore collation this in this case.
The following results in a strange-looking error.
> =# create table pt1 partition of p for values in (a);
> ERROR: column "a" does not exist
> LINE 1: create table pt1 partition of p for values in (a);
The p/pt1 actually have a column a.
The following results in similar error and it is wrong, too.
> =# create table pr1 partition of pr for values from (a + 1) to (a + 2);
> ERROR: column "a" does not exist
> LINE 1: create table pr1 partition of pr for values from (a + 1) to ...
Being similar but a bit different, the following command leads to
a SEGV since it creates PARTITION_RANGE_DATUM_VALUE with value =
NULL. Even if it leaves the original node, validateInfiniteBounds
rejects it and aborts.
> =# create table pr1 partition of pr for values from (hoge) to (hige);
(crash)
I fixed this using pre-columnref hook in the attached v3. This
behavles for columnrefs differently than DEFAULT.
The v3-2 does the same thing with DEFAULT. The behevior differs
whether the column exists or not.
> ERROR: cannot use column referencees in bound value
> ERROR: column "b" does not exist
I personally think that such similarity between DEFALUT and
partition bound (v3-2) is not required. But inserting the hook
(v3) doesn't look good for me.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 763b4f573c..b6f3ddc22f 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -275,10 +275,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<para>
Each of the values specified in
the <replaceable class="parameter">partition_bound_spec</replaceable> is
- a literal, <literal>NULL</literal>, <literal>MINVALUE</literal>, or
- <literal>MAXVALUE</literal>. Each literal value must be either a
- numeric constant that is coercible to the corresponding partition key
- column's type, or a string literal that is valid input for that type.
+ any variable-free expression (subqueries and cross-references to other
+ columns in the current table are not allowed). The data type of the
+ partition bound expression must match the data type of the corresponding
+ partition key column.
</para>
<para>
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 505ae0af85..77ebb40ef2 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -150,8 +150,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,
@@ -4840,7 +4838,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 5a36367446..8b0680a7fa 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -581,8 +581,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 <list> hash_partbound partbound_datum_list range_datum_list
+%type <list> hash_partbound
%type <defelt> hash_partbound_elem
/*
@@ -2739,7 +2738,7 @@ PartitionBoundSpec:
}
/* a LIST partition */
- | FOR VALUES IN_P '(' partbound_datum_list ')'
+ | FOR VALUES IN_P '(' expr_list ')'
{
PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
@@ -2752,7 +2751,7 @@ PartitionBoundSpec:
}
/* a RANGE partition */
- | FOR VALUES FROM '(' range_datum_list ')' TO '(' range_datum_list ')'
+ | FOR VALUES FROM '(' expr_list ')' TO '(' expr_list ')'
{
PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
@@ -2795,59 +2794,6 @@ hash_partbound:
}
;
-partbound_datum:
- Sconst { $$ = makeStringConst($1, @1); }
- | NumericOnly { $$ = makeAConst($1, @1); }
- | TRUE_P { $$ = makeStringConst(pstrdup("true"), @1); }
- | FALSE_P { $$ = makeStringConst(pstrdup("false"), @1); }
- | NULL_P { $$ = makeNullAConst(@1); }
- ;
-
-partbound_datum_list:
- partbound_datum { $$ = list_make1($1); }
- | partbound_datum_list ',' partbound_datum
- { $$ = lappend($1, $3); }
- ;
-
-range_datum_list:
- PartitionRangeDatum { $$ = list_make1($1); }
- | range_datum_list ',' PartitionRangeDatum
- { $$ = lappend($1, $3); }
- ;
-
-PartitionRangeDatum:
- MINVALUE
- {
- PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
- n->kind = PARTITION_RANGE_DATUM_MINVALUE;
- n->value = NULL;
- n->location = @1;
-
- $$ = (Node *) n;
- }
- | MAXVALUE
- {
- PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
- n->kind = PARTITION_RANGE_DATUM_MAXVALUE;
- n->value = NULL;
- n->location = @1;
-
- $$ = (Node *) n;
- }
- | partbound_datum
- {
- PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
- n->kind = PARTITION_RANGE_DATUM_VALUE;
- n->value = $1;
- n->location = @1;
-
- $$ = (Node *) n;
- }
- ;
-
/*****************************************************************************
*
* ALTER TYPE
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 61727e1d71..55a5304a38 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -499,6 +499,13 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
else
err = _("grouping operations are not allowed in EXECUTE parameters");
+ break;
+ case EXPR_KIND_PARTITION_BOUND:
+ if (isAgg)
+ err = _("aggregate functions are not allowed in partition bound");
+ else
+ err = _("grouping operations are not allowed in partition bound");
+
break;
case EXPR_KIND_TRIGGER_WHEN:
if (isAgg)
@@ -899,6 +906,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("window functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("window functions are not allowed in partition bound");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("window functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 385e54a9b6..f8759f185b 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1849,6 +1849,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_CALL_ARGUMENT:
err = _("cannot use subquery in CALL argument");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("cannot use subquery in partition bound");
+ break;
/*
* There is intentionally no default: case here, so that the
@@ -3473,6 +3476,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "WHEN";
case EXPR_KIND_PARTITION_EXPRESSION:
return "PARTITION BY";
+ case EXPR_KIND_PARTITION_BOUND:
+ return "partition bound";
case EXPR_KIND_CALL_ARGUMENT:
return "CALL";
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index ea5d5212b4..570ae860ae 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2303,6 +2303,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_BOUND:
+ err = _("set-returning functions are not allowed in partition bound");
+ 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 c6f3628def..a948225bec 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,9 +139,12 @@ static void transformConstraintAttrs(CreateStmtContext *cxt,
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 transformPartitionRangeBounds(ParseState *pstate, List *blist);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod);
+static Node *boundValuePreColumnRefCheck(ParseState *pstate, ColumnRef *cref);
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid partCollation);
/*
@@ -3649,6 +3653,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
+ Oid partcollation;
if (spec->strategy != PARTITION_STRATEGY_LIST)
ereport(ERROR,
@@ -3668,17 +3673,19 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, 0);
coltypmod = get_partition_col_typmod(key, 0);
+ partcollation = get_partition_col_collation(key, 0);
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,
- colname, coltype, coltypmod);
+ value = transformPartitionBoundValue(pstate, expr,
+ colname, coltype, coltypmod,
+ partcollation);
/* Don't add to the result if the value is a duplicate */
duplicate = false;
@@ -3725,7 +3732,9 @@ transformPartitionBound(ParseState *pstate, Relation parent,
* Once we see MINVALUE or MAXVALUE for one column, the remaining
* columns must be the same.
*/
+ transformPartitionRangeBounds(pstate, spec->lowerdatums);
validateInfiniteBounds(pstate, spec->lowerdatums);
+ transformPartitionRangeBounds(pstate, spec->upperdatums);
validateInfiniteBounds(pstate, spec->upperdatums);
/* Transform all the constants */
@@ -3738,8 +3747,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
Const *value;
+ Oid partcollation;
/* Get the column's name in case we need to output an error */
if (key->partattrs[i] != 0)
@@ -3756,13 +3765,15 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, i);
coltypmod = get_partition_col_typmod(key, i);
+ partcollation = get_partition_col_collation(key, i);
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
- coltype, coltypmod);
+ coltype, coltypmod,
+ partcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3773,10 +3784,11 @@ 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);
+ coltype, coltypmod,
+ partcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3799,6 +3811,65 @@ transformPartitionBound(ParseState *pstate, Relation parent,
return result_spec;
}
+/*
+ * transformPartitionRangeBounds
+ * This converts the expressions for range partition bounds from the raw
+ * grammar representation to PartitionRangeDatum structs
+ */
+static void
+transformPartitionRangeBounds(ParseState *pstate, List *blist)
+{
+ ListCell *lc;
+
+ foreach(lc, blist)
+ {
+ Node *expr = lfirst(lc);
+ PartitionRangeDatum *prd = makeNode(PartitionRangeDatum);
+
+ /*
+ * Infinite range bounds -- "minvalue" and "maxvalue" -- get passed
+ * in as ColumnRefs.
+ */
+ if (IsA(expr, ColumnRef))
+ {
+ ColumnRef *cref = (ColumnRef *) expr;
+ char *cname;
+
+ if (list_length(cref->fields) == 1 &&
+ IsA(linitial(cref->fields), String))
+ cname = strVal(linitial(cref->fields));
+
+ if (strcmp("minvalue", cname) == 0)
+ {
+ prd->kind = PARTITION_RANGE_DATUM_MINVALUE;
+ prd->value = NULL;
+ prd->location = exprLocation(expr);
+ }
+ else if (strcmp("maxvalue", cname) == 0)
+ {
+ prd->kind = PARTITION_RANGE_DATUM_MAXVALUE;
+ prd->value = NULL;
+ prd->location = exprLocation(expr);
+ }
+ else
+ {
+ /* to issue ERROR for column reference */
+ boundValuePreColumnRefCheck(pstate, (ColumnRef *) expr);
+ /* not reaches here */
+ }
+ }
+ else
+ {
+ prd->kind = PARTITION_RANGE_DATUM_VALUE;
+ prd->value = expr;
+ prd->location = exprLocation(expr);
+ }
+
+ /* Use the existing List structure. */
+ lfirst(lc) = prd;
+ }
+}
+
/*
* validateInfiniteBounds
*
@@ -3839,17 +3910,38 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
}
}
+/*
+ * Column references are not allowed in partition bound values. Set this
+ * callback to error out them in transformExpr.
+ */
+static Node *
+boundValuePreColumnRefCheck(ParseState *pstate, ColumnRef *cref)
+{
+ ereport(ERROR,
+ (errcode (ERRCODE_SYNTAX_ERROR),
+ errmsg ("column reference is not allowed in bound value"),
+ parser_errposition(pstate, exprLocation((Node *) cref))));
+}
+
/*
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod)
+transformPartitionBoundValue(ParseState *pstate, Node *val,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid partCollation)
{
Node *value;
+ PreParseColumnRefHook oldhook = pstate->p_pre_columnref_hook;
- /* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ /* We reject column references within val */
+ pstate->p_pre_columnref_hook = boundValuePreColumnRefCheck;
+
+ /* Transform raw parsetree */
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUND);
+
+ /* Do we need to restore the hook? */
+ pstate->p_pre_columnref_hook = oldhook;
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3865,21 +3957,20 @@ 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 */
+ /* We ignore the value's collation */
if (!IsA(value, Const))
- value = (Node *) expression_planner((Expr *) value);
+ value = (Node *) eval_const_expressions(NULL, value);
- /* Fail if we don't have a constant (i.e., non-immutable coercion) */
+ /*
+ * If it is not a Const yet, Evaluate the expression applying given
+ * collation.
+ */
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)));
+ value = (Node *) evaluate_expr((Expr *) value, colType, colTypmod,
+ partCollation);
+ Assert(IsA(value, Const));
return (Const *) value;
}
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index ed854fdd40..06034d499b 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -88,4 +88,6 @@ extern Query *inline_set_returning_function(PlannerInfo *root,
extern List *expand_function_arguments(List *args, Oid result_type,
HeapTuple func_tuple);
+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 0230543810..68bec4b932 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -69,6 +69,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_BOUND, /* partition bounds value */
EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */
} ParseExprKind;
diff --git a/src/include/utils/partcache.h b/src/include/utils/partcache.h
index c1d029fdb3..105e76ced3 100644
--- a/src/include/utils/partcache.h
+++ b/src/include/utils/partcache.h
@@ -93,4 +93,10 @@ get_partition_col_typmod(PartitionKey key, int col)
return key->parttypmod[col];
}
+static inline Oid
+get_partition_col_collation(PartitionKey key, int col)
+{
+ return key->partcollation[col];
+}
+
#endif /* PARTCACHE_H */
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 470fca0cab..264cbcbefe 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -449,14 +449,6 @@ CREATE TABLE list_parted (
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');
- ^
-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);
- ^
-- 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 +482,8 @@ 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_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);
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 140bf41f76..5ba35038f5 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -432,8 +432,6 @@ CREATE TABLE list_parted (
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');
-CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
@@ -458,7 +456,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-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);
DROP TABLE moneyp;
-- immutable cast should work, though
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 763b4f573c..b6f3ddc22f 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -275,10 +275,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<para>
Each of the values specified in
the <replaceable class="parameter">partition_bound_spec</replaceable> is
- a literal, <literal>NULL</literal>, <literal>MINVALUE</literal>, or
- <literal>MAXVALUE</literal>. Each literal value must be either a
- numeric constant that is coercible to the corresponding partition key
- column's type, or a string literal that is valid input for that type.
+ any variable-free expression (subqueries and cross-references to other
+ columns in the current table are not allowed). The data type of the
+ partition bound expression must match the data type of the corresponding
+ partition key column.
</para>
<para>
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 505ae0af85..77ebb40ef2 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -150,8 +150,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,
@@ -4840,7 +4838,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 5a36367446..8b0680a7fa 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -581,8 +581,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 <list> hash_partbound partbound_datum_list range_datum_list
+%type <list> hash_partbound
%type <defelt> hash_partbound_elem
/*
@@ -2739,7 +2738,7 @@ PartitionBoundSpec:
}
/* a LIST partition */
- | FOR VALUES IN_P '(' partbound_datum_list ')'
+ | FOR VALUES IN_P '(' expr_list ')'
{
PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
@@ -2752,7 +2751,7 @@ PartitionBoundSpec:
}
/* a RANGE partition */
- | FOR VALUES FROM '(' range_datum_list ')' TO '(' range_datum_list ')'
+ | FOR VALUES FROM '(' expr_list ')' TO '(' expr_list ')'
{
PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
@@ -2795,59 +2794,6 @@ hash_partbound:
}
;
-partbound_datum:
- Sconst { $$ = makeStringConst($1, @1); }
- | NumericOnly { $$ = makeAConst($1, @1); }
- | TRUE_P { $$ = makeStringConst(pstrdup("true"), @1); }
- | FALSE_P { $$ = makeStringConst(pstrdup("false"), @1); }
- | NULL_P { $$ = makeNullAConst(@1); }
- ;
-
-partbound_datum_list:
- partbound_datum { $$ = list_make1($1); }
- | partbound_datum_list ',' partbound_datum
- { $$ = lappend($1, $3); }
- ;
-
-range_datum_list:
- PartitionRangeDatum { $$ = list_make1($1); }
- | range_datum_list ',' PartitionRangeDatum
- { $$ = lappend($1, $3); }
- ;
-
-PartitionRangeDatum:
- MINVALUE
- {
- PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
- n->kind = PARTITION_RANGE_DATUM_MINVALUE;
- n->value = NULL;
- n->location = @1;
-
- $$ = (Node *) n;
- }
- | MAXVALUE
- {
- PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
- n->kind = PARTITION_RANGE_DATUM_MAXVALUE;
- n->value = NULL;
- n->location = @1;
-
- $$ = (Node *) n;
- }
- | partbound_datum
- {
- PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
- n->kind = PARTITION_RANGE_DATUM_VALUE;
- n->value = $1;
- n->location = @1;
-
- $$ = (Node *) n;
- }
- ;
-
/*****************************************************************************
*
* ALTER TYPE
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 61727e1d71..55a5304a38 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -499,6 +499,13 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
else
err = _("grouping operations are not allowed in EXECUTE parameters");
+ break;
+ case EXPR_KIND_PARTITION_BOUND:
+ if (isAgg)
+ err = _("aggregate functions are not allowed in partition bound");
+ else
+ err = _("grouping operations are not allowed in partition bound");
+
break;
case EXPR_KIND_TRIGGER_WHEN:
if (isAgg)
@@ -899,6 +906,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("window functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("window functions are not allowed in partition bound");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("window functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 385e54a9b6..f8759f185b 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1849,6 +1849,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_CALL_ARGUMENT:
err = _("cannot use subquery in CALL argument");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("cannot use subquery in partition bound");
+ break;
/*
* There is intentionally no default: case here, so that the
@@ -3473,6 +3476,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "WHEN";
case EXPR_KIND_PARTITION_EXPRESSION:
return "PARTITION BY";
+ case EXPR_KIND_PARTITION_BOUND:
+ return "partition bound";
case EXPR_KIND_CALL_ARGUMENT:
return "CALL";
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index ea5d5212b4..570ae860ae 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2303,6 +2303,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_BOUND:
+ err = _("set-returning functions are not allowed in partition bound");
+ 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 c6f3628def..ba9170ce7e 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -48,7 +48,9 @@
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
+#include "optimizer/clauses.h"
#include "optimizer/planner.h"
+#include "optimizer/var.h"
#include "parser/analyze.h"
#include "parser/parse_clause.h"
#include "parser/parse_coerce.h"
@@ -138,9 +140,13 @@ static void transformConstraintAttrs(CreateStmtContext *cxt,
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 transformPartitionRangeBounds(ParseState *pstate, List *blist,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid partCollation);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod);
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid partCollation);
/*
@@ -3599,6 +3605,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
{
PartitionBoundSpec *result_spec;
PartitionKey key = RelationGetPartitionKey(parent);
+ RangeTblEntry *rte;
char strategy = get_partition_strategy(key);
int partnatts = get_partition_natts(key);
List *partexprs = get_partition_exprs(key);
@@ -3623,6 +3630,13 @@ transformPartitionBound(ParseState *pstate, Relation parent,
return result_spec;
}
+ /*
+ * Add RangeTblEntry for the relation. We assume this function is not
+ * called recursively.
+ */
+ rte = addRangeTableEntryForRelation(pstate, parent, NULL, false, true);
+ addRTEtoQuery(pstate, rte, true, true, true);
+
if (strategy == PARTITION_STRATEGY_HASH)
{
if (spec->strategy != PARTITION_STRATEGY_HASH)
@@ -3649,6 +3663,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
+ Oid partcollation;
if (spec->strategy != PARTITION_STRATEGY_LIST)
ereport(ERROR,
@@ -3668,17 +3683,19 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, 0);
coltypmod = get_partition_col_typmod(key, 0);
+ partcollation = get_partition_col_collation(key, 0);
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,
- colname, coltype, coltypmod);
+ value = transformPartitionBoundValue(pstate, expr,
+ colname, coltype, coltypmod,
+ partcollation);
/* Don't add to the result if the value is a duplicate */
duplicate = false;
@@ -3701,6 +3718,10 @@ transformPartitionBound(ParseState *pstate, Relation parent,
}
else if (strategy == PARTITION_STRATEGY_RANGE)
{
+ char *colname;
+ Oid coltype;
+ int32 coltypmod;
+ Oid partcollation;
ListCell *cell1,
*cell2;
int i,
@@ -3721,11 +3742,31 @@ transformPartitionBound(ParseState *pstate, Relation parent,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("TO must specify exactly one value per partitioning column")));
+ /* Get the only column's name in case we need to output an error */
+ if (key->partattrs[0] != 0)
+ colname = get_attname(RelationGetRelid(parent),
+ key->partattrs[0], false);
+ else
+ colname = deparse_expression((Node *) linitial(partexprs),
+ deparse_context_for(RelationGetRelationName(parent),
+ RelationGetRelid(parent)),
+ false, false);
+ /* Need its type data too */
+ coltype = get_partition_col_typid(key, 0);
+ coltypmod = get_partition_col_typmod(key, 0);
+ partcollation = get_partition_col_collation(key, 0);
+
/*
* Once we see MINVALUE or MAXVALUE for one column, the remaining
* columns must be the same.
*/
+ transformPartitionRangeBounds(pstate, spec->lowerdatums,
+ colname, coltype, coltypmod,
+ partcollation);
validateInfiniteBounds(pstate, spec->lowerdatums);
+ transformPartitionRangeBounds(pstate, spec->upperdatums,
+ colname, coltype, coltypmod,
+ partcollation);
validateInfiniteBounds(pstate, spec->upperdatums);
/* Transform all the constants */
@@ -3738,8 +3779,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
Const *value;
+ Oid partcollation;
/* Get the column's name in case we need to output an error */
if (key->partattrs[i] != 0)
@@ -3756,13 +3797,15 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, i);
coltypmod = get_partition_col_typmod(key, i);
+ partcollation = get_partition_col_collation(key, i);
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
- coltype, coltypmod);
+ coltype, coltypmod,
+ partcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3773,10 +3816,11 @@ 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);
+ coltype, coltypmod,
+ partcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3799,6 +3843,70 @@ transformPartitionBound(ParseState *pstate, Relation parent,
return result_spec;
}
+/*
+ * transformPartitionRangeBounds
+ * This converts the expressions for range partition bounds from the raw
+ * grammar representation to PartitionRangeDatum structs
+ */
+static void
+transformPartitionRangeBounds(ParseState *pstate, List *blist,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid partCollation)
+{
+ ListCell *lc;
+
+ foreach(lc, blist)
+ {
+ Node *expr = lfirst(lc);
+ PartitionRangeDatum *prd = makeNode(PartitionRangeDatum);
+ bool done = false;
+
+ /*
+ * Infinite range bounds -- "minvalue" and "maxvalue" -- get passed
+ * in as ColumnRefs.
+ */
+ if (IsA(expr, ColumnRef))
+ {
+ ColumnRef *cref = (ColumnRef *) expr;
+ char *cname;
+
+ if (list_length(cref->fields) == 1 &&
+ IsA(linitial(cref->fields), String))
+ cname = strVal(linitial(cref->fields));
+
+ if (strcmp("minvalue", cname) == 0)
+ {
+ prd->kind = PARTITION_RANGE_DATUM_MINVALUE;
+ prd->value = NULL;
+ prd->location = exprLocation(expr);
+ done = true;
+ }
+ else if (strcmp("maxvalue", cname) == 0)
+ {
+ prd->kind = PARTITION_RANGE_DATUM_MAXVALUE;
+ prd->value = NULL;
+ prd->location = exprLocation(expr);
+ done = true;
+ }
+ }
+
+ if (!done)
+ {
+ Const *value = transformPartitionBoundValue(pstate,
+ (Node *) expr,
+ colName, colType,
+ colTypmod,
+ partCollation);
+ prd->kind = PARTITION_RANGE_DATUM_VALUE;
+ prd->value = (Node *) value;
+ prd->location = exprLocation(expr);
+ }
+
+ /* Use the existing List structure. */
+ lfirst(lc) = prd;
+ }
+}
+
/*
* validateInfiniteBounds
*
@@ -3843,13 +3951,22 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod)
+transformPartitionBoundValue(ParseState *pstate, Node *val,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid partCollation)
{
Node *value;
- /* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ /* Transform raw parsetree */
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUND);
+
+ /* No column reference is allowed */
+ if (contain_var_clause(value))
+ ereport(ERROR,
+ (errcode (ERRCODE_SYNTAX_ERROR),
+ errmsg ("cannot use column referencees in bound value"),
+ parser_errposition(pstate, exprLocation((Node *) val))));
+
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3865,21 +3982,20 @@ 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 */
+ /* We ignore the value's collation */
if (!IsA(value, Const))
- value = (Node *) expression_planner((Expr *) value);
+ value = (Node *) eval_const_expressions(NULL, value);
- /* Fail if we don't have a constant (i.e., non-immutable coercion) */
+ /*
+ * If it is not a Const yet, Evaluate the expression applying given
+ * collation.
+ */
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)));
+ value = (Node *) evaluate_expr((Expr *) value, colType, colTypmod,
+ partCollation);
+ Assert(IsA(value, Const));
return (Const *) value;
}
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index ed854fdd40..06034d499b 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -88,4 +88,6 @@ extern Query *inline_set_returning_function(PlannerInfo *root,
extern List *expand_function_arguments(List *args, Oid result_type,
HeapTuple func_tuple);
+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 0230543810..68bec4b932 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -69,6 +69,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_BOUND, /* partition bounds value */
EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */
} ParseExprKind;
diff --git a/src/include/utils/partcache.h b/src/include/utils/partcache.h
index c1d029fdb3..105e76ced3 100644
--- a/src/include/utils/partcache.h
+++ b/src/include/utils/partcache.h
@@ -93,4 +93,10 @@ get_partition_col_typmod(PartitionKey key, int col)
return key->parttypmod[col];
}
+static inline Oid
+get_partition_col_collation(PartitionKey key, int col)
+{
+ return key->partcollation[col];
+}
+
#endif /* PARTCACHE_H */
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 470fca0cab..264cbcbefe 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -449,14 +449,6 @@ CREATE TABLE list_parted (
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');
- ^
-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);
- ^
-- 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 +482,8 @@ 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_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);
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 140bf41f76..5ba35038f5 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -432,8 +432,6 @@ CREATE TABLE list_parted (
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');
-CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
@@ -458,7 +456,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-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);
DROP TABLE moneyp;
-- immutable cast should work, though
Horiguchi-san, Thanks a lot for the review and sorry it took me a while to reply. Thought I'd refresh the patch as it's in the July CF. On 2018/04/24 18:08, Kyotaro HORIGUCHI wrote: > Thanks. I have almost missed this. > > At Mon, 23 Apr 2018 11:44:14 +0900, Amit Langote wrote: >> On 2018/04/23 11:37, Amit Langote wrote: >>> I tried to update the patch to do things that way. I'm going to create a >>> new entry in the next CF titled "generalized expression syntax for >>> partition bounds" and add the patch there. >> >> Tweaked the commit message to credit all the authors. > > + any variable-free expression (subqueries, window functions, aggregate > + functions, and set-returning functions are not allowed). The data type > + of the partition bound expression must match the data type of the > + corresponding partition key column. > > Parititioning value is slimiar to column default expression in > the sense that it appeas within a DDL. The description for > DEFAULT is: > > | The value is any variable-free expression (subqueries and > | cross-references to other columns in the current table are not > | allowed) > > It actually refuses aggregates, window functions and SRFs but it > is not mentioned. Whichever we choose, they ought to have the > similar description. Actually, I referenced the default value expression syntax a lot when working on this issue, so agree that there's a close resemblance. Maybe, we should fix the description for default expression to say that it can't contain subqueries, cross-references to other columns in the table, aggregate expressions, window functions, and set-returning functions. I also sent a patch separately: https://www.postgresql.org/message-id/2059e8f2-e6e6-7a9f-0de8-11eed291e641@lab.ntt.co.jp >> /* >> * Strip any top-level COLLATE clause, as they're inconsequential. >> * XXX - Should we add a WARNING here? >> */ >> while (IsA(value, CollateExpr)) >> value = (Node *) ((CollateExpr *) value)->arg; > > Ok, I'll follow Tom's suggestion but collate is necessarilly > appears in this shape. For example ('a' collate "de_DE") || 'b') > has the collate node under the top ExprOp and we get a complaint > like following with it. > >> ERROR: unrecognized node type: 123 > > 123 is T_CollateExpr. The expression "value" (mmm..) reuires > eval_const_expressions to eliminate CollateExprs. It requires > assign_expr_collations to retrieve value's collation but we don't > do that since we ignore collation this in this case. Ah yes, it seems better to call eval_const_expressions as a first step to get rid of CollateExpr's, followed by evaluate_expr if the first step didn't return a Const node. > The following results in a strange-looking error. > >> =# create table pt1 partition of p for values in (a); >> ERROR: column "a" does not exist >> LINE 1: create table pt1 partition of p for values in (a); > > The p/pt1 actually have a column a. > > The following results in similar error and it is wrong, too. > >> =# create table pr1 partition of pr for values from (a + 1) to (a + 2); >> ERROR: column "a" does not exist >> LINE 1: create table pr1 partition of pr for values from (a + 1) to ... This one is better fixed by initializing ParseState properly as demonstrated in your v3-2 patch. > Being similar but a bit different, the following command leads to > a SEGV since it creates PARTITION_RANGE_DATUM_VALUE with value = > NULL. Even if it leaves the original node, validateInfiniteBounds > rejects it and aborts. > >> =# create table pr1 partition of pr for values from (hoge) to (hige); > (crash) Oops. > I fixed this using pre-columnref hook in the attached v3. This > behavles for columnrefs differently than DEFAULT. > > The v3-2 does the same thing with DEFAULT. The behevior differs > whether the column exists or not. > >> ERROR: cannot use column referencees in bound value >> ERROR: column "b" does not exist > > I personally think that such similarity between DEFALUT and > partition bound (v3-2) is not required. But inserting the hook > (v3) doesn't look good for me. Actually, I too like the solution of v3-2 better, instead of using the hook, so I adopted it in the updated patch. I also changed how range bounds are processed in transformPartitionBound, moving some code into newly added transformPartitionRangeBounds to reduce code duplication. Updated patch attached. Thanks, Amit
Вложения
Hello.
cf-bot compained on this but I wondered why it got so many
errors. At the first look I found a spare semicolon before a bare
then calause:p
> - if (!IsA(value, Const));
> + if (!IsA(value, Const))
> elog(ERROR, "could not evaluate partition bound expression");
The attached is the fixed and rebsaed version.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From bbf76ee4a185133e625be88bf0784f2bc308772b Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 6 Jul 2018 14:05:22 +0900
Subject: [PATCH] Allow generalized expression syntax for partition bounds
Authors: Kyotaro Horiguchi, Tom Lane, Amit Langote
---
doc/src/sgml/ref/alter_table.sgml | 6 +-
doc/src/sgml/ref/create_table.sgml | 16 +--
src/backend/commands/tablecmds.c | 8 ++
src/backend/optimizer/util/clauses.c | 4 +-
src/backend/parser/gram.y | 60 +--------
src/backend/parser/parse_agg.c | 10 ++
src/backend/parser/parse_expr.c | 5 +
src/backend/parser/parse_func.c | 3 +
src/backend/parser/parse_utilcmd.c | 201 ++++++++++++++++++-----------
src/include/optimizer/clauses.h | 2 +
src/include/parser/parse_node.h | 1 +
src/include/utils/partcache.h | 6 +
src/test/regress/expected/create_table.out | 49 ++++---
src/test/regress/sql/create_table.sql | 16 ++-
14 files changed, 221 insertions(+), 166 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1cce00eaf9..aa70beeb32 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -87,9 +87,9 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
<phrase>and <replaceable class="parameter">partition_bound_spec</replaceable> is:</phrase>
-IN ( { <replaceable class="parameter">numeric_literal</replaceable> | <replaceable
class="parameter">string_literal</replaceable>| TRUE | FALSE | NULL } [, ...] ) |
-FROM ( { <replaceable class="parameter">numeric_literal</replaceable> | <replaceable
class="parameter">string_literal</replaceable>| TRUE | FALSE | MINVALUE | MAXVALUE } [, ...] )
- TO ( { <replaceable class="parameter">numeric_literal</replaceable> | <replaceable
class="parameter">string_literal</replaceable>| TRUE | FALSE | MINVALUE | MAXVALUE } [, ...] ) |
+IN ( <replaceable class="parameter">partition_bound_expr</replaceable> [, ...] ) |
+FROM ( { <replaceable class="parameter">partition_bound_expr</replaceable> | MINVALUE | MAXVALUE } [, ...] )
+ TO ( { <replaceable class="parameter">partition_bound_expr</replaceable> | MINVALUE | MAXVALUE } [, ...] ) |
WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REMAINDER <replaceable
class="parameter">numeric_literal</replaceable>)
<phrase>and <replaceable class="parameter">column_constraint</replaceable> is:</phrase>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 2a1eac9592..fcbabc78ff 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -86,9 +86,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
<phrase>and <replaceable class="parameter">partition_bound_spec</replaceable> is:</phrase>
-IN ( { <replaceable class="parameter">numeric_literal</replaceable> | <replaceable
class="parameter">string_literal</replaceable>| TRUE | FALSE | NULL } [, ...] ) |
-FROM ( { <replaceable class="parameter">numeric_literal</replaceable> | <replaceable
class="parameter">string_literal</replaceable>| TRUE | FALSE | MINVALUE | MAXVALUE } [, ...] )
- TO ( { <replaceable class="parameter">numeric_literal</replaceable> | <replaceable
class="parameter">string_literal</replaceable>| TRUE | FALSE | MINVALUE | MAXVALUE } [, ...] ) |
+IN ( <replaceable class="parameter">partition_bound_expr</replaceable> [, ...] ) |
+FROM ( { <replaceable class="parameter">partition_bound_expr</replaceable> | MINVALUE | MAXVALUE } [, ...] )
+ TO ( { <replaceable class="parameter">partition_bound_expr</replaceable> | MINVALUE | MAXVALUE } [, ...] ) |
WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REMAINDER <replaceable
class="parameter">numeric_literal</replaceable>)
<phrase><replaceable class="parameter">index_parameters</replaceable> in <literal>UNIQUE</literal>, <literal>PRIMARY
KEY</literal>,and <literal>EXCLUDE</literal> constraints are:</phrase>
@@ -273,12 +273,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</para>
<para>
- Each of the values specified in
- the <replaceable class="parameter">partition_bound_spec</replaceable> is
- a literal, <literal>NULL</literal>, <literal>MINVALUE</literal>, or
- <literal>MAXVALUE</literal>. Each literal value must be either a
- numeric constant that is coercible to the corresponding partition key
- column's type, or a string literal that is valid input for that type.
+ <replaceable class="parameter">partition_bound_expr</replaceable> is
+ any variable-free expression (subqueries, window functions, aggregate
+ functions, and set-returning functions are not allowed). Its data type
+ must match the data type of the corresponding partition key column.
</para>
<para>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7c0cf0d7ee..fc24e6fa2b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -796,6 +796,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
defaultPartOid;
Relation parent,
defaultRel = NULL;
+ RangeTblEntry *rte;
/* Already have strong enough lock on the parent */
parent = heap_open(parentId, NoLock);
@@ -838,6 +839,13 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
pstate = make_parsestate(NULL);
pstate->p_sourcetext = queryString;
+ /*
+ * Add an RTE containing this relation, so that transformExpr called
+ * on partition bound expressions is able to report errors using a
+ * proper context.
+ */
+ rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, false);
+ addRTEtoQuery(pstate, rte, false, true, true);
bound = transformPartitionBound(pstate, parent, stmt->partbound);
/*
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 505ae0af85..77ebb40ef2 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -150,8 +150,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,
@@ -4840,7 +4838,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 90dfac2cb1..183622fe44 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -581,8 +581,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 <list> hash_partbound partbound_datum_list range_datum_list
+%type <list> hash_partbound
%type <defelt> hash_partbound_elem
/*
@@ -2739,7 +2738,7 @@ PartitionBoundSpec:
}
/* a LIST partition */
- | FOR VALUES IN_P '(' partbound_datum_list ')'
+ | FOR VALUES IN_P '(' expr_list ')'
{
PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
@@ -2752,7 +2751,7 @@ PartitionBoundSpec:
}
/* a RANGE partition */
- | FOR VALUES FROM '(' range_datum_list ')' TO '(' range_datum_list ')'
+ | FOR VALUES FROM '(' expr_list ')' TO '(' expr_list ')'
{
PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
@@ -2795,59 +2794,6 @@ hash_partbound:
}
;
-partbound_datum:
- Sconst { $$ = makeStringConst($1, @1); }
- | NumericOnly { $$ = makeAConst($1, @1); }
- | TRUE_P { $$ = makeStringConst(pstrdup("true"), @1); }
- | FALSE_P { $$ = makeStringConst(pstrdup("false"), @1); }
- | NULL_P { $$ = makeNullAConst(@1); }
- ;
-
-partbound_datum_list:
- partbound_datum { $$ = list_make1($1); }
- | partbound_datum_list ',' partbound_datum
- { $$ = lappend($1, $3); }
- ;
-
-range_datum_list:
- PartitionRangeDatum { $$ = list_make1($1); }
- | range_datum_list ',' PartitionRangeDatum
- { $$ = lappend($1, $3); }
- ;
-
-PartitionRangeDatum:
- MINVALUE
- {
- PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
- n->kind = PARTITION_RANGE_DATUM_MINVALUE;
- n->value = NULL;
- n->location = @1;
-
- $$ = (Node *) n;
- }
- | MAXVALUE
- {
- PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
- n->kind = PARTITION_RANGE_DATUM_MAXVALUE;
- n->value = NULL;
- n->location = @1;
-
- $$ = (Node *) n;
- }
- | partbound_datum
- {
- PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
- n->kind = PARTITION_RANGE_DATUM_VALUE;
- n->value = $1;
- n->location = @1;
-
- $$ = (Node *) n;
- }
- ;
-
/*****************************************************************************
*
* ALTER TYPE
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 61727e1d71..55a5304a38 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -499,6 +499,13 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
else
err = _("grouping operations are not allowed in EXECUTE parameters");
+ break;
+ case EXPR_KIND_PARTITION_BOUND:
+ if (isAgg)
+ err = _("aggregate functions are not allowed in partition bound");
+ else
+ err = _("grouping operations are not allowed in partition bound");
+
break;
case EXPR_KIND_TRIGGER_WHEN:
if (isAgg)
@@ -899,6 +906,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("window functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("window functions are not allowed in partition bound");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("window functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 385e54a9b6..f8759f185b 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1849,6 +1849,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_CALL_ARGUMENT:
err = _("cannot use subquery in CALL argument");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("cannot use subquery in partition bound");
+ break;
/*
* There is intentionally no default: case here, so that the
@@ -3473,6 +3476,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "WHEN";
case EXPR_KIND_PARTITION_EXPRESSION:
return "PARTITION BY";
+ case EXPR_KIND_PARTITION_BOUND:
+ return "partition bound";
case EXPR_KIND_CALL_ARGUMENT:
return "CALL";
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index abe1dbc521..5190b628bd 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2348,6 +2348,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_BOUND:
+ err = _("set-returning functions are not allowed in partition bound");
+ 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 17b54b20cc..a94cf863c1 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -48,7 +48,9 @@
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
+#include "optimizer/clauses.h"
#include "optimizer/planner.h"
+#include "optimizer/var.h"
#include "parser/analyze.h"
#include "parser/parse_clause.h"
#include "parser/parse_coerce.h"
@@ -138,9 +140,12 @@ static void transformConstraintAttrs(CreateStmtContext *cxt,
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 List *transformPartitionRangeBounds(ParseState *pstate, List *blist,
+ Relation parent);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod);
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid partCollation);
/*
@@ -3649,6 +3654,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
+ Oid partcollation;
if (spec->strategy != PARTITION_STRATEGY_LIST)
ereport(ERROR,
@@ -3668,17 +3674,19 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, 0);
coltypmod = get_partition_col_typmod(key, 0);
+ partcollation = get_partition_col_collation(key, 0);
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,
- colname, coltype, coltypmod);
+ value = transformPartitionBoundValue(pstate, expr,
+ colname, coltype, coltypmod,
+ partcollation);
/* Don't add to the result if the value is a duplicate */
duplicate = false;
@@ -3701,11 +3709,6 @@ transformPartitionBound(ParseState *pstate, Relation parent,
}
else if (strategy == PARTITION_STRATEGY_RANGE)
{
- ListCell *cell1,
- *cell2;
- int i,
- j;
-
if (spec->strategy != PARTITION_STRATEGY_RANGE)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
@@ -3722,23 +3725,78 @@ transformPartitionBound(ParseState *pstate, Relation parent,
errmsg("TO must specify exactly one value per partitioning column")));
/*
- * Once we see MINVALUE or MAXVALUE for one column, the remaining
- * columns must be the same.
+ * Convert raw parser nodes into PartitionRangeDatum nodes and perform
+ * any necessary validation.
*/
- validateInfiniteBounds(pstate, spec->lowerdatums);
- validateInfiniteBounds(pstate, spec->upperdatums);
+ result_spec->lowerdatums =
+ transformPartitionRangeBounds(pstate, spec->lowerdatums,
+ parent);
+ result_spec->upperdatums =
+ transformPartitionRangeBounds(pstate, spec->upperdatums,
+ parent);
+ }
+ else
+ elog(ERROR, "unexpected partition strategy: %d", (int) strategy);
- /* Transform all the constants */
- i = j = 0;
- result_spec->lowerdatums = result_spec->upperdatums = NIL;
- forboth(cell1, spec->lowerdatums, cell2, spec->upperdatums)
+ return result_spec;
+}
+
+/*
+ * transformPartitionRangeBounds
+ * This converts the expressions for range partition bounds from the raw
+ * grammar representation to PartitionRangeDatum structs
+ */
+static List *
+transformPartitionRangeBounds(ParseState *pstate, List *blist,
+ Relation parent)
+{
+ List *result = NIL;
+ PartitionKey key = RelationGetPartitionKey(parent);
+ List *partexprs = get_partition_exprs(key);
+ ListCell *lc;
+ int i,
+ j;
+
+ i = j = 0;
+ foreach(lc, blist)
+ {
+ Node *expr = lfirst(lc);
+ PartitionRangeDatum *prd = NULL;
+
+ /*
+ * Infinite range bounds -- "minvalue" and "maxvalue" -- get passed
+ * in as ColumnRefs.
+ */
+ if (IsA(expr, ColumnRef))
+ {
+ ColumnRef *cref = (ColumnRef *) expr;
+ char *cname = NULL;
+
+ if (list_length(cref->fields) == 1 &&
+ IsA(linitial(cref->fields), String))
+ cname = strVal(linitial(cref->fields));
+
+ Assert(cname != NULL);
+ if (strcmp("minvalue", cname) == 0)
+ {
+ prd = makeNode(PartitionRangeDatum);
+ prd->kind = PARTITION_RANGE_DATUM_MINVALUE;
+ prd->value = NULL;
+ }
+ else if (strcmp("maxvalue", cname) == 0)
+ {
+ prd = makeNode(PartitionRangeDatum);
+ prd->kind = PARTITION_RANGE_DATUM_MAXVALUE;
+ prd->value = NULL;
+ }
+ }
+
+ if (prd == NULL)
{
- PartitionRangeDatum *ldatum = (PartitionRangeDatum *) lfirst(cell1);
- PartitionRangeDatum *rdatum = (PartitionRangeDatum *) lfirst(cell2);
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
+ Oid partcollation;
Const *value;
/* Get the column's name in case we need to output an error */
@@ -3753,50 +3811,38 @@ transformPartitionBound(ParseState *pstate, Relation parent,
false, false);
++j;
}
+
/* Need its type data too */
coltype = get_partition_col_typid(key, i);
coltypmod = get_partition_col_typmod(key, i);
+ partcollation = get_partition_col_collation(key, i);
- if (ldatum->value)
- {
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
- colname,
- coltype, coltypmod);
- if (value->constisnull)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("cannot specify NULL in range bound")));
- ldatum = copyObject(ldatum); /* don't scribble on input */
- ldatum->value = (Node *) value;
- }
-
- if (rdatum->value)
- {
- con = castNode(A_Const, rdatum->value);
- value = transformPartitionBoundValue(pstate, con,
- colname,
- coltype, coltypmod);
- if (value->constisnull)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("cannot specify NULL in range bound")));
- rdatum = copyObject(rdatum); /* don't scribble on input */
- rdatum->value = (Node *) value;
- }
-
- result_spec->lowerdatums = lappend(result_spec->lowerdatums,
- ldatum);
- result_spec->upperdatums = lappend(result_spec->upperdatums,
- rdatum);
-
+ value = transformPartitionBoundValue(pstate, expr,
+ colname,
+ coltype, coltypmod,
+ partcollation);
+ if (value->constisnull)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot specify NULL in range bound")));
+ prd = makeNode(PartitionRangeDatum);
+ prd->kind = PARTITION_RANGE_DATUM_VALUE;
+ prd->value = (Node *) value;
++i;
}
- }
- else
- elog(ERROR, "unexpected partition strategy: %d", (int) strategy);
- return result_spec;
+ prd->location = exprLocation(expr);
+
+ result = lappend(result, prd);
+ }
+
+ /*
+ * Once we see MINVALUE or MAXVALUE for one column, the remaining
+ * columns must be the same.
+ */
+ validateInfiniteBounds(pstate, result);
+
+ return result;
}
/*
@@ -3845,13 +3891,14 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod)
+transformPartitionBoundValue(ParseState *pstate, Node *val,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid partCollation)
{
Node *value;
- /* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ /* Transform raw parsetree */
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUND);
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3867,21 +3914,29 @@ 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 */
+ /* We effectively ignore the value's collation. */
if (!IsA(value, Const))
- value = (Node *) expression_planner((Expr *) value);
+ value = (Node *) eval_const_expressions(NULL, value);
- /* Fail if we don't have a constant (i.e., non-immutable coercion) */
- if (!IsA(value, Const))
+ /*
+ * Make sure default expr does not refer to any vars (we need this check
+ * since the pstate includes the target table).
+ */
+ if (contain_var_clause(value))
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)));
+ (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+ errmsg("cannot use column references in partition bound expression")));
+
+ /*
+ * If it is still not a Const, evaluate the expression applying the
+ * partition key's collation.
+ */
+ value = (Node *) evaluate_expr((Expr *) value, colType, colTypmod,
+ partCollation);
+ if (!IsA(value, Const))
+ elog(ERROR, "could not evaluate partition bound expression");
return (Const *) value;
}
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index ed854fdd40..06034d499b 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -88,4 +88,6 @@ extern Query *inline_set_returning_function(PlannerInfo *root,
extern List *expand_function_arguments(List *args, Oid result_type,
HeapTuple func_tuple);
+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 0230543810..27398dd0b8 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -69,6 +69,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_BOUND, /* partition bound value */
EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */
} ParseExprKind;
diff --git a/src/include/utils/partcache.h b/src/include/utils/partcache.h
index 873c60fafd..dffe561ebf 100644
--- a/src/include/utils/partcache.h
+++ b/src/include/utils/partcache.h
@@ -93,4 +93,10 @@ get_partition_col_typmod(PartitionKey key, int col)
return key->parttypmod[col];
}
+static inline Oid
+get_partition_col_collation(PartitionKey key, int col)
+{
+ return key->partcollation[col];
+}
+
#endif /* PARTCACHE_H */
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 672719e5d5..4202594c73 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -444,19 +444,40 @@ DROP TABLE partitioned, partitioned2;
CREATE TABLE list_parted (
a int
) PARTITION BY LIST (a);
--- syntax allows only string literal, numeric literal and null to be
--- specified for a partition bound value
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_3 PARTITION OF list_parted FOR VALUES IN ((2+1));
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');
- ^
-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);
- ^
+\d+ list_parted
+ Table "public.list_parted"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ a | integer | | | | plain | |
+Partition key: LIST (a)
+Partitions: part_1 FOR VALUES IN (1),
+ part_2 FOR VALUES IN (2),
+ part_3 FOR VALUES IN (3),
+ part_null FOR VALUES IN (NULL)
+
+-- forbidden expressions for partition bound
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN (somename);
+ERROR: column "somename" does not exist
+LINE 1: ...expr_fail PARTITION OF list_parted FOR VALUES IN (somename);
+ ^
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN (a);
+ERROR: cannot use column references in partition bound expression
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN (sum(a));
+ERROR: aggregate functions are not allowed in partition bound
+LINE 1: ...s_expr_fail PARTITION OF list_parted FOR VALUES IN (sum(a));
+ ^
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN ((select 1));
+ERROR: cannot use subquery in partition bound
+LINE 1: ...expr_fail PARTITION OF list_parted FOR VALUES IN ((select 1)...
+ ^
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN (generate_series(4, 6));
+ERROR: set-returning functions are not allowed in partition bound
+LINE 1: ...expr_fail PARTITION OF list_parted FOR VALUES IN (generate_s...
+ ^
-- 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 +511,8 @@ 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_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);
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 78944950fe..30ad89b864 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -427,13 +427,18 @@ DROP TABLE partitioned, partitioned2;
CREATE TABLE list_parted (
a int
) PARTITION BY LIST (a);
--- syntax allows only string literal, numeric literal and null to be
--- specified for a partition bound value
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_3 PARTITION OF list_parted FOR VALUES IN ((2+1));
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');
-CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
+\d+ list_parted
+
+-- forbidden expressions for partition bound
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN (somename);
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN (a);
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN (sum(a));
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN ((select 1));
+CREATE TABLE part_bogus_expr_fail PARTITION OF list_parted FOR VALUES IN (generate_series(4, 6));
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
@@ -458,7 +463,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-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);
DROP TABLE moneyp;
-- immutable cast should work, though
--
2.16.3
Horiguchi-san, On 2018/07/06 14:26, Kyotaro HORIGUCHI wrote: > Hello. > > cf-bot compained on this but I wondered why it got so many > errors. At the first look I found a spare semicolon before a bare > then calause:p > >> - if (!IsA(value, Const)); >> + if (!IsA(value, Const)) >> elog(ERROR, "could not evaluate partition bound expression"); > > The attached is the fixed and rebsaed version. Thanks for taking care of that. Regards, Amit
Rebased due to change in addRangeTableEntryForRelation's API. Thanks, Amit
Вложения
On 2018/11/09 14:38, Amit Langote wrote: > Rebased due to change in addRangeTableEntryForRelation's API. Rebased again due to changes in psql's describe output for partitioned tables. Thanks, Amit
Вложения
On 26/11/2018 05:58, Amit Langote wrote:
> On 2018/11/09 14:38, Amit Langote wrote:
>> Rebased due to change in addRangeTableEntryForRelation's API.
>
> Rebased again due to changes in psql's describe output for partitioned tables.
Review:
Is "partition bound" the right term? For list partitioning, it's not
really a bound. Maybe it's OK.
Keep the ordering of EXPR_KIND_PARTITION_BOUND in the various switch
statements consistent.
I don't see any treatment of non-immutable expressions. There is a test
case with a non-immutable cast that was removed, but that's all. There
was considerable discussion earlier in the thread on whether
non-immutable expressions should be allowed. I'm not sure what the
resolution was, but in any case there should be documentation and tests
of the outcome.
The collation handling might need another look. The following is
allowed without any diagnostic:
CREATE TABLE t2 (
a text COLLATE "POSIX"
) PARTITION BY RANGE (a);
CREATE TABLE t2a PARTITION OF t2 FOR VALUES FROM ('foo' COLLATE "C") TO
('xyz');
I think the correct behavior would be that an explicit collation in the
partition bound expression is an error.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Thanks for the review and sorry it took me a while to reply. On 2019/01/02 22:58, Peter Eisentraut wrote: > On 26/11/2018 05:58, Amit Langote wrote: >> On 2018/11/09 14:38, Amit Langote wrote: >>> Rebased due to change in addRangeTableEntryForRelation's API. >> >> Rebased again due to changes in psql's describe output for partitioned tables. > > Review: > > Is "partition bound" the right term? For list partitioning, it's not > really a bound. Maybe it's OK. Hmm, maybe "partition bound value"? Just want to stress that the expression specifies "bounding" value of a partition. > Keep the ordering of EXPR_KIND_PARTITION_BOUND in the various switch > statements consistent. OK, fixed. > I don't see any treatment of non-immutable expressions. There is a test > case with a non-immutable cast that was removed, but that's all. There > was considerable discussion earlier in the thread on whether > non-immutable expressions should be allowed. I'm not sure what the > resolution was, but in any case there should be documentation and tests > of the outcome. The partition bound expression is evaluated only once, that is, when the partition creation command is executed, and what gets stored in the catalog is a Const expression that contains the value that was computed, not the original non-immutable expression. So, we don't need to do anything special in terms of code to handle users possibly specifying a non-immutable expression as partition bound. Tom said something in that thread which seems to support such a position: https://www.postgresql.org/message-id/22534.1523374457%40sss.pgh.pa.us Part of the test case that was removed is the error that used to be emitted: 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_10 PARTITION OF moneyp FOR VALUES IN ('10'); which is no longer emitted, because the patched transformPartitionBoundValue evaluates the expression, instead of emitting error because the expression hasn't become a Const even after applying eval_const_expressions(). Do we need to mention any aspect of how this now works in the user-facing documentation? > The collation handling might need another look. The following is > allowed without any diagnostic: > > CREATE TABLE t2 ( > a text COLLATE "POSIX" > ) PARTITION BY RANGE (a); > CREATE TABLE t2a PARTITION OF t2 FOR VALUES FROM ('foo' COLLATE "C") TO > ('xyz'); > > I think the correct behavior would be that an explicit collation in the > partition bound expression is an error. I tend to agree with that. What happens is the partition bound expression is assigned the collation of the parent's partition key: + partcollation = get_partition_col_collation(key, 0); + value = transformPartitionBoundValue(pstate, expr, + colname, coltype, coltypmod, + partcollation); But that's essentially ignoring the collation specified by the user for the partition bound value without providing any information about that to the user. I've fixed that by explicitly checking if the collate clause in the partition bound value expression contains the same collation as the parent partition key collation and give an error otherwise. Updated patch attached. Thanks, Amit
Вложения
On 15/01/2019 07:31, Amit Langote wrote: >> Is "partition bound" the right term? For list partitioning, it's not >> really a bound. Maybe it's OK. > > Hmm, maybe "partition bound value"? Just want to stress that the > expression specifies "bounding" value of a partition. I was more concerned about the term "bound", which it is not range partitioning. But I can't think of a better term. I wouldn't change expr to value as you have done between v7 and v8, since the point of this patch is to make expressions valid where previously only values were. >> I don't see any treatment of non-immutable expressions. There is a test >> case with a non-immutable cast that was removed, but that's all. There >> was considerable discussion earlier in the thread on whether >> non-immutable expressions should be allowed. I'm not sure what the >> resolution was, but in any case there should be documentation and tests >> of the outcome. > > The partition bound expression is evaluated only once, that is, when the > partition creation command is executed, and what gets stored in the > catalog is a Const expression that contains the value that was computed, > not the original non-immutable expression. So, we don't need to do > anything special in terms of code to handle users possibly specifying a > non-immutable expression as partition bound. Tom said something in that > thread which seems to support such a position: > > https://www.postgresql.org/message-id/22534.1523374457%40sss.pgh.pa.us OK, if we are going with that approach, then it needs to be documented and there should be test cases. >> The collation handling might need another look. The following is >> allowed without any diagnostic: >> >> CREATE TABLE t2 ( >> a text COLLATE "POSIX" >> ) PARTITION BY RANGE (a); >> CREATE TABLE t2a PARTITION OF t2 FOR VALUES FROM ('foo' COLLATE "C") TO >> ('xyz'); >> >> I think the correct behavior would be that an explicit collation in the >> partition bound expression is an error. > But that's essentially ignoring the collation specified by the user for > the partition bound value without providing any information about that to > the user. I've fixed that by explicitly checking if the collate clause in > the partition bound value expression contains the same collation as the > parent partition key collation and give an error otherwise. I think that needs more refinement. In v8, the following errors CREATE TABLE t2 ( a name COLLATE "POSIX" ) PARTITION BY RANGE (a); CREATE TABLE t2a PARTITION OF t2 FOR VALUES FROM (name 'foo') TO (name 'xyz'); ERROR: collation of partition bound value for column "a" does not match partition key collation "POSIX" The problem here is that the "name" type has a collation that is neither the one of the column nor the default collation. We can allow that. What we don't want is someone writing an explicit COLLATE clause. I think we just need to check that there is no top-level COLLATE clause. This would then sort of match the logic parse_collate.c for combining collations. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Thanks for the review. On 2019/01/15 22:24, Peter Eisentraut wrote: > On 15/01/2019 07:31, Amit Langote wrote: >>> Is "partition bound" the right term? For list partitioning, it's not >>> really a bound. Maybe it's OK. >> >> Hmm, maybe "partition bound value"? Just want to stress that the >> expression specifies "bounding" value of a partition. > > I was more concerned about the term "bound", which it is not range > partitioning. But I can't think of a better term. > > I wouldn't change expr to value as you have done between v7 and v8, > since the point of this patch is to make expressions valid where > previously only values were. OK, will change it back to partition_bound_expr. Removing "bound" from it makes the term ambiguous? >>> I don't see any treatment of non-immutable expressions. There is a test >>> case with a non-immutable cast that was removed, but that's all. There >>> was considerable discussion earlier in the thread on whether >>> non-immutable expressions should be allowed. I'm not sure what the >>> resolution was, but in any case there should be documentation and tests >>> of the outcome. >> >> The partition bound expression is evaluated only once, that is, when the >> partition creation command is executed, and what gets stored in the >> catalog is a Const expression that contains the value that was computed, >> not the original non-immutable expression. So, we don't need to do >> anything special in terms of code to handle users possibly specifying a >> non-immutable expression as partition bound. Tom said something in that >> thread which seems to support such a position: >> >> https://www.postgresql.org/message-id/22534.1523374457%40sss.pgh.pa.us > > OK, if we are going with that approach, then it needs to be documented > and there should be test cases. How about the following note in the documentation: + Although volatile expressions such as + <literal><function>CURRENT_TIMESTAMP</function></literal> can be used + for this, be careful when using them, because + <productname>PostgreSQL</productname> will evaluate them only once + when adding the partition. Sorry but I'm not sure how or what I would test about this. Maybe, just add a test in create_table.sql/alter_table.sql that shows that using volatile expression doesn't cause an error? >>> The collation handling might need another look. The following is >>> allowed without any diagnostic: >>> >>> CREATE TABLE t2 ( >>> a text COLLATE "POSIX" >>> ) PARTITION BY RANGE (a); >>> CREATE TABLE t2a PARTITION OF t2 FOR VALUES FROM ('foo' COLLATE "C") TO >>> ('xyz'); >>> >>> I think the correct behavior would be that an explicit collation in the >>> partition bound expression is an error. > >> But that's essentially ignoring the collation specified by the user for >> the partition bound value without providing any information about that to >> the user. I've fixed that by explicitly checking if the collate clause in >> the partition bound value expression contains the same collation as the >> parent partition key collation and give an error otherwise. > > I think that needs more refinement. In v8, the following errors > > CREATE TABLE t2 ( a name COLLATE "POSIX" ) PARTITION BY RANGE (a); > CREATE TABLE t2a PARTITION OF t2 FOR VALUES FROM (name 'foo') TO (name > 'xyz'); > ERROR: collation of partition bound value for column "a" does not match > partition key collation "POSIX" > > The problem here is that the "name" type has a collation that is neither > the one of the column nor the default collation. We can allow that. So, should the "name" type's collation should simply be discarded in favor of "POSIX" that's being used for partitioning? > What we don't want is someone writing an explicit COLLATE clause. I > think we just need to check that there is no top-level COLLATE clause. > This would then sort of match the logic parse_collate.c for combining > collations. Maybe I'm missing something, but isn't it OK to allow the COLLATE clause as long as it specifies the matching collation as the parent? So: CREATE TABLE t2b PARTITION OF t2 FOR VALUES FROM (name 'bar' collate "C") TO (name 'foo'); ERROR: collation of partition bound value for column "a" does not match partition key collation "POSIX" -- either of the following is ok CREATE TABLE t2b PARTITION OF t2 FOR VALUES FROM (name 'bar' collate "POSIX") TO (name 'foo'); CREATE TABLE t2b PARTITION OF t2 FOR VALUES FROM (name 'bar') TO (name 'foo'); Thanks, Amit
On 16/01/2019 08:41, Amit Langote wrote: > OK, will change it back to partition_bound_expr. Removing "bound" from it > makes the term ambiguous? Yeah, let's leave it in. > How about the following note in the documentation: > > + Although volatile expressions such as > + <literal><function>CURRENT_TIMESTAMP</function></literal> can be used > + for this, be careful when using them, because > + <productname>PostgreSQL</productname> will evaluate them only once > + when adding the partition. I don't think we have to phrase it in this warning way. Just say that volatile expressions are evaluated at the time of the DDL statement. > Sorry but I'm not sure how or what I would test about this. Maybe, just > add a test in create_table.sql/alter_table.sql that shows that using > volatile expression doesn't cause an error? Possibilities: Create a range partition with current_timestamp as the upper bound and then in a separate transaction insert current_timestamp and have it appear in the default partition. Or create list partition with session_user as one partition's value and then insert session_user and have it appear in that table. Or something like those. >> I think that needs more refinement. In v8, the following errors >> >> CREATE TABLE t2 ( a name COLLATE "POSIX" ) PARTITION BY RANGE (a); >> CREATE TABLE t2a PARTITION OF t2 FOR VALUES FROM (name 'foo') TO (name >> 'xyz'); >> ERROR: collation of partition bound value for column "a" does not match >> partition key collation "POSIX" >> >> The problem here is that the "name" type has a collation that is neither >> the one of the column nor the default collation. We can allow that. > > So, should the "name" type's collation should simply be discarded in favor > of "POSIX" that's being used for partitioning? In that specific case, yes, I think so. >> What we don't want is someone writing an explicit COLLATE clause. I >> think we just need to check that there is no top-level COLLATE clause. >> This would then sort of match the logic parse_collate.c for combining >> collations. > > Maybe I'm missing something, but isn't it OK to allow the COLLATE clause > as long as it specifies the matching collation as the parent? Yes, that should be OK. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Thanks for the comments. On 2019/01/18 16:48, Peter Eisentraut wrote: >> How about the following note in the documentation: >> >> + Although volatile expressions such as >> + <literal><function>CURRENT_TIMESTAMP</function></literal> can be used >> + for this, be careful when using them, because >> + <productname>PostgreSQL</productname> will evaluate them only once >> + when adding the partition. > > I don't think we have to phrase it in this warning way. Just say that > volatile expressions are evaluated at the time of the DDL statement. OK, then just this: + Even volatile expressions such as + <literal><function>CURRENT_TIMESTAMP</function></literal> can be used. >> Sorry but I'm not sure how or what I would test about this. Maybe, just >> add a test in create_table.sql/alter_table.sql that shows that using >> volatile expression doesn't cause an error? > > Possibilities: Create a range partition with current_timestamp as the > upper bound and then in a separate transaction insert current_timestamp > and have it appear in the default partition. Or create list partition > with session_user as one partition's value and then insert session_user > and have it appear in that table. Or something like those. OK, added a test that uses current_timestamp. >> So, should the "name" type's collation should simply be discarded in favor >> of "POSIX" that's being used for partitioning? > > In that specific case, yes, I think so. > >>> What we don't want is someone writing an explicit COLLATE clause. I >>> think we just need to check that there is no top-level COLLATE clause. >>> This would then sort of match the logic parse_collate.c for combining >>> collations. >> >> Maybe I'm missing something, but isn't it OK to allow the COLLATE clause >> as long as it specifies the matching collation as the parent? > > Yes, that should be OK. Alright, I've included a test that uses cast expression in partition bound. Updated patch attached. Thanks, Amit
Вложения
Hello. At Fri, 18 Jan 2019 17:50:56 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <cbe3823f-f61e-1f37-c9ee-a3de9d8d565e@lab.ntt.co.jp> > Thanks for the comments. > > On 2019/01/18 16:48, Peter Eisentraut wrote: > >> How about the following note in the documentation: > >> > >> + Although volatile expressions such as > >> + <literal><function>CURRENT_TIMESTAMP</function></literal> can be used > >> + for this, be careful when using them, because > >> + <productname>PostgreSQL</productname> will evaluate them only once > >> + when adding the partition. > > > > I don't think we have to phrase it in this warning way. Just say that > > volatile expressions are evaluated at the time of the DDL statement. > > OK, then just this: > > + Even volatile expressions such as > + <literal><function>CURRENT_TIMESTAMP</function></literal> can be used. I agree to not to phrase in a warning way, but it seems too-simplified. I think the reason is still required, like this?: === The expression is evaluated once at the table creation time so it can involve even volatile expressions such as <literal><function>CURRENT_TIMESTAMP</function></literal>. === > >> Sorry but I'm not sure how or what I would test about this. Maybe, just > >> add a test in create_table.sql/alter_table.sql that shows that using > >> volatile expression doesn't cause an error? > > > > Possibilities: Create a range partition with current_timestamp as the > > upper bound and then in a separate transaction insert current_timestamp > > and have it appear in the default partition. Or create list partition > > with session_user as one partition's value and then insert session_user > > and have it appear in that table. Or something like those. > > OK, added a test that uses current_timestamp. > > >> So, should the "name" type's collation should simply be discarded in favor > >> of "POSIX" that's being used for partitioning? > > > > In that specific case, yes, I think so. > > > >>> What we don't want is someone writing an explicit COLLATE clause. I > >>> think we just need to check that there is no top-level COLLATE clause. > >>> This would then sort of match the logic parse_collate.c for combining > >>> collations. > >> > >> Maybe I'm missing something, but isn't it OK to allow the COLLATE clause > >> as long as it specifies the matching collation as the parent? > > > > Yes, that should be OK. > > Alright, I've included a test that uses cast expression in partition bound. > > Updated patch attached. -- Kyotaro Horiguchi NTT Open Source Software Center
Horiguchi-san, Thanks for checking. On 2019/01/24 19:03, Kyotaro HORIGUCHI wrote: > At Fri, 18 Jan 2019 17:50:56 +0900, Amit Langote wrote: >> On 2019/01/18 16:48, Peter Eisentraut wrote: >>>> How about the following note in the documentation: >>>> >>>> + Although volatile expressions such as >>>> + <literal><function>CURRENT_TIMESTAMP</function></literal> can be used >>>> + for this, be careful when using them, because >>>> + <productname>PostgreSQL</productname> will evaluate them only once >>>> + when adding the partition. >>> >>> I don't think we have to phrase it in this warning way. Just say that >>> volatile expressions are evaluated at the time of the DDL statement. >> >> OK, then just this: >> >> + Even volatile expressions such as >> + <literal><function>CURRENT_TIMESTAMP</function></literal> can be used. > > I agree to not to phrase in a warning way, but it seems > too-simplified. I think the reason is still required, like this?: > > === > The expression is evaluated once at the table creation time so it > can involve even volatile expressions such as > <literal><function>CURRENT_TIMESTAMP</function></literal>. Ah, that's perhaps a better way of describing this feature. Attached rebased patch containing above change. Thanks, Amit
Вложения
Why did you lose the parser_errposition in parse_utilcmd.c line 3854?
> - /* Fail if we don't have a constant (i.e., non-immutable coercion) */
> - if (!IsA(value, Const))
> + /* Make sure the expression does not refer to any vars. */
> + if (contain_var_clause(value))
> 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)));
> + (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
> + errmsg("cannot use column references in partition bound expression")));
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
Thanks for looking.
On 2019/01/24 21:00, Alvaro Herrera wrote:
> Why did you lose the parser_errposition in parse_utilcmd.c line 3854?
>
>> - /* Fail if we don't have a constant (i.e., non-immutable coercion) */
>> - if (!IsA(value, Const))
>> + /* Make sure the expression does not refer to any vars. */
>> + if (contain_var_clause(value))
>> 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)));
>> + (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
>> + errmsg("cannot use column references in partition bound expression")));
The if (contain_var_clause(value)) block is new code, but I agree its
ereport should have parser_errposition just like other ereports in that
function. Fixed that in the attached.
Thanks,
Amit
Вложения
On 24/01/2019 13:57, Amit Langote wrote: > The if (contain_var_clause(value)) block is new code, but I agree its > ereport should have parser_errposition just like other ereports in that > function. Fixed that in the attached. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> committed
Some of the buildfarm members are having sort-ordering problems
with this. Looks like you could work around it with different
partition names (don't assume the relative sort order of
letters and digits).
regards, tom lane
On Sat, Jan 26, 2019 at 12:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > > committed > > Some of the buildfarm members are having sort-ordering problems > with this. Looks like you could work around it with different > partition names (don't assume the relative sort order of > letters and digits). How about replacing \d+ list_parted with couple of \d on individual partitions, like in the attached? Thanks, Amit
Вложения
On Sat, Jan 26, 2019 at 03:14:51AM +0900, Amit Langote wrote: > How about replacing \d+ list_parted with couple of \d on individual > partitions, like in the attached? That would make it. Why just part_1 and part_3 though? It looks more complete to add part_null and part_2 as well. -- Michael
Вложения
On Sat, Jan 26, 2019 at 03:14:51AM +0900, Amit Langote wrote:
> How about replacing \d+ list_parted with couple of \d on individual
> partitions, like in the attached?
That would make it. Why just part_1 and part_3 though? It looks more
complete to add part_null and part_2 as well.
The describe lines are there just to show that the stored expessions are not verbatim same as the input expressions, so it seemed an overkill to add them for all of the partitions.
Thanks,
Amit
PS: away from a computer, typing on the smartphone
On Sat, Jan 26, 2019 at 12:14:33PM +0900, Amit Langote wrote: > The describe lines are there just to show that the stored expessions are > not verbatim same as the input expressions, so it seemed an overkill to add > them for all of the partitions. I see, so per 7c079d7 this is the reason why showing part_3 matters because you want to show the result of the expression after executing the DDL, and this has been just added: +CREATE TABLE part_3 PARTITION OF list_parted FOR VALUES IN ((2+1)); I think that it would be a good thing to show at least the NULL partition because its partition definition has semantics different from the three others so as it replaces part_1. What do you think? -- Michael
Вложения
On 25/01/2019 16:19, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> committed > > Some of the buildfarm members are having sort-ordering problems > with this. Looks like you could work around it with different > partition names (don't assume the relative sort order of > letters and digits). Fixed by light renaming. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Peter, On 2019/01/26 17:25, Peter Eisentraut wrote: > On 25/01/2019 16:19, Tom Lane wrote: >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>> committed >> >> Some of the buildfarm members are having sort-ordering problems >> with this. Looks like you could work around it with different >> partition names (don't assume the relative sort order of >> letters and digits). > > Fixed by light renaming. Thank you for committing and taking care of BF failures. Regards, Amit