Re: An inefficient query caused by unnecessary PlaceHolderVar

On Wed, May 31, 2023 at 1:27 AM James Coleman <jtc331(at)gmail(dot)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