Re: [sqlsmith] Failed assertion in adjust_appendrel_attrs_mutator

Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
> On 2017/10/23 2:07, Tom Lane wrote:
>> Hmm. adjust_appendrel_attrs() thinks it's only used after conversion
>> of sublinks to subplans, but this is a counterexample. I wonder if
>> that assumption was ever correct? Or maybe we need to rethink what
>> it does when recursing into RTE subqueries.

> Looking at the backtrace, the crashing SubLink seems to have been
> referenced from a PlaceHolderVar that is in turn referenced by
> joinaliasvars of a JOIN rte in query->rtable.

Right. The core of the issue is that joinaliasvars lists don't get run
through preprocess_expression, so (among other things) any SubLinks in
them don't get expanded to subplans. Probably the reason we've not
heard field complaints about this is that in a non-assert build there
would be no observable bad effects --- the scan would simply ignore
the subquery, and whether the joinaliasvars entry has been correctly
mutated doesn't matter at all because it will never be used again.

> I wonder if we shouldn't just ignore those (joinaliasvars in JOIN rtes)
> while adjust_appendrel_attrs() is doing its job on a Query, just like we
> ask to ignore subqueries by passing QTW_IGNORE_RC_SUBQUERIES to
> query_tree_mutator()?

I don't really like this fix, because ISTM it's fixing one symptom rather
than the root of the problem. The root is that joinaliasvars lists
diverge from the representation of expressions elsewhere in the tree,
and not only with respect to SubLinks --- another example is that function
calls with named arguments won't have been rearranged into executable
form. That could easily be a dangerous thing, if we allow arbitrary
expression processing to get done on them. Moreover, your patch is
letting the divergence get even bigger: now the joinaliasvars lists don't
even have the right varnos, making them certainly unusable for anything.

So what I'm thinking is that we would be well advised to actually remove
the untransformed joinaliasvars from the tree as soon as we're done with
preprocessing expressions. We'd drop them at the end of planning anyway
(cf. add_rte_to_flat_rtable) so this is just getting rid of them a bit
sooner, and it won't affect anything after the planner.

In short, I'm thinking something more like the attached.

Comments?

regards, tom lane