On Mon, Sep 3, 2018 at 7:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > You haven't mentioned anything about backpatching, but I don't see any
> > problem with backpatching this fix.
>
> Yeah, we definitely need to back-patch, and that's another reason not
> to touch the catalog contents.
>
> > I will commit the attached patches in a day or so unless somebody sees
> > any problem.
>
> Looking more closely at the patch:
>
> * The general design in max_parallel_hazard_walker appears to be that
> after the initial check_functions_in_node test, the rest of it should be
> an if ... else if ... else if ... else if ... chain of mutually-exclusive
> IsA tests. Whoever stuck in the NextValueExpr test (possibly me?) did so
> with a tin ear, and you've duplicated that mistake here. Please make it
> "else if", and fix the NextValueExpr test to be "else if" while at it.
> (Or else get rid of all the "else"s, but that would be a shade less
> efficient unless the compiler is very very smart.)
>
> * The plan tree for the added test case is hard to read because it's
> unclear which tenk1 scan is which. I'd suggest adding aliases to
> clarify that, eg
>
> +explain (costs off, verbose)
> + select count(*) from tenk1 a where (unique1, two) in
> + (select unique1, row_number() over() from tenk1 b);
>
> HEAD patch is OK otherwise. I didn't look at the back-branch patches.
>
Thanks for the review. After making the changes suggested by you, I
have pushed and back-patched it till 9.6.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
