On Thu, May 27, 2021 at 9:01 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Tue, Dec 8, 2020 at 10:46 AM James Coleman <jtc331@gmail.com> wrote:
> >
> > While I haven't actually tracked down to guarantee this is handled
> > elsewhere, a thought experiment -- I think -- shows it must be so.
> > Here's why: suppose we don't have a limit here, but the query return
> > order is different in different backends. Then we would have the same
> > problem you bring up. In that case this code is already setting
> > consider_parallel=true on the rel. So I don't think we're changing any
> > behavior here.
> >
>
> AFAICS, the patch seems very reasonable and specifically targets
> lateral subqueries with limit/offset. It seems like the uncorrelated
> case is the only real concern.
> I generally agree that the current patch is probably not changing any
> behavior in the uncorrelated case (and like yourself, haven't yet
> found a case for which it breaks), but I'm not sure Brian's concerns
> can be ruled out entirely.
>
> How about a minor update to the patch to make it slightly more
> restrictive, to exclude the case when there are no lateral
> cross-references, so we'll be allowing parallelism only when we know
> the lateral subquery will be evaluated anew for each source row?
> I was thinking of the following patch modification:
>
> BEFORE:
> - if (limit_needed(subquery))
> + if (!rte->lateral && limit_needed(subquery))
>
> AFTER:
> - if (limit_needed(subquery))
> + if ((!rte->lateral || bms_is_empty(rel->lateral_relids)) &&
> + limit_needed(subquery))
>
>
> Thoughts?
Apologies for the delayed response; this seems fine to me. I've
attached patch v2.
Thanks,
James Coleman
