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
signature.asc
Description: PGP signature
