On 2017/10/24 0:22, Tom Lane wrote: > Amit Langote <langote_amit...@lab.ntt.co.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.
Ah, I missed that. > 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 see. >> 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. That's true. > 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. Hmm, yes. Although, I only proposed that patch because I had convinced myself that joinaliasvars lists weren't looked at anymore. > 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. Yeah, that makes more sense. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers