Re: WIP patch for consolidating misplaced-aggregate checks

On Thu, Aug 9, 2012 at 11:56 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> At the moment, the patch faithfully preserves (well, 99% preserves)
> the current spellings of the error messages, so that no regression
> test entries change. Once all those messages were brought together,
> it became painfully obvious that we have been less than consistent
> about how to phrase them:
>
> errmsg("aggregates not allowed in JOIN conditions"),
> errmsg("aggregates not allowed in FROM clause"),
> errmsg("cannot use aggregate function in function expression in FROM"),
> errmsg("aggregates not allowed in WHERE clause"),
> errmsg("argument of RANGE must not contain aggregate functions"),
> errmsg("argument of ROWS must not contain aggregate functions"),
> errmsg("cannot use aggregate function in INSERT"),
> errmsg("cannot use aggregate function in UPDATE"),
> errmsg("aggregates not allowed in GROUP BY clause"),
> errmsg("argument of LIMIT must not contain aggregate functions"),
> errmsg("argument of OFFSET must not contain aggregate functions"),
> errmsg("cannot use aggregate function in RETURNING"),
> errmsg("cannot use aggregate function in VALUES"),
>
> (and that's not even counting the utility-statement callers, which I've
> not unified yet). I think obviously this should be fixed, but what
> phrasing shall we settle on? At the moment I'm leaning to "aggregate
> functions are not allowed in <context>", which is not exactly like any
> of these but seems better English.

I like that.

> Also, at least for the cases where the <context> is just a SQL keyword,
> it is mighty tempting to use just one message string, ie "aggregate
> functions are not allowed in %s". I know that we do this in assorted
> places, but I've never had a good grasp of exactly how painful that is
> for translators to deal with. Does anyone better-informed than me want
> to vote on whether to collapse the multiple messages like that?

I am not better informed on this point than you, so I'll pass on
expressing an opinion here.

> If we do go with the %s-for-a-SQL-keyword approach, it would then become
> tempting to force-fit all of the cases into that style. We would lose a
> certain amount of specificity of the messages if we did that; for
> example, "function expression in FROM" would probably become just
> "FROM", and messages that currently mention "index expression" or "index
> predicate" would probably say "CREATE INDEX". But I think this is all
> right considering we'd be producing an error cursor pointing at the
> aggregate or window function, which the utility-statement call sites
> generally don't produce at the moment.

I don't really like this, though. I don't think an error cursor is a
good substitute for a clear statement of the categorical rule; or to
put that another way, I think that forcing all of those messages into
this model is going to be awkward. In fact, I think even some of the
existing phrasing is vulnerable to improvement, e.g. cannot use
aggregate function in INSERT cannot possibly be true in general,
considering that INSERT clauses can have a WITH clause preceding them
and a SELECT following them. What is really meant is probably
something more like "aggregate functions are not allowed in the VALUES
portion of an INSERT statement" or something like that. Maybe it's OK
as-is, but I don't think we want to make that exact format into a
straight-jacket; it seems inevitable that there will be cases where
it's less clear than some more verbose explanatory text.

There will be a real improvement in maintainability from lining all of
these error messages up in a table rather than having them spread
around the source base, even if we allow some minor variation in the
message text.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company