ERROR: PlaceHolderVar found where not expected
I came across an ERROR as $subject with query below.
create table t (a int, b int);
create statistics if not exists t_s0 (dependencies, ndistinct) on a, b from
t;
insert into t values(1,1);
analyze t;
SELECT * FROM t LEFT JOIN (select true as c0) s0 ON true INNER JOIN (select
true as c0) s1 ON s0.c0 INNER JOIN (select true as c0) s2 ON s0.c0;
This is due to that we use 0 flags for pull_var_clause in
dependency_is_compatible_expression, assuming that the 'clause_expr'
cannot contain Aggrefs, WindowFuncs or PlaceHolderVars. This should be
an oversight as we can see that it's possible to find PHVs here. We can
fix it by
--- a/src/backend/statistics/dependencies.c
+++ b/src/backend/statistics/dependencies.c
@@ -1316,7 +1316,7 @@ dependency_is_compatible_expression(Node *clause,
Index relid, List *statlist, N
if (IsA(clause_expr, RelabelType))
clause_expr = (Node *) ((RelabelType *) clause_expr)->arg;- vars = pull_var_clause(clause_expr, 0);
+ vars = pull_var_clause(clause_expr, PVC_RECURSE_PLACEHOLDERS);But I'm not sure if Aggrefs and WindowFuncs are possible to be found
here.
This issue can be seen on 14, 15 and master.
Thanks
Richard
Richard Guo <guofenglinux@gmail.com> writes:
I came across an ERROR as $subject with query below.
...
This is due to that we use 0 flags for pull_var_clause in
dependency_is_compatible_expression, assuming that the 'clause_expr'
cannot contain Aggrefs, WindowFuncs or PlaceHolderVars. This should be
an oversight as we can see that it's possible to find PHVs here.
Nice catch.
But I'm not sure if Aggrefs and WindowFuncs are possible to be found
here.
WindowFuncs should be disallowed in qual clauses, so I think it's okay
to leave those flags out. An Aggref could occur in a HAVING qual though.
I'm not sure if this code could get applied to HAVING ... but it's
not immediately clear that it can't. I'd be inclined to add
PVC_RECURSE_AGGREGATES, as that seems more likely to be okay than not.
regards, tom lane
I wrote:
WindowFuncs should be disallowed in qual clauses, so I think it's okay
to leave those flags out. An Aggref could occur in a HAVING qual though.
I'm not sure if this code could get applied to HAVING ... but it's
not immediately clear that it can't. I'd be inclined to add
PVC_RECURSE_AGGREGATES, as that seems more likely to be okay than not.
Actually, on closer look: why don't we just nuke that pull_var_clause
call entirely, along with the following loop inspecting its result?
The subsequent loop that looks for a matching StatisticExtInfo
expression will do just fine at rejecting any expression that
contains Vars of the wrong relation. Maybe there is some performance
argument why the pull_var_clause precheck is worth the trouble,
but I'm inclined to bet that it's actually a net loss.
regards, tom lane
On Tue, Mar 14, 2023 at 11:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Actually, on closer look: why don't we just nuke that pull_var_clause
call entirely, along with the following loop inspecting its result?The subsequent loop that looks for a matching StatisticExtInfo
expression will do just fine at rejecting any expression that
contains Vars of the wrong relation. Maybe there is some performance
argument why the pull_var_clause precheck is worth the trouble,
but I'm inclined to bet that it's actually a net loss.
Yes agreed. The pull_var_clause precheck is not necessary considering
we would match the 'clause_expr' to StatisticExtInfo expressions with
equal(). +1 to remove it.
Thanks
Richard