Re: remove_useless_joins vs. bug #19560

I'm getting back to this task, after much too long in security-fix
land. I had the idea that we could resolve the disagreement about
whether to use ChangeVarNodes by inserting Asserts in it to catch
any places where it'd attempt to store an invalid value into a
Var's varno (or other fields that hold a relid). However, when
I tried that, it blew up! Some investigation showed that it
descends into the subquery fields of subquery RTEs even when those
RTEs are no longer relevant to the query, and there it can find
Var references that trip the assertions. The sequence is basically
that we pull up a lateral-referencing subquery into the main query,
and after flattening and maybe some preliminary join simplification,
the query no longer references some RTE so we decide we can remove
that. But ChangeVarNodes will descend into the original subquery RTE,
find the since-elided lateral reference, and boom.

So this is a pretty disturbing discovery: it means that as things
stand, we can't put any sanity checks into ChangeVarNodes, and aside
from that we might be doing quite a bit of useless work mutating
no-longer-interesting subqueries. Switching to code based on
remove_result_refs() doesn't help since that also scans the whole
query tree. (That function doesn't have an issue itself, since
at the time it runs we don't have any invalidated subqueries.)

One idea for fixing this is to null out the subquery field of a
subquery RTE as soon as we've pulled it up, but there is probably
code that will crash on a null subquery pointer. I'm also thinking
about invoking ChangeVarNodes on only portions of the query tree,
not including the rangetable, but that seems messy and not very
maintainable. Any thoughts?

regards, tom lane