Re: WIP patch for consolidating misplaced-aggregate checks

I wrote:
> I find it fairly annoying though that parseCheckAggregates (and likewise
> parseCheckWindowFuncs) have to dig through previously parsed query trees
> to look for misplaced aggregates; so adding even more of that is grating
> on me. It would be a lot cleaner if transformAggregateCall and
> transformWindowFuncCall could throw these errors immediately. The
> reason they can't is lack of context about what portion of the query we
> are currently parsing. I'm thinking it'd be worthwhile to add an enum
> field to ParseState that shows whether we're currently parsing the
> associated query level's target list, WHERE clause, GROUP BY clause,
> etc. The easiest way to ensure this gets set for all cases should be to
> add the enum value as another argument to transformExpr(), which
> would then save it into the ParseState for access by subsidiary
> expression transformation functions.

I've been fooling around with this concept, and attach a draft patch
that I'm not terribly satisfied with yet. It's not making the code
any shorter, although it holds some promise of being able to do that
if we push the concept even further. There are some judgment calls
and things that need discussion; in particular I need some input from
people who do message translation work.

In the attached patch, I invented an EXPR_KIND_XXX enum value for each
part of a SELECT/INSERT/UPDATE/DELETE query that we could want to
distinguish, but lumped all utility-statement cases into one
EXPR_KIND_UTILITY entry. That means the utility-statement callers still
all need to do their own checking/reporting of disallowed aggregates and
window functions, so that they can produce appropriate messages.
I'm tempted to enlarge the set of EXPR_KINDs to include cases for
utility statements so that those checks can be moved too; but that would
make the enum much more in need of continuing maintenance in the future.
On the other hand, if we did that then we could also push the
responsibility for rejecting sub-selects (which practically all of those
callers also do) into transformSubLink; which seems like a nice thing.

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.

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?

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 had hoped that we might be able to get rid of after-the-fact
rechecking of parse trees entirely, and thereby get rid of
contain_aggs_of_level and locate_agg_of_level; which would go a long way
towards making this a net code savings. It turns out that this is
basically impossible for the case of detecting nested aggregates,
because you have to parse-analyze an aggregate's arguments (so as to
determine their datatypes) before you can look up the function and
discover that it is indeed an aggregate. The other place where it's
problematic is "select max(x) from ... group by 1". That has to be
rejected, but at the time you're parsing max(x) you only know it's a
targetlist entry, so it looks fine. The GROUP BY code has to go back
and recheck when accepting a GROUP BY reference to an existing tlist
entry. But I did manage to get rid of parse tree rescans in other
places. Also, it turned out that we were searching an aggregate's
arguments up to *three* times: once to look for variables to determine
its semantic level, once to look for nested aggregates, and once to look
for nested window functions. So I got rid of find_minimum_var_level,
which never proved to have any other use anyway, and replaced it with a
dedicated function check_agg_arguments that does all that work in one
pass. So even though this isn't any smaller, it should be a bit faster
than the existing code, at least for complex queries.

One other thing worth mentioning is that this exercise has so far
revealed two bugs:

1. The parser fails to detect nested outer aggregates, for instance
(with un-patched code):

regression=# select (select max(max(q1))) from int8_tbl;
ERROR: aggregate function calls cannot be nested

That looks like it's all right, but where's the error cursor? If you
turn on VERBOSE you find that actually that's coming out of the planner,
which is being paranoid and rechecking for possible nesting. If
somebody were to remove that check or reduce it to an Assert, we'd have
a problem. Since the backstop is there in existing branches, this
doesn't seem to need a back-patch, but if we don't apply some form of
the attached patch I think we need to fix this some other way in HEAD.

2. We forgot to add a no-window-functions check to DefineIndex, so
the executor fails instead:

regression=# create index fooey on int8_tbl (max(q1) over ());
ERROR: WindowFunc found in non-WindowAgg plan node

Again, I don't feel a strong need to back-patch a fix, but we'd want to
repair this oversight in HEAD.

Thoughts, opinions, better ideas?

regards, tom lane

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Aug 9, 2012 at 11:56 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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.

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

Fair enough. I was not sold on doing that either. I would still like
to know if it's okay to use one string with %s for the cases where
there's not a good reason for the "context" to be more than just a
SQL keyword. That would save a few lines of code and also reduce
the number of strings for translators to deal with; so if it's not
horrid from a translation-quality standpoint, it seems worth doing.

One other thing I forgot to mention is that there was already some use
of error messages with a "constructName" string in almost the same
places, and in fact in the draft patch transformWhereClause and
transformLimitClause have *both* exprKind and constructName arguments,
which is kind of ugly and redundant. The reason I left the
constructName arguments in place is that currently they're fed to
coerce_to_boolean and/or coerce_to_specific_type, and it did not seem
very practical to convert those two functions to use exprKind instead.
The problem is that they are also used from within expression trees,
with constructNames like "AND", "OR", "NOT". We don't want the exprKind
to change when descending into AND sub-clauses for instance, so I didn't
see a good way to unify those usages with the overall clause type.
Anybody have an idea for improvement there?

regards, tom lane

Excerpts from Tom Lane's message of jue ago 09 12:40:08 -0400 2012:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Thu, Aug 9, 2012 at 11:56 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> 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.
>
> > 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.
>
> Fair enough. I was not sold on doing that either. I would still like
> to know if it's okay to use one string with %s for the cases where
> there's not a good reason for the "context" to be more than just a
> SQL keyword. That would save a few lines of code and also reduce
> the number of strings for translators to deal with; so if it's not
> horrid from a translation-quality standpoint, it seems worth doing.

Yes, that part seems to work fine -- at least I haven't seen any
translator complaining.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Here's an updated version taking into account the discussion so far.
It's still a net addition of code (about +200 lines according to
diffstat), but I think the consolidation of logic is probably worth
that.

Any further comments?

regards, tom lane

On Thu, Aug 9, 2012 at 5:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fair enough. I was not sold on doing that either. I would still like
> to know if it's okay to use one string with %s for the cases where
> there's not a good reason for the "context" to be more than just a
> SQL keyword.

Given that the SQL keyword is going to be an English word I can't
imagine how this could be a big deal for translators. It might not
match gender or case or something but only if the reader is
automatically mentally translating the keyword into their language and
then applying that language's rules to it. At least to me it makes
sense to refer to "VALUES" as a singular noun or "LIMIT" as a generic
male noun even though "limitation" would be feminine (I had to look
that one up though so perhaps I'm not the best person to judge).

--
greg

Dear sender,
I'm away from the office till 19th of August. In the meantime, please contact my colleague Alfred Vater at alfred(dot)vater(at)aegaeon(dot)de

Excerpts from Greg Stark's message of vie ago 10 12:57:25 -0400 2012:
> On Thu, Aug 9, 2012 at 5:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Fair enough. I was not sold on doing that either. I would still like
> > to know if it's okay to use one string with %s for the cases where
> > there's not a good reason for the "context" to be more than just a
> > SQL keyword.
>
> Given that the SQL keyword is going to be an English word I can't
> imagine how this could be a big deal for translators. It might not
> match gender or case or something but only if the reader is
> automatically mentally translating the keyword into their language and
> then applying that language's rules to it. At least to me it makes
> sense to refer to "VALUES" as a singular noun or "LIMIT" as a generic
> male noun even though "limitation" would be feminine (I had to look
> that one up though so perhaps I'm not the best person to judge).

Speaking of english words, I was wondering at "check" the other day.
For example, we have

#: catalog/heap.c:2171
#, c-format
msgid "check constraint \"%s\" already exists"

#: catalog/heap.c:2534
#, c-format
msgid "only table \"%s\" can be referenced in check constraint"

And so on (there are several more). Note that here we use "check
constraint" without any capitalization. However this doesn't translate
too well as is; I mean, if I were to translate "check" into its
equivalent spanish word, I'm sure to cause a great deal of confusion.
So I've opted for putting the check word, verbatim, in quotes; for
example:

msgid "check constraint \"%s\" already exists"
msgstr "la restricción «check» «%s» ya existe"

However this is also a bit ugly because I now have two sets of quoted
words -- check itself and then the constraint name.

If we were to have CHECK in uppercase, this would be easy:

msgid "check constraint \"%s\" already exists"
msgstr "la restricción CHECK «%s» ya existe"

Maybe I should just do that -- uppercase the keyword instead of sticking
it in quotes.

(As for the gender of limit, in spanish, you'd probably think of "el
límite" which is masculine. But in general, I agree with you: I think
it makes sense to keep the key word in english in the error message.)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Speaking of english words, I was wondering at "check" the other day.
> For example, we have

> #: catalog/heap.c:2171
> #, c-format
> msgid "check constraint \"%s\" already exists"

> #: catalog/heap.c:2534
> #, c-format
> msgid "only table \"%s\" can be referenced in check constraint"

> And so on (there are several more). Note that here we use "check
> constraint" without any capitalization.

FWIW, I think I changed "check" to "CHECK" in a couple of messages
recently, for exactly the reason that it seemed to be used in its
keyword meaning rather than as plain English text. Perhaps we
should just go around and do that consistently.

regards, tom lane

On Fri, 2012-08-10 at 17:57 +0100, Greg Stark wrote:
> On Thu, Aug 9, 2012 at 5:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Fair enough. I was not sold on doing that either. I would still like
> > to know if it's okay to use one string with %s for the cases where
> > there's not a good reason for the "context" to be more than just a
> > SQL keyword.
>
> Given that the SQL keyword is going to be an English word I can't
> imagine how this could be a big deal for translators. It might not
> match gender or case or something but only if the reader is
> automatically mentally translating the keyword into their language and
> then applying that language's rules to it. At least to me it makes
> sense to refer to "VALUES" as a singular noun or "LIMIT" as a generic
> male noun even though "limitation" would be feminine (I had to look
> that one up though so perhaps I'm not the best person to judge).

In some languages the grammatical adjustments do not depend on the
gender but on other aspects of the word, such as the last letter. So
it's possible to construct cases where this is a problem.

That said, you can usually work around this by translating something
like "cannot use aggregates with %s" along the lines of "cannot use
aggregates in context %s".

On Tue, 2012-08-14 at 12:04 -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Speaking of english words, I was wondering at "check" the other day.
> > For example, we have
>
> > #: catalog/heap.c:2171
> > #, c-format
> > msgid "check constraint \"%s\" already exists"
>
> > #: catalog/heap.c:2534
> > #, c-format
> > msgid "only table \"%s\" can be referenced in check constraint"
>
> > And so on (there are several more). Note that here we use "check
> > constraint" without any capitalization.
>
> FWIW, I think I changed "check" to "CHECK" in a couple of messages
> recently, for exactly the reason that it seemed to be used in its
> keyword meaning rather than as plain English text. Perhaps we
> should just go around and do that consistently.

I'm not in favor of that. "Check constraint" is a database term that
exists outside of SQL, just like "primary key", for instance. You
wouldn't write the latter in all upper case everywhere, I think.

On Tue, 2012-08-14 at 11:30 -0400, Alvaro Herrera wrote:
> And so on (there are several more). Note that here we use "check
> constraint" without any capitalization. However this doesn't
> translate
> too well as is; I mean, if I were to translate "check" into its
> equivalent spanish word, I'm sure to cause a great deal of confusion.
> So I've opted for putting the check word, verbatim, in quotes; for
> example:
>
> msgid "check constraint \"%s\" already exists"
> msgstr "la restricción «check» «%s» ya existe"
>
> However this is also a bit ugly because I now have two sets of quoted
> words -- check itself and then the constraint name.

I can't really advise you with the language issue, but I think just
writing check without quotes would be fine here. I also find it useful
on occasion to consult existing (non-PostgreSQL, non-MySQL) books about
databases in the respective language for typical translations.