Disallow outer-level and WHERE-clause aggregates in GRAPH_TABLE

Sami Imseih <samimseih@gmail.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

Thanks for the comments!

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,

I was not feeling totally convinced about this either, but was not
sure if the tree traversal is that big of a problem. We should
definitely avoid doing this, if we can.

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.

You are right. This makes sense to me now. We can introduce
EXPR_KIND_GRAPH_TABLE_COLUMNS and
EXPR_KIND_GRAPH_TABLE_WHERE ParseExprKind's.

Inside check_agglevels_and_constraints(), we can check the restriction
before we walk up to the query level the aggregate belongs to.

Also, transformWindowFuncCall() and check_srf_call_placement() should
do the same thing, meaning the work done in f58567105is effectively
reverted.

The attached patch does this. Is this what you have in mind?

--
Sami

v3 attached. No .c changes from v2.

Added tests for GROUPING and for an aggregate in the graph-pattern WHERE.
Also dropped the inner-reference element-pattern WHERE test, since it would
have passed under f585671055d1 and proved nothing. The outer-reference
element-pattern test was already present.

Also, I created a CF entry https://commitfest.postgresql.org/patch/7144/

--
Sami

Sami Imseih <samimseih@gmail.com> writes:

v3 attached. No .c changes from v2.

I looked this over. The general pattern of adding more EXPR_KIND
values looks fine, but I really didn't like the nonstandard way in
which you tested for GRAPH_TABLE in check_agglevels_and_constraints.
Testing before running up to the aggregate's semantic level is just
wrong: it would fail to reject an aggregate within a subquery within
one of these GRAPH_TABLE clauses. And if we're testing at some other
level than the semantic level, should we also reject GRAPH_TABLE
context at intermediate parse levels?

This was ugly enough that it motivated me to go look at exactly why
an outer aggregate doesn't work here, in hopes of removing the
inconsistent restriction. I found it: replace_property_refs_mutator,
which increments Vars' varlevelsup to account for the fact that
they're being pushed into a subquery, failed to do the equivalent
thing for Aggrefs and GroupingFuncs. (Compare, for instance,
IncrementVarSublevelsUp.) The case seems to Just Work after fixing
that, so I changed check_agglevels_and_constraints to enforce the
restriction in just the same way as it does for other clauses where
we disallow aggs.

As a minor improvement, you can just use the "errkind = true" option
in these error-reporting functions; less code, fewer strings for the
translators to deal with, same or better message wording.

v4 attached. I think this is committable if you don't see anything
else to change.

regards, tom lane

Thanks for v4, Tom!

This was ugly enough that it motivated me to go look at exactly why
an outer aggregate doesn't work here, in hopes of removing the
inconsistent restriction. I found it: replace_property_refs_mutator,
which increments Vars' varlevelsup to account for the fact that
they're being pushed into a subquery,

You are right. This is not something I considered. No reason
why an outer aggregate can't be used inside the GRAPH_TABLE.
The restriction only applies to the same-level aggregate,
which there's no machinery for.

I tested v4 with a mixed same-level and outer-level aggregate, and
this combination is not rejected as it should be.

Using the same EXISTS test from v4, none of these produce the clean
parse-time rejection we expect. The COLUMNS case is worse, it returns
a row instead of erroring. Under EXISTS the aggregate's value is never
required, so the incorrect same-level aggregate is not evaluated and
the bad query is accepted.

```
postgres=# SELECT EXISTS(SELECT num FROM GRAPH_TABLE (myshop MATCH (c
IS customers) COLUMNS (count(c.customer_id + o.customer_id) AS num))
t) FROM customers o;
exists
--------
t
(1 row)

postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE count(c.customer_id + o.customer_id) > 0) COLUMNS
(c.name AS nm)) t) FROM customers o;
ERROR: Upper-level Var found where not expected
postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE GROUPING(c.customer_id, o.customer_id) = 1) COLUMNS
(c.name AS nm)) t) FROM customers o GROUP BY customer_id;
ERROR: arguments to GROUPING must be grouping expressions of the
associated query level
```

So, based on your explanation earlier, this led me to find that the
leveling is still wrong. Specifically, min_varlevel > 0, since the
level-zero GraphPropertyRef is not accounted for in
check_agg_arguments_walker. In this case min_varlevel should be 0.

The fix is to teach check_agg_arguments_walker that a GraphPropertyRef
carries a level, the GRAPH_TABLE's own level. This is safe because a
GraphPropertyRef can only ever refer to its own GRAPH_TABLE's level
and can never appear inside a sub-select, so treating it as a level-zero
reference is always correct. Such an aggregate then resolves to the
GRAPH_TABLE's own level and the existing same-level rejection fires.
With that in place all of the cases above are rejected cleanly at
parse time.

```
postgres=# SELECT EXISTS(SELECT num FROM GRAPH_TABLE (myshop MATCH (c
IS customers) COLUMNS (count(c.customer_id + o.customer_id) AS num))
t) FROM customers o;
ERROR: aggregate functions are not allowed in GRAPH_TABLE COLUMNS
postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE count(c.customer_id + o.customer_id) > 0) COLUMNS
(c.name AS nm)) t) FROM customers o;
ERROR: aggregate functions are not allowed in GRAPH_TABLE WHERE
postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE GROUPING(c.customer_id, o.customer_id) = 1) COLUMNS
(c.name AS nm)) t) FROM customers o GROUP BY customer_id;
ERROR: grouping operations are not allowed in GRAPH_TABLE WHERE
```

v5 attached fixes the leveling problem, folded into your v4, with a
mixed-level test cases added.

What do you think?

--
Sami Imseih
Amazon Web Services (AWS)

Sami Imseih <samimseih@gmail.com> writes:

postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE count(c.customer_id + o.customer_id) > 0) COLUMNS
(c.name AS nm)) t) FROM customers o;
ERROR: Upper-level Var found where not expected

Ouch.

So, based on your explanation earlier, this led me to find that the
leveling is still wrong. Specifically, min_varlevel > 0, since the
level-zero GraphPropertyRef is not accounted for in
check_agg_arguments_walker. In this case min_varlevel should be 0.

Right, I came to the same conclusion.

The fix is to teach check_agg_arguments_walker that a GraphPropertyRef
carries a level, the GRAPH_TABLE's own level. This is safe because a
GraphPropertyRef can only ever refer to its own GRAPH_TABLE's level
and can never appear inside a sub-select, so treating it as a level-zero
reference is always correct.

I find that argument pretty shaky. If I can write
MATCH WHERE c.customer_id > 0
why can't I write
MATCH WHERE (SELECT c.customer_id > 0)
? I see that that in fact doesn't work, but that seems like a bug
in itself.

More generally, what this suggests to me is that we have a ton of
other bugs-of-omission in places that process Vars and don't know that
a GraphPropertyRef acts like a Var. So what I'm thinking right now is
that this is a fundamental design error, and that we should nuke
GraphPropertyRef altogether in favor of using a Var that references
the appropriate column of the RTE_GRAPH_TABLE relation. We have
learned the hard way in the past that you don't want to invent things
that act like Vars but aren't Vars; if you must, you are going to be
doing a heck of a lot of work to patch up everyplace that will need to
know about them. The hazard here is limited somewhat by the fact that
the planner and executor will never see GraphPropertyRefs, but I'm
afraid that still leaves plenty of scope for bugs (in the rewriter, in
particular).

regards, tom lane

I find that argument pretty shaky.

You are right. I was wrong to assume a GraphPropertyRef can only ever refer
to its own GRAPH_TABLE's level. Written inside a sub-select it does not.

If I can write
MATCH WHERE c.customer_id > 0
why can't I write
MATCH WHERE (SELECT c.customer_id > 0)
? I see that that in fact doesn't work, but that seems like a bug
in itself.

Disallowing subqueries in GRAPH_TABLE is the current intent of the
feature. The code
in upstream today only checks p_hasSubLinks at the end of
transformRangeGraphTable, after the WHERE and COLUMNS list is transformed, so
it only catches subqueries that transform cleanly, i.e. "(SELECT 1)" gets the
intended error, but "(SELECT c.customer_id > 0)" fails with "missing FROM-clause
entry"

The attached v6 does two things. First, it rejects a SubLink under the
GRAPH_TABLE
expr kinds in transformSubLink, the moment it is seen and before the
sub-select body
is analyzed. So MATCH WHERE (SELECT c.customer_id > 0) now reports

```
ERROR: subqueries within GRAPH_TABLE reference are not supported
```

instead of

```
ERROR: missing FROM-clause entry for table "c"
```

Also, because GRAPH_TABLE now rejects subqueries, a property reference
can
never sit inside one, so it is always at its own GRAPH_TABLE's level. That lets
check_agg_arguments_walker treat a GraphPropertyRef as a Var with
varlevelsup 0,
which it otherwise cannot see since a GraphPropertyRef is not a Var.
The walker still
adjusts for the level it is found at, the same as for a Var.

This corrects the aggregate case I showed earlier. The patch includes
regression tests
for it.

More generally, what this suggests to me is that we have a ton of
other bugs-of-omission in places that process Vars and don't know that
a GraphPropertyRef acts like a Var.

Agreed, and v6 is an example of this. It teaches check_agg_arguments_walker
that a GraphPropertyRef behaves like a Var.

Besides other potential bugs, It also limits the feature in the future.
Keeping subqueries out of GRAPH_TABLE is part of what makes some
of the bugs discovered fixable with the current GraphPropertyRef, but
I suspect it will be a real limitation if we try to relax these restrictions.

So what I'm thinking right now is
that this is a fundamental design error, and that we should nuke
GraphPropertyRef altogether in favor of using a Var that references
the appropriate column of the RTE_GRAPH_TABLE relation.

I spent time today on this and I think the direction is right. In what
I have running
locally, a property reference is emitted as a plain Var over the
RTE_GRAPH_TABLE relation,
so the Var handles leveling and the rest for free. GraphPropertyRef
does not go away
entirely, though. It moves onto a side list on the RTE that the
rewriter uses to resolve
each property Var back to its graph property.

The design has some open questions, but I will share if this is the direction
to go.

--
Sami Imseih
Amazon Web Services (AWS)