On Wed, May 31, 2023 at 1:27 AM James Coleman <jtc...@gmail.com> wrote:
> This looks good to me. Thanks for the review! > A few small tweaks suggested to comment wording: > > +-- lateral reference for simple Var can escape PlaceHolderVar if the > +-- referenced rel is under the same lowest nulling outer join > +explain (verbose, costs off) > > I think this is clearer: "lateral references to simple Vars do not > need a PlaceHolderVar when the referenced rel is part of the same > lowest nulling outer join"? Thanks for the suggestion! How about we go with "lateral references to simple Vars do not need a PlaceHolderVar when the referenced rel is under the same lowest nulling outer join"? This seems a little more consistent with the comment in prepjointree.c. > * lateral references to something outside the subquery > being > - * pulled up. (Even then, we could omit the > PlaceHolderVar if > - * the referenced rel is under the same lowest outer > join, but > - * it doesn't seem worth the trouble to check that.) > + * pulled up. Even then, we could omit the > PlaceHolderVar if > + * the referenced rel is under the same lowest nulling > outer > + * join. > > I think this is clearer: "references something outside the subquery > being pulled up and is not under the same lowest outer join." Agreed. Will use this one. > One other thing: it would be helpful to have the test query output be > stable between HEAD and this patch; perhaps add: > > order by 1, 2, 3, 4, 5, 6, 7 > > to ensure stability? Thanks for the suggestion! I wondered about that too but I'm a bit confused about whether we should add ORDER BY in test case. I checked 'sql/join.sql' and found that some queries are using ORDER BY but some are not. Not sure what the criteria are. Thanks Richard
