Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins - Mailing list pgsql-hackers

On 9 March 2017 at 18:06, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
>>>
>>> Here's the patch attached.
>>
>> Agreed. It seems like a good idea to keep that logic in a single location
>
> Ok.
>
>>
>> I've beaten your patch around a bit and come up with the attached.
>
> The new name merge_fdw_options() is shorter than the one I chose, but
> we are not exactly merging options for an upper relation since there
> isn't the other fpinfo to merge from. But I think we can live with
> merge_fdw_options().

Perhaps "combine" is a better word? I didn't really see a problem with
that. After I posted this I wished I'd made the function even more
generic to accept either side as NULL and make up the new fpinfo from
the non-NULL one, or Merge if both were non-NULL.  I liked that way
much better than giving the function too much knowledge of what its
purpose actually is. It's more likely to get used for something else
in the future, which means there's less chance that someone else will
make the same mistake.

> Looks like you forgot to compile the patch. It gave error
>
> postgres_fdw.c: In function ‘merge_fdw_options’:
> postgres_fdw.c:4337:2: error: ‘RelOptInfo’ has no member named ‘server’
> postgres_fdw.c:4337:2: error: ‘RelOptInfo’ has no member named ‘server’

Seems I forgot to test with asserts enabled. Thanks for finding/fixing that

> Once I fixed that, the testcases started showing an assertion failure,
> since fpinfo of a base relation can not have an outerrel. Fixed the
> assertion as well. If we are passing fpinfo's of joining relations,
> there's no need to have outerrel and innerrel in fpinfo of join.

Looks like you forgot to update the comment on the Assert()

> Also, you had removed comment
> /*
> +     * Set the foreign server to which this join will be shipped if found safe
> +     * to push-down. We need server specific options like extensions to decide
> +     * push-down safety, so set FDW specific options.
> +     */
> But I think it's important to have it there, so that reader knows why
> merge_fdw_options() needs to be called before assessing quals.

No objections.

> I have
> updated it a bit to clarify this further. Also, merge_fdw_options()
> shouldn't set fpinfo->server since it's not an FDW option per say.
> It's better to keep that outside of this function. With your patch
> fpinfo->server was being set twice for an upper relation.

Thanks for noticing. I intended that not to be there, but seems I
forgot to hit delete.

>>
>> The changes from yours are mostly cosmetic, but I've also added a
>> regression test too.
>
> Thanks a lot for the test.
>
> PFA updated patch.

Thanks for making the chances.

I'd say it's ready to go, pending updating the out of date comment:

+ /* We must always have fpinfo_o and it must always have an outerrel */
+ Assert(fpinfo_o);


-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services