Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

Thanks for the comments.

2015/05/01 22:35、Robert Haas <robertmhaas@gmail.com> のメール：
> On Mon, Apr 27, 2015 at 5:07 AM, Shigeru Hanada
> <shigeru.hanada@gmail.com> wrote:
>> 2015-04-27 11:00 GMT+09:00 Kouhei Kaigai <kaigai@ak.jp.nec.com>:
>>> Hanada-san, could you adjust your postgres_fdw patch according to
>>> the above new (previous?) definition.
>>
>> The attached v14 patch is the revised version for your v13 patch.  It also contains changed for Ashutosh’s comments.
>
> We should probably move this discussion to a new thread now that the
> other patch is committed.  Changing subject line accordingly.
>
> Generally, there's an awful lot of changes in this patch - it is over
> 2000 insertions and more than 450 deletions - and it's not awfully
> obvious why all of those changes are there.  I think this patch needs
> a detailed README to accompany it explaining what the various changes
> in the patch are and why those things got changed; or maybe there is a
> way to break it up into multiple patches so that we can take a more
> incremental approach.  I am really suspicious of the amount of
> wholesale reorganization of code that this patch is doing.  It's
> really hard to validate that a reorganization like that is necessary,
> or that it's correct, and it's gonna make back-patching noticeably
> harder in the future.  If we really need this much code churn it needs
> careful justification; if we don't, we shouldn't do it.
>

I agree.  I’ll write detailed description for the patch and repost the new one with rebasing onto current HEAD.  I’m
sorrybut it will take a day or so... 

> +SET enable_mergejoin = off; -- planner choose MergeJoin even it has
> higher costs, so disable it for testing.
>
> This seems awfully strange.  Why would the planner choose a plan if it
> had a higher cost?

I thought so but I couldn’t reproduce such situation now.  I’ll investigate it more.  If the issue has gone, I’ll
removethat SET statement for straightforward tests. 


>
> -        * If the table or the server is configured to use remote estimates,
> -        * identify which user to do remote access as during planning.  This
> +        * Identify which user to do remote access as during planning.  This
>         * should match what ExecCheckRTEPerms() does.  If we fail due
> to lack of
>         * permissions, the query would have failed at runtime anyway.
>         */
> -       if (fpinfo->use_remote_estimate)
> -       {
> -               RangeTblEntry *rte = planner_rt_fetch(baserel->relid, root);
> -               Oid                     userid = rte->checkAsUser ?
> rte->checkAsUser : GetUserId();
> -
> -               fpinfo->user = GetUserMapping(userid, fpinfo->server->serverid);
> -       }
> -       else
> -               fpinfo->user = NULL;
> +       rte = planner_rt_fetch(baserel->relid, root);
> +       fpinfo->userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
>
> So, wait a minute, remote estimates aren't optional any more?

No, it seems to be removed accidentally.  I’ll check the reason again though, but I’ll revert the change unless I find
anyproblem. 

--
Shigeru HANADA
shigeru.hanada@gmail.com