Re: Disallow outer-level and WHERE-clause aggregates in GRAPH_TABLE

Sami Imseih <samimseih(at)gmail(dot)com> writes:
> Commit f585671055d1 [1] disallowed aggregates, window functions, and SRFs in a
> GRAPH_TABLE COLUMNS list. For the aggregate case it tests pstate->p_hasAggs
> after transforming the columns. That is not enough. An aggregate that
> references an outer query is attributed to a parent query level, so
> check_agglevels_and_constraints sets p_hasAggs on that parent ParseState and
> not on the GRAPH_TABLE's own, so the aggregate is not caught.

It kind of seems like that should work, since an outer-level Agg is
effectively a constant within any execution of the lower query.
However, I'm content to say that making it work is out of scope for
v19.

> The attached patch closes both gaps by walking the transformed COLUMNS list and
> the graph pattern for Aggref and GroupingFunc nodes. Unlike p_hasAggs, the walk
> detects an aggregate by its presence in those the trees. Window functions
> and SRFs only mark the local ParseState, so their checks introduced in
> f585671055d1 remain in place.

I do not like this fix approach, and for that matter I don't like
f58567105. This is expensive thanks to the extra tree traversal,
f58567105 is user-unfriendly because it fails to say exactly what
or where is the construct it's rejecting, and neither patch is
following the perfectly good structure that the parser already
has for this kind of check. IMO the correct way to handle this
restriction is to check it in check_agglevels_and_constraints() based
on the ParseExprKind of the surrounding expression. It looks like the
graph transformation steps in the parser think they can get away with
using EXPR_KIND_WHERE and EXPR_KIND_SELECT_TARGET, but that's just
wrong if the constraints on these subexpressions are any different
from what they are for regular WHERE and SELECT targets. The right
way to go about this is to back-fill new ParseExprKind(s) as needed
and then make the necessary checks in a way similar to existing code.
Doing that might help you find other comparable oversights, too.

regards, tom lane