Re: postgres_fdw bug in 9.6

On 2017/03/30 20:16, Ashutosh Bapat wrote:
> The patch applies cleanly, compiles. make check in regress as well as
> postgres_fdw works fine. Here are few comments

Thanks for the review!

> local-join should be local join.

OK, done.

> The comments should explain why.
> +        /* Should be unparameterized */
> +        Assert(outer_path->param_info == NULL);
> +        Assert(inner_path->param_info == NULL);

Done.

> +     a suitable local join path, which can be used as the alternative local
> May be we should reword this as ... which can be used to create an alternative
> local ... This rewording is required even in the existing docs.

Done.

> +                /* outer_path should not require rels from inner_path */
> +                Assert(!PATH_PARAM_BY_REL(outer_path, inner_path->parent));
> Probably this should throw an error or return NULL in such case rather than
> Asserting. This function is callable from any FDW, and that FDW may provide
> such paths, may be because of an internal bug. Same case with
> +                /* Neither path should require rels from the other path */
> +                Assert(!PATH_PARAM_BY_REL(outer_path, inner_path->parent) ||
> +                       !PATH_PARAM_BY_REL(inner_path, outer_path->parent));

Good idea!  I think it's better to throw an error because that is a bug 
in the FDW; done that way.

> While the comment below mentions ON true, the testcase you have added is for ON
> false. Either the testcase should change or this comment. That raises another
> question, what happens when somebody does FULL JOIN ON false?
> +                     * If special case: for "x FULL JOIN y ON true", there

FULL JOIN ON FALSE would be handled the same way as FULL JOIN ON TRUE, 
so I think we should rewrite that comment into something like this: If 
special case: for "x FULL JOIN y ON true" or "x FULL JOIN y ON false"...

> Why JOIN_RIGHT is being treated differently from JOIN_LEFT? We should be able
> to create a nested loop join for JOIN_RIGHT?
> +        case JOIN_RIGHT:
> +        case JOIN_FULL:

I don't think so, because nestloop joins aren't supported for 
JOIN_RIGHT.  See ExecInitNestLoop().

Best regards,
Etsuro Fujita