On 2019/03/13 14:15, Amit Langote wrote:
> On 2019/03/11 16:21, Michael Paquier wrote:
>> On Mon, Mar 11, 2019 at 03:44:39PM +0900, Amit Langote wrote:
>>> We could make the error message more meaningful depending on the context,
>>> but maybe it'd better be pursue it as a separate project.
>>
>> Yeah, I noticed that stuff when working on it this afternoon. The
>> error message does not completely feel right even in your produced
>> tests. Out of curiosity I have been working on this thing myself,
>> and it is possible to have a context-related message. Please see
>> attached, that's in my opinion less confusing, and of course
>> debatable. Still this approach does not feel completely right either
>> as that means hijacking the code path which generates a generic
>> message for missing RTEs. :(
>
> @@ -3259,6 +3259,9 @@ errorMissingRTE(ParseState *pstate, RangeVar
> + *
> + * Also, in the context of parsing a partition bound, produce a more
> + * helpful error message.
> */
> if (rte && rte->alias &&
> strcmp(rte->eref->aliasname, relation->relname) != 0 &&
>
>
> - if (rte)
> + if (pstate->p_expr_kind == EXPR_KIND_PARTITION_BOUND)
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_TABLE),
> + errmsg("invalid reference in partition bound
> expression for table
> \"%s\"",
> + relation->relname)));
> + else if (rte)
>
> Hmm, it seems odd to me that it's OK for default expressions to emit the
> "missing RTE" error, whereas partition bound expressions would emit this
> special error message?
>
> create table foo (a int default (bar.a));
> ERROR: missing FROM-clause entry for table "bar"
Looking into this a bit more, I wonder if it would be a good idea to to
have one of those error-emitting switch (pstate->p_expr_kind) blocks in
transformColumnRef(), as shown in the attached patch?
For example, the following error is emitted by one of such blocks that's
in transformSubLink():
create table foo (a int default (select * from non_existent_table));
ERROR: cannot use subquery in DEFAULT expression
However, if we decide to go with this approach, we will start getting
different error messages from what HEAD gives in certain cases. With the
patch, you will get the following error when trying to use an aggregate
function in for DEFAULT expression:
create table foo (a int default (avg(foo.a)));
ERROR: cannot use column reference in DEFAULT expression
but on HEAD, you get:
create table foo (a int default (avg(foo.a)));
ERROR: aggregate functions are not allowed in DEFAULT expressions
That's because, on HEAD, transformAggregateCall() (or something that calls
it) first calls transformColumnRef() to resolve 'foo.a', which checks that
foo.a is a valid column reference, but it doesn't concern itself with the
fact that the bigger expression it's part of is being used for DEFAULT
expression. It's only after 'foo.a' has been resolved as a valid column
reference that check_agglevels_and_constraints(), via
transformAggregateCall, emits an error that the overall expression is
invalid to use as a DEFAULT expression. With patches, error will be
emitted even before resolving 'foo.a'.
While transformAggregateCall() works like that, transformSubLink(), which
I mentioned in the first example, doesn't bother to analyze the query
first (select * from non_existent_table) to notice that the referenced
table doesn't exist. If it had bothered to analyze the query first, we
would've most likely gotten an error from errorMissingRTE(), not what we
get today. So, there are certainly some inconsistencies even today in how
these errors are emitted.
Thanks,
Amit
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index c7b5ff62f9..aa36d19117 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2939,18 +2939,9 @@ cookDefault(ParseState *pstate,
expr = transformExpr(pstate, raw_default, EXPR_KIND_COLUMN_DEFAULT);
/*
- * 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(expr))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("cannot use column references in
default expression")));
-
- /*
- * transformExpr() should have already rejected subqueries, aggregates,
- * window functions, and SRFs, based on the EXPR_KIND_ for a default
- * expression.
+ * transformExpr() should have already rejected column references,
+ * subqueries, aggregates, window functions, and SRFs, based on the
+ * EXPR_KIND_ for a default expression.
*/
/*
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index e559353529..6ce7a5e57e 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -520,6 +520,79 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
CRERR_WRONG_DB,
CRERR_TOO_MANY
} crerr = CRERR_NO_COLUMN;
+ const char *err;
+
+ /*
+ * Check to see if the column reference is in an invalid place within
the
+ * query. We allow column references in most places, except in default
+ * and partition bound expressions.
+ */
+ err = NULL;
+ switch (pstate->p_expr_kind)
+ {
+ case EXPR_KIND_NONE:
+ Assert(false); /* can't happen */
+ break;
+ case EXPR_KIND_OTHER:
+ case EXPR_KIND_JOIN_ON:
+ case EXPR_KIND_JOIN_USING:
+ case EXPR_KIND_FROM_SUBSELECT:
+ case EXPR_KIND_FROM_FUNCTION:
+ case EXPR_KIND_WHERE:
+ case EXPR_KIND_POLICY:
+ case EXPR_KIND_HAVING:
+ case EXPR_KIND_FILTER:
+ case EXPR_KIND_WINDOW_PARTITION:
+ case EXPR_KIND_WINDOW_ORDER:
+ case EXPR_KIND_WINDOW_FRAME_RANGE:
+ case EXPR_KIND_WINDOW_FRAME_ROWS:
+ case EXPR_KIND_WINDOW_FRAME_GROUPS:
+ case EXPR_KIND_SELECT_TARGET:
+ case EXPR_KIND_INSERT_TARGET:
+ case EXPR_KIND_UPDATE_SOURCE:
+ case EXPR_KIND_UPDATE_TARGET:
+ case EXPR_KIND_GROUP_BY:
+ case EXPR_KIND_ORDER_BY:
+ case EXPR_KIND_DISTINCT_ON:
+ case EXPR_KIND_LIMIT:
+ case EXPR_KIND_OFFSET:
+ case EXPR_KIND_RETURNING:
+ case EXPR_KIND_VALUES:
+ case EXPR_KIND_VALUES_SINGLE:
+ case EXPR_KIND_CHECK_CONSTRAINT:
+ case EXPR_KIND_DOMAIN_CHECK:
+ case EXPR_KIND_FUNCTION_DEFAULT:
+ case EXPR_KIND_INDEX_EXPRESSION:
+ case EXPR_KIND_INDEX_PREDICATE:
+ case EXPR_KIND_ALTER_COL_TRANSFORM:
+ case EXPR_KIND_EXECUTE_PARAMETER:
+ case EXPR_KIND_TRIGGER_WHEN:
+ case EXPR_KIND_PARTITION_EXPRESSION:
+ case EXPR_KIND_CALL_ARGUMENT:
+ case EXPR_KIND_COPY_WHERE:
+ /* okay */
+ break;
+
+ case EXPR_KIND_COLUMN_DEFAULT:
+ err = _("cannot use column reference in DEFAULT
expression");
+ break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("cannot use column reference in partition bound
expression");
+ break;
+
+ /*
+ * There is intentionally no default: case here, so
that the
+ * compiler will warn if we add a new ParseExprKind
without
+ * extending this switch. If we do see an unrecognized
value at
+ * runtime, the behavior will be the same as for
EXPR_KIND_OTHER,
+ * which is sane anyway.
+ */
+ }
+ if (err)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg_internal("%s", err),
+ parser_errposition(pstate, cref->location)));
/*
* Give the PreParseColumnRefHook, if any, first shot. If it returns
diff --git a/src/backend/parser/parse_utilcmd.c
b/src/backend/parser/parse_utilcmd.c
index a37d1f18be..c45b6d7caa 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3875,6 +3875,12 @@ transformPartitionBoundValue(ParseState *pstate, Node
*val,
value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUND);
/*
+ * transformExpr() should have already rejected column references,
+ * subqueries, aggregates, window functions, and SRFs, based on the
+ * EXPR_KIND_ for a default expression.
+ */
+
+ /*
* Check that the input expression's collation is compatible with one
* specified for the parent's partition key (partcollation). Don't
* throw an error if it's the default collation which we'll replace with
@@ -3914,13 +3920,6 @@ transformPartitionBoundValue(ParseState *pstate, Node
*val,
if (!IsA(value, Const))
value = (Node *) expression_planner((Expr *) value);
- /* Make sure the expression does not refer to any vars. */
- if (contain_var_clause(value))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("cannot use column references in
partition bound expression"),
- parser_errposition(pstate,
exprLocation(value))));
-
/*
* Evaluate the expression, assigning the partition key's collation to
the
* resulting Const expression.
diff --git a/src/test/regress/expected/create_table.out
b/src/test/regress/expected/create_table.out
index d51e547278..52cbc154b8 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -491,17 +491,17 @@ Partitions: 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
+ERROR: cannot use column reference in partition bound expression
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
+ERROR: cannot use column reference in partition bound expression
LINE 1: ..._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));
-ERROR: aggregate functions are not allowed in partition bound
+ERROR: cannot use column reference in partition bound expression
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)...
