Re: Misleading errors with column references in default expressions and partition bounds

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Misleading errors with column references in default expressions and partition bounds
Date: 2019-03-26 14:03:35
Message-ID: 17662.1553609015@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> One idea which came from Amit, and it seems to me that it is a good
> idea, would be to have more context-related error messages directly in
> transformColumnRef(), so as we can discard at an early stage column
> references which are part of contexts where there is no meaning to
> have them.

+1 for the general idea, but I find the switch a bit overly verbose.
Do we really need to force every new EXPR_KIND to visit this spot,
when so few of them have a need to do anything? I'd be a bit inclined
to simplify it to

switch (pstate->p_expr_kind)
{
case EXPR_KIND_COLUMN_DEFAULT:
ereport(...);
break;
case EXPR_KIND_PARTITION_BOUND:
ereport(...);
break;
default:
break;
}

That's just a nitpick though.

> While this takes care of the RTE issues, this has a downside though.
> Take for example this case using an expression with an aggregate and
> a column reference:
> =# 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

I don't see that as an issue.

> The docs of CREATE TABLE also look incorrect to me when it comes to
> default expressions. It says the following: "other columns in the
> current table are not allowed". However *all* columns are not
> authorized, including the column which uses the expression.

I think the idea is that trying to reference another column is something
that people might well try to do, whereas referencing the DEFAULT's
own column is obviously silly. In particular the use of "cross-reference"
implies that another column is what is being referenced. If we dumb it
down to just "references to columns in the current table", then it's
consistent, but it's also completely redundant with the main part of the
sentence. It doesn't help that somebody decided to jam the independent
issue of subqueries into the same sentence. In short, maybe it'd be
better like this:

... The value
is any variable-free expression (in particular, cross-references
to other columns in the current table are not allowed). Subqueries
are not allowed either.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2019-03-26 14:16:00 Re: Shouldn't current_schema() be at least PARALLEL RESTRICTED?
Previous Message Dean Rasheed 2019-03-26 13:37:33 Re: [HACKERS] PATCH: multivariate histograms and MCV lists