Hello,
At Fri, 02 Feb 2018 18:04:44 -0500, Tom Lane <[email protected]> wrote in
<[email protected]>
> Robert Haas <[email protected]> writes:
> > On Fri, Feb 2, 2018 at 4:40 PM, Peter Eisentraut
> > <[email protected]> wrote:
> >> There might be other options, but one way to solve this would be to
> >> treat partition bounds as a general expression in the grammar and then
> >> check in post-parse analysis that it's a constant.
>
> > Yeah -- isn't the usual way of handling this to run the user's input
> > through eval_const_expressions and see if the result is constant?
At Mon, 29 Jan 2018 13:21:54 +0900, Amit Langote
<[email protected]> wrote in
<[email protected]>
> Partition bound literals as captured gram.y don't have any type
> information attached. They're carried over in a A_Const to
> transformPartitionBoundValue() and coerced to the target partition key
> type there. Note that each of the the partition bound literal datums
> received from gram.y is castNode(A_Const)'d in parse_utilcmds.c.
eval_const_expressions is already running under
transformPartitionBoundValue to resolve remaining coercion. I
suppose we can use AexprConst to restrict the syntax within
appropriate variations. Please find the attached patch.
It allows the following syntax as a by-prodcut.
| create table c4 partition of t for values in (numeric(1,0) '5');
Parser accepts arbitrary defined types but it does no harm.
| create table c2 partition of t for values from (line '{0,1,0}') to (1);
| ERROR: specified value cannot be cast to type double precision for column "a"
It rejects unacceptable functions but the message may look
somewhat unfriendly.
| =# create table c1 partition of t for values in (random());
| ERROR: syntax error at or near ")"
| LINE 1: create table c1 partition of t for values in (random());
| ^
(marker is placed under the closing parenthesis of "random()")
| =# create table c1 partition of t for values in (random(0) 'x');
| ERROR: type "random" does not exist
| LINE 1: create table c1 partition of t for values in (random(0) 'x')...
(marker is placed under the first letter of the "random".)
> Not sure we want to go quite that far: at minimum it would imply invoking
> arbitrary stuff during a utility statement, which we generally try to
> avoid. Still, copy-from-query does that, so we can certainly make it
> work if we wish.
>
> Perhaps more useful to discuss: would that truly be the semantics we want,
> or should we just evaluate the expression and have done? It's certainly
> arguable that "IN (random())" ought to draw an error, not compute some
> random value and use that. But if you are insistent on partition bounds
> being immutable in any strong sense, you've already got problems, because
> e.g. a timestamptz literal's interpretation isn't necessarily fixed.
> It's only after we've reduced the original input to Datum form that we
> can make any real promises about the value not moving. So I'm not seeing
> where is the bright line between "IN ('today')" and "IN (random())".
>
> regards, tom lane
The patch leaves the ambiguity of values like 'today' but doesn't
accept arbitrary functions. Howerver, it needs additional message
for errors that never happen since the patch adds a new item in
ParseExprKind...
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5329432..c5d8526 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2790,9 +2790,7 @@ hash_partbound:
;
partbound_datum:
- Sconst { $$ = makeStringConst($1, @1); }
- | NumericOnly { $$ = makeAConst($1, @1); }
- | NULL_P { $$ = makeNullAConst(@1); }
+ AexprConst { $$ = $1; }
;
partbound_datum_list:
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 6a9f1b0..9bbe9b1 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -506,6 +506,12 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
else
err = _("grouping operations are not allowed in partition key expression");
+ case EXPR_KIND_PARTITION_BOUNDS:
+ if (isAgg)
+ err = _("aggregate functions are not allowed in partition bounds value");
+ else
+ err = _("grouping operations are not allowed in partition bounds value");
+
break;
case EXPR_KIND_CALL:
@@ -891,6 +897,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("window functions are not allowed in partition key expression");
break;
+ case EXPR_KIND_PARTITION_BOUNDS:
+ err = _("window functions are not allowed in partition bounds value");
+ break;
case EXPR_KIND_CALL:
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 b2f5e46..d1f9b02 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1846,6 +1846,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("cannot use subquery in partition key expression");
break;
+ case EXPR_KIND_PARTITION_BOUNDS:
+ err = _("cannot use subquery in partition bounds value");
+ break;
/*
* There is intentionally no default: case here, so that the
@@ -3468,6 +3471,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "WHEN";
case EXPR_KIND_PARTITION_EXPRESSION:
return "PARTITION BY";
+ case EXPR_KIND_PARTITION_BOUNDS:
+ return "partition bounds";
case EXPR_KIND_CALL:
return "CALL";
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index ffae0f3..9e83ffe 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2289,6 +2289,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("set-returning functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUNDS:
+ err = _("set-returning functions are not allowed in partition bounds value");
+ break;
case EXPR_KIND_CALL:
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 d415d71..0b05dda 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -134,7 +134,7 @@ static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
const char *colName, Oid colType, int32 colTypmod);
@@ -3420,12 +3420,12 @@ transformPartitionBound(ParseState *pstate, Relation parent,
result_spec->listdatums = NIL;
foreach(cell, spec->listdatums)
{
- A_Const *con = castNode(A_Const, lfirst(cell));
+ Node *expr = (Node *)lfirst (cell);
Const *value;
ListCell *cell2;
bool duplicate;
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate, expr,
colname, coltype, coltypmod);
/* Don't add to the result if the value is a duplicate */
@@ -3486,7 +3486,6 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
Const *value;
/* Get the column's name in case we need to output an error */
@@ -3507,8 +3506,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3521,8 +3520,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (rdatum->value)
{
- con = castNode(A_Const, rdatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ rdatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3591,13 +3590,13 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+transformPartitionBoundValue(ParseState *pstate, Node *val,
const char *colName, Oid colType, int32 colTypmod)
{
Node *value;
/* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUNDS);
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3613,7 +3612,7 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("specified value cannot be cast to type %s for column \"%s\"",
format_type_be(colType), colName),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
/* Simplify the expression, in case we had a coercion */
if (!IsA(value, Const))
@@ -3627,7 +3626,7 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
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)));
+ parser_errposition(pstate, exprLocation(val))));
return (Const *) value;
}
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 4e96fa7..23451cb 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -68,6 +68,7 @@ typedef enum ParseExprKind
EXPR_KIND_TRIGGER_WHEN, /* WHEN condition in CREATE TRIGGER */
EXPR_KIND_POLICY, /* USING or WITH CHECK expr in policy */
EXPR_KIND_PARTITION_EXPRESSION, /* PARTITION BY expression */
+ EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */
EXPR_KIND_CALL /* CALL argument */
} ParseExprKind;
