Re: Boolean partitions syntax

Hello,

At Fri, 02 Feb 2018 18:04:44 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <14732.1517612684@sss.pgh.pa.us>
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Fri, Feb 2, 2018 at 4:40 PM, Peter Eisentraut
> > <peter.eisentraut@2ndquadrant.com> 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 <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<6844d7f9-8219-a9ff-88f9-82c05fc90d70@lab.ntt.co.jp>
> 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;