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

On Tue, Mar 26, 2019 at 10:03:35AM -0400, Tom Lane wrote:
> +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.

ParseExprKind is an enum, so listing all the options without the
default has the advantage to generate a warning if somebody adds a
value. This way anybody changing this code will need to think about
it.

> I don't see that as an issue.

Thanks!

> 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.

Okay, I think I see your point here. That sounds sensible.
--
Michael