On Mon, Apr 27, 2015 at 5:07 AM, Shigeru Hanada
<shigeru.han...@gmail.com> wrote:
> 2015-04-27 11:00 GMT+09:00 Kouhei Kaigai <kai...@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.

+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?

-        * 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?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to