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 sorry but 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 remove that 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 any problem.

--
Shigeru HANADA
shigeru.hanada@gmail.com

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

Attached is the v15 patch of foreign join support for postgres_fdw.

This patch is based on current master, and having being removed some
hunks which are not essential.

And I wrote description of changes done by the patch. It is little
bit long but I hope it would help understanding what the patch does.

The total LOC of the patch is 3.7k, 1.8k for code and 2.0k for
regression tests. This is not a small patch, as Robert says, so I'd
like to summarize changed done by this patch and explain why they are
necessary.

Outline of join push-down support for postgres_fdw
==================================================

This patch provides new capability to join between foriegn tables
managed by same foreign server on remote side, by constructing a
remote query containing join clause, and executing it as source of a
pseudo foreign scan. This patch is based on Custom/Foreign join patch
written by Kohei KaiGai.

PostgreSQL's planning for a query containing join is done with these steps:

1. generate possible scan paths for each base relations
2. generate join paths with bottom-up approach
3. generate plan nodes required for the cheapest path
4. execute the plan nodes to obtain result tuples

Generating path node
--------------------
As of now, postgres_fdw generates a ForeignPath which represents a
result of a join for each RelOptInfo, and planner can determine which
path is cheapest from its cost values.

GetForeignJoinPaths is called once for each join combination, i.e. A
JOIN B and B JOIN A are considered separately. So GetForeignJoinPath
should return immediately to skip its job when the call is the
reversed combination of already considered one. For this purpose, I
added update_safe flag to PgFdwRelationInfo. This flag is always set
for simple foriegn scans, but for join relation it is set only when
the join can be pushed down. The reason of adding this flag is that
checking RelOptInfo#fdw_private is MULL can't prevent useless
processing for a join combination which is reversed one of already
considered join which can't be pushed down.

postgres_fdw's GetForeignJoinPaths() does various checks, to ensure
that the result has same semantics as local joins. Now postgres_fdw
have these criteria:

a) join type must be one of INNER/LEFT OUTER/RIGHT OUTER/FULL OUTER join
This check is done with given jointype argument. IOW, CROSS joins and
SEMI/ANTI joins are not pushed down. This is because 1) CROSS joins
would produe more result than separeted join sources, and 2) ANTI/SEMI
joins need to be deparsed as sub-query and it seems to take some more
time to implement.
b) Both outer and inner must have RelOptInfo#fdw_private
Having fdw_private means that the RelOptInfo is safe to push down, so
having no fdw_private means that portion is not safe to push down and
thus the whole join is not safe to push down.
c) All relations in the join must belong to the same server
This check is done with serverid stored in RelOptInfo#fdw_private as
PgFdwRelationInfo. Joining relations belong to different servers is
not leagal. Even they finally have completely same connection
information, they should accessed via different libpq sessions.
Responsibility of checking server matching is under discussion in the
Custom/Foreign join thread, and I'd vote for checking it in core. If
it is decided, I remove this criterion soon.
d) All relations must have the same effective user id
This check is done with userid stored in PgFdwRelationInfo, which is
valid only when underlying relations have the same effective user id.
Here "effective user id" means id of the user executing the query, or
the owner of the view when the foreign table is accessed via view.
Tom suggested that it is necessary to check that user mapping matches
for security reason, and now I think it's better than checking
effective user as current postgres_fdw does.
e) Each source relation must not have any local filter
Evaluating conditions of join source talbe potentially produces
different result in OUTER join cases. This can be relaxed for the
cases when the join is INNER and local filters don't contain any
volatile function/operator, but it is left as future enhancement.
f) All join conditions of non-inner join must be safe to push down
This is similar to e).

A join which passes all criteria above is safe to push-down, so
postgres_fdw create a ForeignPath for the join and add it to
RelOptInfo. Currently postgres_fdw doesn't set pathkeys (ordering
information) nor require_outer (information for parameterized path).

PgFdwRelationInfo is used to store various planning information
specific to postgres_fdw. To support join push-down, I added some
fields which are necessary to deparse join query recursively in
deparseSelectSql.

- outer RelOptInfo, to generate source relatoin as subquery
- inner RelOptInfo, to generate source relation as subquery
- jointype, to deparse JOIN keyword string (e.g. INNER JOIN)
- joinclauses, to deprase ON part of JOIN clause
- otherclauses, to deparse WHERE clause of join query

I also moved it from postgres_fdw.c to postgres_fdw.h, because
deparse.c needs to refer the definition during deparsing query.

Generating plan node
--------------------
Once planner find the cheapest path, it generates plan tree from the
path tree. During the steps, planner calls GetForeignPlan for the
ForeignPath in the top of a join tree. IOW, GetForeignPlan is not
called for underlying joins and scans, so postgres_fdw needs a way to
do its task (mainly generating query string) recursively.

GetForeignPlan generates remote query and store it in ForeignScan node
to be returned. The construction of remote query is done by calling
deparseSelectSql for given RelOptInfo. This function was modified to
accept both base relations and join relations to support join
push-down. Main part of generating join query is implemented in
deparseJoinSql, which is a newly added function, and deparseSelectSql
calls it if given relation was a join relation.

One big change about deparsing base relation is aliasing. This patch
adds column alias to SELECT clause even original query is a simple
single table SELECT.

fdw=# EXPLAIN (VERBOSE, COSTS false) SELECT * FROM pgbench_branches b;
QUERY PLAN
------------------------------------------------------------------------------------
Foreign Scan on public.pgbench_branches b
Output: bid, bbalance, filler
Remote SQL: SELECT bid a9, bbalance a10, filler a11 FROM
public.pgbench_branches
(3 rows)

As you see, every column has alias in format "a%d" with index value
derived from pg_attribute.attnum. Index value is attnum + 8, and the
magic number "8" comes from FirstLowInvalidHeapAttributeNumber for the
adjustment that makes attribute number of system attributes positive.
To make the code more readable, I introduced local macro
GET_RELATIVE_ATTNO(), but I'm not sure that this name is very nice.
This column alias system allows to refer a particular column pointed
by Var node in upper join level without consideration about column
position in SELECT clause. To achieve this, deparseVar is expanded to
handle variables used in join query, and deparseJoinVar is added to
deparse column reference with table alias "l" or "r" for outer or
inner respectively.

As mentioned at the beginning of this section, GetForeignPlan is
called only for the top node of the join tree, so we need to do
something recursively. So postgres_fdw has information about join
tree structure in PgFdwRelationInfo and pass it via
RelOptInfo#fdw_private. This link works down to the base relation at
the leaf of the join tree.

When deparseSelectSql is called for a join relation, it calls
deparseSelectSql for each outer and inner RelOptInfo to generate query
strings, and pass them to deparseJoinSql as parts for FROM clause
subquery. For example of two way join:

[table definition]
CREATE FOREIGN TABLE table1 (
id int NOT NULL,
name text,
...
) SERVER server;
CREATE FOREIGN TABLE table2 (
id int NOT NULL,
name text,
...
) SERVER server;

[original query]
SELECT t1.name, t2.name FROM table1 t1 INNER JOIN table2 t2 ON t1.id = t2.id;

[remote query]
SELECT l.a2, r.a2
FROM (SELECT id, name FROM table1) l (a1, a2)
INNER JOIN
(SELECT id, name FROM table2) r (a1, a2)
ON (l.a1 = r.a1)
;

During deparsing join query, deparseJoinSql maintains fdw_scan_tlist
list to hold TargetEntry for columns used in SELECT clause of every
query.

One thing tricky is "peusdo projection" which is done by
deparseProjectionSql for whole-row reference. This is done by put the
query string in FROM subquery and add whole-row reference in outer
SELECT clause. This is done by ExecProjection in 9.4 and older, but
we need to do this in postgres_fdw because ExecProjection is not
called any more.

For various conditions, such as join conditions in JOIN clauses and
filter conditions in WHERE clauses, appendCondition is used to deparse
condition strings. This was expanded version of appendWhereClause.
Note that appendConditions accepts list of RestrictInfo or list of
Expr as source, and downcasts them properly. As of 9.4
appendWhereClause accepted only RestrictInfo, but join conditions are
Expr, so I made it little flexible.

Finally deparseJoinSql construct whole query by putting parts into the
right places. Note that column aliases are not written in SELECT
clause but in FROM clause, after table alias. This simpifies SELECT
clause construction simpler.

debug stuffs
------------
bms_to_str is a function which prints contents of a bitmapset in
human-readable format. I added this for debug purpose, but IMO it's
ok to have such function as public bitmapset API.

2015-05-03 10:51 GMT+09:00 Shigeru HANADA <shigeru.hanada@gmail.com>:

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 sorry but 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 remove that 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 any problem.

--
Shigeru HANADA
shigeru.hanada@gmail.com

--
Shigeru HANADA

On Sat, May 16, 2015 at 6:34 PM, Shigeru Hanada <shigeru.hanada@gmail.com>
wrote:

Attached is the v15 patch of foreign join support for postgres_fdw.

This patch is based on current master, and having being removed some
hunks which are not essential.

And I wrote description of changes done by the patch. It is little
bit long but I hope it would help understanding what the patch does.

The total LOC of the patch is 3.7k, 1.8k for code and 2.0k for
regression tests. This is not a small patch, as Robert says, so I'd
like to summarize changed done by this patch and explain why they are
necessary.

Outline of join push-down support for postgres_fdw
==================================================

This patch provides new capability to join between foriegn tables
managed by same foreign server on remote side, by constructing a
remote query containing join clause, and executing it as source of a
pseudo foreign scan. This patch is based on Custom/Foreign join patch
written by Kohei KaiGai.

PostgreSQL's planning for a query containing join is done with these steps:

1. generate possible scan paths for each base relations
2. generate join paths with bottom-up approach
3. generate plan nodes required for the cheapest path
4. execute the plan nodes to obtain result tuples

Generating path node
--------------------
As of now, postgres_fdw generates a ForeignPath which represents a
result of a join for each RelOptInfo, and planner can determine which
path is cheapest from its cost values.

GetForeignJoinPaths is called once for each join combination, i.e. A
JOIN B and B JOIN A are considered separately. So GetForeignJoinPath
should return immediately to skip its job when the call is the
reversed combination of already considered one. For this purpose, I
added update_safe flag to PgFdwRelationInfo. This flag is always set
for simple foriegn scans, but for join relation it is set only when
the join can be pushed down. The reason of adding this flag is that
checking RelOptInfo#fdw_private is MULL can't prevent useless
processing for a join combination which is reversed one of already
considered join which can't be pushed down.

This is just a suggestion, but you may actually get rid of the flag by
restricting the path generation only when say outer relation's pointer or
OID or relid is greater/lesser than inner relation's corresponding property.

postgres_fdw's GetForeignJoinPaths() does various checks, to ensure
that the result has same semantics as local joins. Now postgres_fdw
have these criteria:

a) join type must be one of INNER/LEFT OUTER/RIGHT OUTER/FULL OUTER join
This check is done with given jointype argument. IOW, CROSS joins and
SEMI/ANTI joins are not pushed down. This is because 1) CROSS joins
would produe more result than separeted join sources,

We might loose on some optimizations in aggregate push-down by not creating
paths altogether for CROSS joins. If there is a count(*) on CROSS join
result, we will push count(*) since there doesn't exist a foreign path for
the join. OR it might be possible that pushing down A INNER JOIN B CROSS
JOIN C is cheaper than performing some or all of the joins locally. I think
we should create a path and let it stay in the paths list. If there is no
path which can use CROSS join path, it will discarded eventually. Sorry for
bringing this so late in the discussion.

and 2) ANTI/SEMI
joins need to be deparsed as sub-query and it seems to take some more
time to implement.
b) Both outer and inner must have RelOptInfo#fdw_private
Having fdw_private means that the RelOptInfo is safe to push down, so
having no fdw_private means that portion is not safe to push down and
thus the whole join is not safe to push down.
c) All relations in the join must belong to the same server
This check is done with serverid stored in RelOptInfo#fdw_private as
PgFdwRelationInfo. Joining relations belong to different servers is
not leagal. Even they finally have completely same connection
information, they should accessed via different libpq sessions.
Responsibility of checking server matching is under discussion in the
Custom/Foreign join thread, and I'd vote for checking it in core. If
it is decided, I remove this criterion soon.
d) All relations must have the same effective user id
This check is done with userid stored in PgFdwRelationInfo, which is
valid only when underlying relations have the same effective user id.
Here "effective user id" means id of the user executing the query, or
the owner of the view when the foreign table is accessed via view.
Tom suggested that it is necessary to check that user mapping matches
for security reason, and now I think it's better than checking
effective user as current postgres_fdw does.
e) Each source relation must not have any local filter
Evaluating conditions of join source talbe potentially produces
different result in OUTER join cases. This can be relaxed for the
cases when the join is INNER and local filters don't contain any
volatile function/operator, but it is left as future enhancement.
f) All join conditions of non-inner join must be safe to push down
This is similar to e).

A join which passes all criteria above is safe to push-down, so
postgres_fdw create a ForeignPath for the join and add it to
RelOptInfo. Currently postgres_fdw doesn't set pathkeys (ordering
information) nor require_outer (information for parameterized path).

PgFdwRelationInfo is used to store various planning information
specific to postgres_fdw. To support join push-down, I added some
fields which are necessary to deparse join query recursively in
deparseSelectSql.

- outer RelOptInfo, to generate source relatoin as subquery
- inner RelOptInfo, to generate source relation as subquery
- jointype, to deparse JOIN keyword string (e.g. INNER JOIN)
- joinclauses, to deprase ON part of JOIN clause
- otherclauses, to deparse WHERE clause of join query

I also moved it from postgres_fdw.c to postgres_fdw.h, because
deparse.c needs to refer the definition during deparsing query.

Generating plan node
--------------------
Once planner find the cheapest path, it generates plan tree from the
path tree. During the steps, planner calls GetForeignPlan for the
ForeignPath in the top of a join tree. IOW, GetForeignPlan is not
called for underlying joins and scans, so postgres_fdw needs a way to
do its task (mainly generating query string) recursively.

GetForeignPlan generates remote query and store it in ForeignScan node
to be returned. The construction of remote query is done by calling
deparseSelectSql for given RelOptInfo. This function was modified to
accept both base relations and join relations to support join
push-down. Main part of generating join query is implemented in
deparseJoinSql, which is a newly added function, and deparseSelectSql
calls it if given relation was a join relation.

One big change about deparsing base relation is aliasing. This patch
adds column alias to SELECT clause even original query is a simple
single table SELECT.

fdw=# EXPLAIN (VERBOSE, COSTS false) SELECT * FROM pgbench_branches b;
QUERY PLAN

------------------------------------------------------------------------------------
Foreign Scan on public.pgbench_branches b
Output: bid, bbalance, filler
Remote SQL: SELECT bid a9, bbalance a10, filler a11 FROM
public.pgbench_branches
(3 rows)

As you see, every column has alias in format "a%d" with index value
derived from pg_attribute.attnum. Index value is attnum + 8, and the
magic number "8" comes from FirstLowInvalidHeapAttributeNumber for the
adjustment that makes attribute number of system attributes positive.
To make the code more readable, I introduced local macro
GET_RELATIVE_ATTNO(), but I'm not sure that this name is very nice.
This column alias system allows to refer a particular column pointed
by Var node in upper join level without consideration about column
position in SELECT clause. To achieve this, deparseVar is expanded to
handle variables used in join query, and deparseJoinVar is added to
deparse column reference with table alias "l" or "r" for outer or
inner respectively.

As mentioned at the beginning of this section, GetForeignPlan is
called only for the top node of the join tree, so we need to do
something recursively. So postgres_fdw has information about join
tree structure in PgFdwRelationInfo and pass it via
RelOptInfo#fdw_private. This link works down to the base relation at
the leaf of the join tree.

When deparseSelectSql is called for a join relation, it calls
deparseSelectSql for each outer and inner RelOptInfo to generate query
strings, and pass them to deparseJoinSql as parts for FROM clause
subquery. For example of two way join:

[table definition]
CREATE FOREIGN TABLE table1 (
id int NOT NULL,
name text,
...
) SERVER server;
CREATE FOREIGN TABLE table2 (
id int NOT NULL,
name text,
...
) SERVER server;

[original query]
SELECT t1.name, t2.name FROM table1 t1 INNER JOIN table2 t2 ON t1.id =
t2.id;

[remote query]
SELECT l.a2, r.a2
FROM (SELECT id, name FROM table1) l (a1, a2)
INNER JOIN
(SELECT id, name FROM table2) r (a1, a2)
ON (l.a1 = r.a1)
;

During deparsing join query, deparseJoinSql maintains fdw_scan_tlist
list to hold TargetEntry for columns used in SELECT clause of every
query.

One thing tricky is "peusdo projection" which is done by
deparseProjectionSql for whole-row reference. This is done by put the
query string in FROM subquery and add whole-row reference in outer
SELECT clause. This is done by ExecProjection in 9.4 and older, but
we need to do this in postgres_fdw because ExecProjection is not
called any more.

For various conditions, such as join conditions in JOIN clauses and
filter conditions in WHERE clauses, appendCondition is used to deparse
condition strings. This was expanded version of appendWhereClause.
Note that appendConditions accepts list of RestrictInfo or list of
Expr as source, and downcasts them properly. As of 9.4
appendWhereClause accepted only RestrictInfo, but join conditions are
Expr, so I made it little flexible.

Finally deparseJoinSql construct whole query by putting parts into the
right places. Note that column aliases are not written in SELECT
clause but in FROM clause, after table alias. This simpifies SELECT
clause construction simpler.

About deparsing stuff, it looks like we are duplicating the functionality
in ruleutils.c and more push-down stuff we add, more will be the
duplication. Can we reuse that functionality?

debug stuffs
------------
bms_to_str is a function which prints contents of a bitmapset in
human-readable format. I added this for debug purpose, but IMO it's
ok to have such function as public bitmapset API.

2015-05-03 10:51 GMT+09:00 Shigeru HANADA <shigeru.hanada@gmail.com>:

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 sorry but 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 remove that 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 any problem.

--
Shigeru HANADA
shigeru.hanada@gmail.com

--
Shigeru HANADA

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

--

Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Sat, May 16, 2015 at 9:04 AM, Shigeru Hanada
<shigeru.hanada@gmail.com> wrote:

d) All relations must have the same effective user id
This check is done with userid stored in PgFdwRelationInfo, which is
valid only when underlying relations have the same effective user id.
Here "effective user id" means id of the user executing the query, or
the owner of the view when the foreign table is accessed via view.
Tom suggested that it is necessary to check that user mapping matches
for security reason, and now I think it's better than checking
effective user as current postgres_fdw does.

So, should this be a separate patch?

One of my concerns about this patch is that it's got a lot of stuff in
it that isn't obviously related to the patch. Anything that is a
separate change should be separated out into its own patch. Perhaps
you can post a set of patches that apply one on top of the next, with
the changes for each one clearly separated.

e) Each source relation must not have any local filter
Evaluating conditions of join source talbe potentially produces
different result in OUTER join cases. This can be relaxed for the
cases when the join is INNER and local filters don't contain any
volatile function/operator, but it is left as future enhancement.

I think this restriction is a sign that you're not really doing this
right. Consider:

(1) SELECT * FROM a LEFT JOIN b ON a.x = b.x AND b.x = 3;
(2) SELECT * FROM a LEFT JOIN b ON a.x = b.x WHERE b.x = 3;

If you push down the scan of b, you can include the b.x = 3 qual in
case (1) but not in case (2). If you push down the join, you can
include the qual in either case, but you must attach it in the same
place where it was before.

One big change about deparsing base relation is aliasing. This patch
adds column alias to SELECT clause even original query is a simple
single table SELECT.

fdw=# EXPLAIN (VERBOSE, COSTS false) SELECT * FROM pgbench_branches b;
QUERY PLAN
------------------------------------------------------------------------------------
Foreign Scan on public.pgbench_branches b
Output: bid, bbalance, filler
Remote SQL: SELECT bid a9, bbalance a10, filler a11 FROM
public.pgbench_branches
(3 rows)

As you see, every column has alias in format "a%d" with index value
derived from pg_attribute.attnum. Index value is attnum + 8, and the
magic number "8" comes from FirstLowInvalidHeapAttributeNumber for the
adjustment that makes attribute number of system attributes positive.

Yeah. I'm not sure this is a good idea. The column labels are
pointless at the outermost level.

I'm not sure it isn't a good idea, either, but I have some doubts.

One thing tricky is "peusdo projection" which is done by
deparseProjectionSql for whole-row reference. This is done by put the
query string in FROM subquery and add whole-row reference in outer
SELECT clause. This is done by ExecProjection in 9.4 and older, but
we need to do this in postgres_fdw because ExecProjection is not
called any more.

What commit changed this?

Thanks for your work on this. Although I know progress has been slow,
I think this work is really important to the project.

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

One thing tricky is "peusdo projection" which is done by
deparseProjectionSql for whole-row reference. This is done by put the
query string in FROM subquery and add whole-row reference in outer
SELECT clause. This is done by ExecProjection in 9.4 and older, but
we need to do this in postgres_fdw because ExecProjection is not
called any more.

What commit changed this?

It seems to me the nature of whole-row reference, not a flaw of
ExecProject().

In case of base relation scan, whole-row reference is transformed
to ExecEvalWholeRowVar, then executed during ExecProject().
It constructs a record datum according to the TupleDesc of the
relation being in scan.
On the other hands, foreign-join also looks like a scan on relation
that is result set of remote join, its record type is defined in
the fdw_scan_tlist.
However, it may contain values come from multiple relations,
so it is not intended behavior if whole-row reference constructs
a record datum that contains all the attributes in the result-
set. In this context, whole-row reference shall contain all the
attributes of the "base" relation.
Only FDW driver can know, and ensure all the attributes to
construct whole-row reference are fetched from remote side.

Hanada-san, could you correct me, if I misunderstood above your
explanation?

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

Thank for your comments.

2015-05-21 23:11 GMT+09:00 Robert Haas <robertmhaas@gmail.com>:

On Sat, May 16, 2015 at 9:04 AM, Shigeru Hanada
<shigeru.hanada@gmail.com> wrote:

d) All relations must have the same effective user id
This check is done with userid stored in PgFdwRelationInfo, which is
valid only when underlying relations have the same effective user id.
Here "effective user id" means id of the user executing the query, or
the owner of the view when the foreign table is accessed via view.
Tom suggested that it is necessary to check that user mapping matches
for security reason, and now I think it's better than checking
effective user as current postgres_fdw does.

So, should this be a separate patch?

Yes, it should be an additional patch for Custom/Foreign join API which is already committed.
The patch would contain these changes:
* add new field usermappingid to RelOptInfo, it is InvalidOid for non-foreign tables
* obtain oid of user mapping for a foreign table, and store it in the RelOptInfo
(we already have serverid in RelOptInfo, so userid is enough to identify user mapping though)
* propagate usermappingid up to the join relation when outer and inner relations have same valid value
* check matching of user mapping before calling GetForeignJoinPaths, rather than serverid

One of my concerns about this patch is that it's got a lot of stuff in
it that isn't obviously related to the patch. Anything that is a
separate change should be separated out into its own patch. Perhaps
you can post a set of patches that apply one on top of the next, with
the changes for each one clearly separated.

IIUC, each patch should not break compilation, and should contain only one complete logical change which can't be separated into pieces. I think whole of the patch is necessary to implement

e) Each source relation must not have any local filter
Evaluating conditions of join source talbe potentially produces
different result in OUTER join cases. This can be relaxed for the
cases when the join is INNER and local filters don't contain any
volatile function/operator, but it is left as future enhancement.

I think this restriction is a sign that you're not really doing this
right. Consider:

(1) SELECT * FROM a LEFT JOIN b ON a.x = b.x AND b.x = 3;
(2) SELECT * FROM a LEFT JOIN b ON a.x = b.x WHERE b.x = 3;

If you push down the scan of b, you can include the b.x = 3 qual in
case (1) but not in case (2). If you push down the join, you can
include the qual in either case, but you must attach it in the same
place where it was before.

One big change about deparsing base relation is aliasing. This patch
adds column alias to SELECT clause even original query is a simple
single table SELECT.

fdw=# EXPLAIN (VERBOSE, COSTS false) SELECT * FROM pgbench_branches b;
QUERY PLAN
------------------------------------------------------------------------------------
Foreign Scan on public.pgbench_branches b
Output: bid, bbalance, filler
Remote SQL: SELECT bid a9, bbalance a10, filler a11 FROM
public.pgbench_branches
(3 rows)

As you see, every column has alias in format "a%d" with index value
derived from pg_attribute.attnum. Index value is attnum + 8, and the
magic number "8" comes from FirstLowInvalidHeapAttributeNumber for the
adjustment that makes attribute number of system attributes positive.

Yeah. I'm not sure this is a good idea. The column labels are
pointless at the outermost level.

I'm not sure it isn't a good idea, either, but I have some doubts.

I fixed the patch to not add column alias to remote queries for single table. This change also reduces amount of differences from master branch slightly.

One thing tricky is "peusdo projection" which is done by
deparseProjectionSql for whole-row reference. This is done by put the
query string in FROM subquery and add whole-row reference in outer
SELECT clause. This is done by ExecProjection in 9.4 and older, but
we need to do this in postgres_fdw because ExecProjection is not
called any more.

What commit changed this?

No commit changed this behavior, as Kaigai-san says. If you still have comments, please refer my response to Kaigai-san.

Thanks for your work on this. Although I know progress has been slow,
I think this work is really important to the project.

I agree. I’ll take more time for this work.

--
Shigeru HANADA

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

2015-05-22 18:37 GMT+09:00 Shigeru Hanada <shigeru.hanada@gmail.com>:

2015-05-21 23:11 GMT+09:00 Robert Haas <robertmhaas@gmail.com>:

On Sat, May 16, 2015 at 9:04 AM, Shigeru Hanada
<shigeru.hanada@gmail.com> wrote:

d) All relations must have the same effective user id
This check is done with userid stored in PgFdwRelationInfo, which is
valid only when underlying relations have the same effective user id.
Here "effective user id" means id of the user executing the query, or
the owner of the view when the foreign table is accessed via view.
Tom suggested that it is necessary to check that user mapping matches
for security reason, and now I think it's better than checking
effective user as current postgres_fdw does.

So, should this be a separate patch?

I wrote patches for this issue. Let me describe their design.

To require the matching of user mapping between two relations (base or
join) which involves foreign tables, it would require these stuffs:

a) Add new field umid to RelOptInfo to hold OID of user mapping used
for the relation and children
b) Propagate umid up to join relation only when both outer and inner
have the save valid values
c) Check matching of umid between two relations to be joined before
calling GetForeignJoinPaths

For a), adding an OID field would not be a serious burden. Obtaining
the OID of user mapping can be accomplished by calling GetUserMapping
in get_relation_info, but it allocates an unnecessary UserMapping
object, so I added GetUserMappingId which just returns OID.

One concern about getting user mapping is checkAsUser. Currently FDW
authors are required to consider which user is valid as argument of
GetUserMapping, because it is different from the result of GetUserId
when the target relation is accessed via a view which is owned by
another user. This requirement would be easily ignored by FDW authors
and the ignorance causes terrible security issues. So IMO it should
be hidden in the core code, and FDW authors should use user mappings
determined by the core. This would break FDW I/F, so we can't change
it right now, but making GetUserMapping obsolete and mention it in the
documents would guide FDW authors to the right direction.

For b), such check should be done in build_join_rel, similarly to serverid.

For c), we can reuse the check about RelOptInfo#fdwroutine in
add_paths_to_joinrel, because all of serverid, umid and fdwroutine are
valid only when both the servers and the user mappings match between
outer and inner relations.

Attached are the patches which implement the idea above except
checkAsUser issue.
usermapping_matching.patch: check matching of user mapping OIDs
add_GetUserMappingById.patch: add helper function which is handy for
FDWs to obtain UserMapping
foreign_join_v16.patch: postgres_fdw which assumes user mapping
matching is done in core

Another idea about a) is to have an entire UserMapping object for each
RelOptInfo, but it would require UserMapping to have extra field of
its Oid, say umid, to compare them by OID. But IMO creating
UserMapping for every RelOptInfo seems waste of space and time.

Thoughts?
--
Shigeru HANADA

Hi Hanada-san,
I have reviewed usermapping patch and here are some comments.

The patch applies cleanly on Head and compiles without problem. The make
check in regression folder does not show any failure.

In find_user_mapping(), if the first cache search returns a valid tuple, it
is checked twice for validity, un-necessarily. Instead if the first search
returns a valid tuple, it should be returned immediately. I see that this
code was copied from GetUserMapping(), where as well it had the same
problem.

In build_join_rel(), we check whether user mappings of the two joining
relations are same. If the user mappings are same, doesn't that imply that
the foreign server and local user are same too?

Rest of the patch looks fine.

On Thu, May 28, 2015 at 10:50 AM, Shigeru Hanada <shigeru.hanada@gmail.com>
wrote:

2015-05-22 18:37 GMT+09:00 Shigeru Hanada <shigeru.hanada@gmail.com>:

2015-05-21 23:11 GMT+09:00 Robert Haas <robertmhaas@gmail.com>:

On Sat, May 16, 2015 at 9:04 AM, Shigeru Hanada
<shigeru.hanada@gmail.com> wrote:

d) All relations must have the same effective user id
This check is done with userid stored in PgFdwRelationInfo, which is
valid only when underlying relations have the same effective user id.
Here "effective user id" means id of the user executing the query, or
the owner of the view when the foreign table is accessed via view.
Tom suggested that it is necessary to check that user mapping matches
for security reason, and now I think it's better than checking
effective user as current postgres_fdw does.

So, should this be a separate patch?

I wrote patches for this issue. Let me describe their design.

To require the matching of user mapping between two relations (base or
join) which involves foreign tables, it would require these stuffs:

a) Add new field umid to RelOptInfo to hold OID of user mapping used
for the relation and children
b) Propagate umid up to join relation only when both outer and inner
have the save valid values
c) Check matching of umid between two relations to be joined before
calling GetForeignJoinPaths

For a), adding an OID field would not be a serious burden. Obtaining
the OID of user mapping can be accomplished by calling GetUserMapping
in get_relation_info, but it allocates an unnecessary UserMapping
object, so I added GetUserMappingId which just returns OID.

One concern about getting user mapping is checkAsUser. Currently FDW
authors are required to consider which user is valid as argument of
GetUserMapping, because it is different from the result of GetUserId
when the target relation is accessed via a view which is owned by
another user. This requirement would be easily ignored by FDW authors
and the ignorance causes terrible security issues. So IMO it should
be hidden in the core code, and FDW authors should use user mappings
determined by the core. This would break FDW I/F, so we can't change
it right now, but making GetUserMapping obsolete and mention it in the
documents would guide FDW authors to the right direction.

For b), such check should be done in build_join_rel, similarly to serverid.

For c), we can reuse the check about RelOptInfo#fdwroutine in
add_paths_to_joinrel, because all of serverid, umid and fdwroutine are
valid only when both the servers and the user mappings match between
outer and inner relations.

Attached are the patches which implement the idea above except
checkAsUser issue.
usermapping_matching.patch: check matching of user mapping OIDs
add_GetUserMappingById.patch: add helper function which is handy for
FDWs to obtain UserMapping
foreign_join_v16.patch: postgres_fdw which assumes user mapping
matching is done in core

Another idea about a) is to have an entire UserMapping object for each
RelOptInfo, but it would require UserMapping to have extra field of
its Oid, say umid, to compare them by OID. But IMO creating
UserMapping for every RelOptInfo seems waste of space and time.

Thoughts?
--
Shigeru HANADA

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

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hi Ashutosh,

Sorry for leaving the thread.

2015-07-20 16:09 GMT+09:00 Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>:

In find_user_mapping(), if the first cache search returns a valid tuple, it
is checked twice for validity, un-necessarily. Instead if the first search
returns a valid tuple, it should be returned immediately. I see that this
code was copied from GetUserMapping(), where as well it had the same
problem.

Oops. I changed find_user_mapping to exit immediately when any valid
cache was found.

In build_join_rel(), we check whether user mappings of the two joining
relations are same. If the user mappings are same, doesn't that imply that
the foreign server and local user are same too?

Yes, validity of umid is identical to serverid. We can remove the
check for serverid for some cycles. One idea is to put Assert for
serverid inside the if-statement block.

Rest of the patch looks fine.

Thanks

--
Shigeru HANADA

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

On Tue, Aug 4, 2015 at 2:20 PM, Shigeru Hanada <shigeru.hanada@gmail.com>
wrote:

Hi Ashutosh,

Sorry for leaving the thread.

2015-07-20 16:09 GMT+09:00 Ashutosh Bapat <ashutosh.bapat@enterprisedb.com

:
In find_user_mapping(), if the first cache search returns a valid tuple,

it

is checked twice for validity, un-necessarily. Instead if the first

search

returns a valid tuple, it should be returned immediately. I see that this
code was copied from GetUserMapping(), where as well it had the same
problem.

Oops. I changed find_user_mapping to exit immediately when any valid
cache was found.

Thanks.

In build_join_rel(), we check whether user mappings of the two joining
relations are same. If the user mappings are same, doesn't that imply

that

the foreign server and local user are same too?

Yes, validity of umid is identical to serverid. We can remove the
check for serverid for some cycles. One idea is to put Assert for
serverid inside the if-statement block.

Rest of the patch looks fine.

Thanks

I started reviewing the other patches.

In patch foreign_join_v16.patch, the user mapping structure being passed to
GetConnection() is the one obtained from GetUserMappingById().
GetUserMappingById() constructs the user mapping structure from the user
mapping catalog. For public user mappings, catalog entries have InvalidOid
as userid. Thus, with this patch there is a chance that userid in
UserMapping being passed to GetConnection(), contains InvalidOid as userid.
This is not the case today. The UserMapping structure constructed using
GetUserMapping(Oid userid, Oid serverid) (which ultimately gets passed to
GetConnection()), has the passed in userid and not the one in the catalog.
Is this change intentional?

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hi All,
It's been a long time since last patch on this thread was posted. I have
started
to work on supporting join pushdown for postgres_fdw. Attached please find
three
patches
1. pg_fdw_core.patch: changes in core related to user mapping handling, GUC
enable_foreignjoin
2. pg_fdw_join.patch: postgres_fdw changes for supporting join pushdown
3. pg_join_pd.patch: patch which combines both of these for easy testing.

This mail describes the high level changes included in the patch.

GUC enable_foreignjoin
======================
Like enable_*join GUCs, this GUC when ON allows planner to consider pushing
down
joins to the foreign server. If OFF, foreign join paths will not be
considered.

Building joinrel for joins involving foreign relations
======================================================
RelOptInfo gets a new field umid for user mapping id used for the given
foreign
relation. For base relations it's set to the user mapping of effective user
and
foreign server for that relation. While building RelOptInfo for a join
between
two foreign relations, if server and user mapping of joining relations are
same,
they are copied to the RelOptInfo of the join relation being built. Also,
fdwroutine is set in joinrel to indicate that the core code considers this
join
as pushable. It should suffice just to check equality of the user mapping
oid
since same user mapping implies same server.

Since user mapping oid is available in RelOptInfo, FDWs can get user
mapping information by using this oid, new function GetUserMappingById()
facilitates that.

Generating paths
================
If fdwroutine is set in joinrel, add_paths_to_joinrel() calls
GetForeignJoinPaths hook of corresponding FDW. For postgres_fdw the hook is
implemented by function postgresGetForeignJoinPaths(). Follows the
description
of this function.

In order to avoid considering same joinrel again, fdw_private member of
RelOptInfo is set by this function and populated with FDW specific
information
about joinrel. When this member is set, it indicates that the pushable paths
have been considered for the given joinrel.

A join between two foreign relations is considered safe to push down if
1. The joining sides are pushable
2. The type of join is OUTER or INNER (LEFT/RIGHT/FULL/INNER). SEMI and ANTI
joins are not considered right now, because of difficulties in
constructing
the queries involving those. The join clauses of SEMI/ANTI joins are not
in a
form that can be readily converted to IN/EXISTS/NOT EXIST kind of
expression.
We might consider this as future optimization.
3. Joining sides do not have clauses which can not be pushed down to the
foreign
server. For an OUTER join this is important since those clauses need to
be
applied before performing the join and thus join can not be pushed to the
foreign server. An example is
SELECT * FROM ft1 LEFT JOIN (SELECT * FROM ft2 where local_cond) ft2 ON
(join clause)
Here the local_cond on ft2 needs to be executed before performing LEFT
JOIN
between ft1 and ft2.
This condition can be relaxed for an INNER join by pulling the local
clauses
up the join tree. But this needs more investigation and is not
considered in
this version.
4. The join conditions (e.g. conditions in ON clause) are all safe to push
down.
This is important for OUTER joins as pushing down join clauses partially
and
applying rest locally changes the result. There are ways [1] by which
partial
OUTER join can be completed by applying unpushable clauses locally and
then
nullifying the nullable side and eliminating duplicate non-nullable side
rows. But that's again out of scope of first version of postgres_fdw join
pushdown.

A ForeignPath is created for a safe-to-push-down join. Recursively applying
this
procedure ends in having a single ForeignPath node for whole pushable join
tree,
which may represent a join between more than two foreign tables.

Generating plan
===============
postgresGetForeignPlan() is used to create plan from path chosen by the
optimizer for both joins and base relation scans. This function constructs
the SQL to
be sent to the remote node and create ForeignPlan node.

The function first separates given scan_clauses into pushable and
safe-to-push
clauses. For joinrel baserestrictlist is NIL and we are not considering
parameterized paths, so there are no scan clauses expected. Next the
function
constructs the SQL to be sent to the foreign server. Then it constructs the
ForeignScan node. Rest of this section describes the logic to construct the
SQL
for join; the logic is implemented as function deparseSelectSqlForRel().

deparseSelectSqlForRel() builds the SQL for given joinrel (and now for
baserel
asd well) recursively.
For joinrels
1. it constructs SQL representing either side of join, by calling itself
in recursive fashion.
2. These SQLs are converted into subqueries and become part of the FROM
clause
with appropriate JOIN type and clauses. The left and right subqueries are
given aliases "l" and "r" respectively. The columns in each subquery are
aliased as "a1", "a2", "a3" and so on. Thus the third column on left
side can
be referenced as "l.a3" at any recursion level.
3. Targetlist is added representing the columns in the join result expected
at
that level.
4. The join clauses are added as part of ON clause
5. Any clauses that planner has deemed fit to be evaluated at that level of
join
are added as part of WHERE clause.

It uses the same old logic for deparsing SQL for base relations, except for
the
deparsing the targetlist. When plan is being constructed only for a base
relation, the targetlist (SELECT clause) is constructed by including all the
columns by looking at attrs_used. This does not work when the base relation
is
part of the join being pushed down, since the join targetlist depend upon
the
targetlist in RelOptInfo of base relation and not necessarily the targetlist
obtained from attrs_used.

Row marks
---------
Because of recursive nature of SQL, the names of relations referenced in row
marks are not available at the top level in SQL built for a given top level
join
relation. Hence we have to add FOR SHARE/UPDATE clauses to the subqueries
built
for foreign base relations. This causes all the rows participating in join
(not
the join result) from the base relations to get locked (on foreign server).
Ideally for a top level row mark clause, we should be locking only those
rows
(on foreign server) which are part of the top level join result. But that
requires flattening of the FROM clause constructed, which would require some
signficant intelligence in the deparser code. I have left this out of scope
for
at least this version of the patch.

Examples of remote SQL can be found in the expected output of regression
test postgres_fdw.sql.

Foreign plan execution
======================
For pushed down joins, the tuple descriptor for the result is obtained from
the
targetlist of the plan. Accordingly the error callback and result handling
has
been modified.

Explain output for a pushed down join provides the join expression that it
represents.

TODOs
=====
This patch is very much WIP patch to show case the approach and invite early
comments. I will continue to improve the patch and some of the areas that
will
be improved are
1. Costing of foreign join paths.
2. Various TODOs in the patch, making it more readable, finishing etc.
3. Tests
4. Any comments/suggestions on approach or the attached patch.

In another thread Robert, Fujita-san and Kaigai-san are discussing about
EvalPlanQual support for foreign joins. Corresponding changes to
postgres_fdw
will need to be added once those changes get committed.

Items that will be considered in subsequent patches for 9.6
===========================================================
1. Parameterized paths for join: For regular joins, the parameterized paths
for
joins consider the parameterization of the joining paths. In case of
foreign
join, we do not consider any specific paths for the joining foreign
relations and it acts as a scan. It should be treated as if
parameterizing a
scan and not like regular join. This requires some work and even without
that
the functionality supported by this patch is quite useful. So, I have
left it
out of the scope for this patch and will be considered in subsequent
patch.

2. Foreign join paths with pathkeys: There's my patch pending for
considering
pathkeys for foreign base relation scan. The support for foreign join
paths
with pathkeys will added once that patch gets committed.

Suggestions/comments are welcome.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hi Ashutosh,

On 2015/12/02 20:45, Ashutosh Bapat wrote:

It's been a long time since last patch on this thread was posted. I have
started
to work on supporting join pushdown for postgres_fdw.

Thanks for the work!

Generating paths
================

A join between two foreign relations is considered safe to push down if
1. The joining sides are pushable
2. The type of join is OUTER or INNER (LEFT/RIGHT/FULL/INNER). SEMI and ANTI
joins are not considered right now, because of difficulties in
constructing
the queries involving those. The join clauses of SEMI/ANTI joins are
not in a
form that can be readily converted to IN/EXISTS/NOT EXIST kind of
expression.
We might consider this as future optimization.
3. Joining sides do not have clauses which can not be pushed down to the
foreign
server. For an OUTER join this is important since those clauses need
to be
applied before performing the join and thus join can not be pushed
to the
foreign server. An example is
SELECT * FROM ft1 LEFT JOIN (SELECT * FROM ft2 where local_cond) ft2
ON (join clause)
Here the local_cond on ft2 needs to be executed before performing
LEFT JOIN
between ft1 and ft2.
This condition can be relaxed for an INNER join by pulling the local
clauses
up the join tree. But this needs more investigation and is not
considered in
this version.
4. The join conditions (e.g. conditions in ON clause) are all safe to
push down.
This is important for OUTER joins as pushing down join clauses
partially and
applying rest locally changes the result. There are ways [1] by
which partial
OUTER join can be completed by applying unpushable clauses locally
and then
nullifying the nullable side and eliminating duplicate non-nullable side
rows. But that's again out of scope of first version of postgres_fdw
join
pushdown.

As for 4, as commented in the patch, we could relax the requirement that
all the join conditions (given by JoinPathExtraData's restrictlist) need
to be safe to push down to the remote server;
* In case of inner join, all the conditions would not need to be safe.
* In case of outer join, all the "otherclauses" would not need to be
safe, while I think all the "joinclauses" need to be safe to get the
right results (where "joinclauses" and "otherclauses" are defined by
extract_actual_join_clauses). And I think we should do this relaxation
to some extent for 9.6, to allow more joins to be pushed down. I don't
know about [1]. May I see more information about [1]?

Generating plan
===============

Rest of this section describes the logic to construct
the SQL
for join; the logic is implemented as function deparseSelectSqlForRel().

deparseSelectSqlForRel() builds the SQL for given joinrel (and now for
baserel
asd well) recursively.
For joinrels
1. it constructs SQL representing either side of join, by calling itself
in recursive fashion.
2. These SQLs are converted into subqueries and become part of the FROM
clause
with appropriate JOIN type and clauses. The left and right
subqueries are
given aliases "l" and "r" respectively. The columns in each subquery are
aliased as "a1", "a2", "a3" and so on. Thus the third column on left
side can
be referenced as "l.a3" at any recursion level.
3. Targetlist is added representing the columns in the join result
expected at
that level.
4. The join clauses are added as part of ON clause
5. Any clauses that planner has deemed fit to be evaluated at that level
of join
are added as part of WHERE clause.

Honestly, I'm not sure that that is a good idea. One reason for that is
that a query string constructed by the procedure is difficult to read
especially when the procedure is applied recursively. So, I'm thinking
to revise the procedure so as to construct a query string with a
flattened FROM clause, as discussed in eg, [2]/messages/by-id/CA+TgmoZH9PB8BC+Z3rE7wo8CwuxAF7VP3066iSG39QfR1jJ+UQ@mail.gmail.com.

TODOs
=====
This patch is very much WIP patch to show case the approach and invite early
comments. I will continue to improve the patch and some of the areas
that will
be improved are
1. Costing of foreign join paths.
2. Various TODOs in the patch, making it more readable, finishing etc.
3. Tests
4. Any comments/suggestions on approach or the attached patch.

That would be great!

In another thread Robert, Fujita-san and Kaigai-san are discussing about
EvalPlanQual support for foreign joins. Corresponding changes to
postgres_fdw
will need to be added once those changes get committed.

Yeah, we would need those changes including helper functions to create a
local join execution plan for that support. I'd like to add those
changes to your updated patch if it's okay.

Best regards,
Etsuro Fujita

[2]: /messages/by-id/CA+TgmoZH9PB8BC+Z3rE7wo8CwuxAF7VP3066iSG39QfR1jJ+UQ@mail.gmail.com
/messages/by-id/CA+TgmoZH9PB8BC+Z3rE7wo8CwuxAF7VP3066iSG39QfR1jJ+UQ@mail.gmail.com

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

On Thu, Dec 3, 2015 at 12:36 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:

Hi Ashutosh,

On 2015/12/02 20:45, Ashutosh Bapat wrote:

It's been a long time since last patch on this thread was posted. I have
started
to work on supporting join pushdown for postgres_fdw.

Thanks for the work!

Generating paths

================

A join between two foreign relations is considered safe to push down if

1. The joining sides are pushable
2. The type of join is OUTER or INNER (LEFT/RIGHT/FULL/INNER). SEMI and
ANTI
joins are not considered right now, because of difficulties in
constructing
the queries involving those. The join clauses of SEMI/ANTI joins are
not in a
form that can be readily converted to IN/EXISTS/NOT EXIST kind of
expression.
We might consider this as future optimization.
3. Joining sides do not have clauses which can not be pushed down to the
foreign
server. For an OUTER join this is important since those clauses need
to be
applied before performing the join and thus join can not be pushed
to the
foreign server. An example is
SELECT * FROM ft1 LEFT JOIN (SELECT * FROM ft2 where local_cond) ft2
ON (join clause)
Here the local_cond on ft2 needs to be executed before performing
LEFT JOIN
between ft1 and ft2.
This condition can be relaxed for an INNER join by pulling the local
clauses
up the join tree. But this needs more investigation and is not
considered in
this version.
4. The join conditions (e.g. conditions in ON clause) are all safe to
push down.
This is important for OUTER joins as pushing down join clauses
partially and
applying rest locally changes the result. There are ways [1] by
which partial
OUTER join can be completed by applying unpushable clauses locally
and then
nullifying the nullable side and eliminating duplicate non-nullable
side
rows. But that's again out of scope of first version of postgres_fdw
join
pushdown.

As for 4, as commented in the patch, we could relax the requirement that
all the join conditions (given by JoinPathExtraData's restrictlist) need to
be safe to push down to the remote server;
* In case of inner join, all the conditions would not need to be safe.
* In case of outer join, all the "otherclauses" would not need to be safe,
while I think all the "joinclauses" need to be safe to get the right
results (where "joinclauses" and "otherclauses" are defined by
extract_actual_join_clauses). And I think we should do this relaxation to
some extent for 9.6, to allow more joins to be pushed down.

agreed. I will work on those.

I don't know about [1]. May I see more information about [1]?

Generating plan

===============

Rest of this section describes the logic to construct

the SQL
for join; the logic is implemented as function deparseSelectSqlForRel().

deparseSelectSqlForRel() builds the SQL for given joinrel (and now for
baserel
asd well) recursively.
For joinrels
1. it constructs SQL representing either side of join, by calling itself
in recursive fashion.
2. These SQLs are converted into subqueries and become part of the FROM
clause
with appropriate JOIN type and clauses. The left and right
subqueries are
given aliases "l" and "r" respectively. The columns in each subquery
are
aliased as "a1", "a2", "a3" and so on. Thus the third column on left
side can
be referenced as "l.a3" at any recursion level.
3. Targetlist is added representing the columns in the join result
expected at
that level.
4. The join clauses are added as part of ON clause
5. Any clauses that planner has deemed fit to be evaluated at that level
of join
are added as part of WHERE clause.

Honestly, I'm not sure that that is a good idea. One reason for that is
that a query string constructed by the procedure is difficult to read
especially when the procedure is applied recursively. So, I'm thinking to
revise the procedure so as to construct a query string with a flattened
FROM clause, as discussed in eg, [2].

Just to confirm, the hook discussed in [2] is not in place right? I can
find only one hook for foreign join
50 typedef void (*GetForeignJoinPaths_function) (PlannerInfo *root,
51 RelOptInfo
*joinrel,
52 RelOptInfo
*outerrel,
53 RelOptInfo
*innerrel,
54 JoinType
jointype,
55 JoinPathExtraData
*extra);
This hook takes an inner and outer relation, so can not be used for N-way
join as discussed in that thread.

Are you suggesting that we should add that hook before we implement join
pushdown in postgres_fdw? Am I missing something?

TODOs

=====
This patch is very much WIP patch to show case the approach and invite
early
comments. I will continue to improve the patch and some of the areas
that will
be improved are
1. Costing of foreign join paths.
2. Various TODOs in the patch, making it more readable, finishing etc.
3. Tests
4. Any comments/suggestions on approach or the attached patch.

That would be great!

In another thread Robert, Fujita-san and Kaigai-san are discussing about

EvalPlanQual support for foreign joins. Corresponding changes to
postgres_fdw
will need to be added once those changes get committed.

Yeah, we would need those changes including helper functions to create a
local join execution plan for that support. I'd like to add those changes
to your updated patch if it's okay.

Right now, we do not have any support for postgres_fdw join pushdown. I was
thinking of adding at least minimal support for the same using this patch,
may be by preventing join pushdown in case there are row marks for now.
That way, we at least have some way to play with postgres_fdw join
pushdown. Once we have that, we can work on remaining items listed for 9.6
and also you can add suport for row marks with fix for EvalPlanQual
independently. This will keep the first patch smaller. Do you agree or you
want to see EvalPlanQual fix to be in the first patch itself?

Best regards,
Etsuro Fujita

[2]
/messages/by-id/CA+TgmoZH9PB8BC+Z3rE7wo8CwuxAF7VP3066iSG39QfR1jJ+UQ@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2015/12/08 17:27, Ashutosh Bapat wrote:

On Thu, Dec 3, 2015 at 12:36 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:

Generating paths
================

A join between two foreign relations is considered safe to push
down if

4. The join conditions (e.g. conditions in ON clause) are all
safe to
push down.
This is important for OUTER joins as pushing down join clauses
partially and
applying rest locally changes the result. There are ways [1] by
which partial
OUTER join can be completed by applying unpushable clauses
locally
and then
nullifying the nullable side and eliminating duplicate
non-nullable side
rows. But that's again out of scope of first version of
postgres_fdw
join
pushdown.

As for 4, as commented in the patch, we could relax the requirement
that all the join conditions (given by JoinPathExtraData's
restrictlist) need to be safe to push down to the remote server;
* In case of inner join, all the conditions would not need to be safe.
* In case of outer join, all the "otherclauses" would not need to be
safe, while I think all the "joinclauses" need to be safe to get the
right results (where "joinclauses" and "otherclauses" are defined by
extract_actual_join_clauses). And I think we should do this
relaxation to some extent for 9.6, to allow more joins to be pushed
down.

agreed. I will work on those.

Great!

Generating plan
===============

Rest of this section describes the logic to construct
the SQL
for join; the logic is implemented as function
deparseSelectSqlForRel().

deparseSelectSqlForRel() builds the SQL for given joinrel (and
now for
baserel
asd well) recursively.
For joinrels
1. it constructs SQL representing either side of join, by
calling itself
in recursive fashion.
2. These SQLs are converted into subqueries and become part of
the FROM
clause
with appropriate JOIN type and clauses. The left and right
subqueries are
given aliases "l" and "r" respectively. The columns in each
subquery are
aliased as "a1", "a2", "a3" and so on. Thus the third
column on left
side can
be referenced as "l.a3" at any recursion level.
3. Targetlist is added representing the columns in the join result
expected at
that level.
4. The join clauses are added as part of ON clause
5. Any clauses that planner has deemed fit to be evaluated at
that level
of join
are added as part of WHERE clause.

Honestly, I'm not sure that that is a good idea. One reason for
that is that a query string constructed by the procedure is
difficult to read especially when the procedure is applied
recursively. So, I'm thinking to revise the procedure so as to
construct a query string with a flattened FROM clause, as discussed
in eg, [2].

Just to confirm, the hook discussed in [2] is not in place right? I can
find only one hook for foreign join
50 typedef void (*GetForeignJoinPaths_function) (PlannerInfo *root,
51
RelOptInfo *joinrel,
52 RelOptInfo
*outerrel,
53 RelOptInfo
*innerrel,
54 JoinType
jointype,
55
JoinPathExtraData *extra);
This hook takes an inner and outer relation, so can not be used for
N-way join as discussed in that thread.

Are you suggesting that we should add that hook before we implement join
pushdown in postgres_fdw? Am I missing something?

I don't mean it. I'm thinking that I'll just revise the procedure so as
to generate a FROM clause that is something like "from c left join d on
(...) full join e on (...)" based on the existing hook you mentioned.

TODOs
=====

In another thread Robert, Fujita-san and Kaigai-san are
discussing about
EvalPlanQual support for foreign joins. Corresponding changes to
postgres_fdw
will need to be added once those changes get committed.

Yeah, we would need those changes including helper functions to
create a local join execution plan for that support. I'd like to
add those changes to your updated patch if it's okay.

Right now, we do not have any support for postgres_fdw join pushdown. I
was thinking of adding at least minimal support for the same using this
patch, may be by preventing join pushdown in case there are row marks
for now. That way, we at least have some way to play with postgres_fdw
join pushdown. Once we have that, we can work on remaining items listed
for 9.6 and also you can add suport for row marks with fix for
EvalPlanQual independently. This will keep the first patch smaller. Do
you agree or you want to see EvalPlanQual fix to be in the first patch
itself?

IMO I want to see the EvalPlanQual fix in the first version for 9.6.

Best regards,
Etsuro Fujita

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

On Tue, Dec 8, 2015 at 6:40 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

IMO I want to see the EvalPlanQual fix in the first version for 9.6.

+1.

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

On Wed, Dec 2, 2015 at 6:45 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

It's been a long time since last patch on this thread was posted. I have
started
to work on supporting join pushdown for postgres_fdw. Attached please find
three
patches
1. pg_fdw_core.patch: changes in core related to user mapping handling, GUC
enable_foreignjoin
2. pg_fdw_join.patch: postgres_fdw changes for supporting join pushdown

It seems useful to break things up this way. However, I'm not sure we
want an enable_foreignjoin GUC; in fact, I think we probably don't.
If we want to have a way to control this, postgres_fdw can provide a
custom GUC or FDW option for that.

And to be honest, I haven't really been able to understand why join
pushdown needs changes to user mapping handling. Just hypothetically
speaking, if I put my foot down and said we're not committing any of
that stuff, how and why would that impact our ability to have join
pushdown in 9.6?

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

On Thu, Dec 10, 2015 at 11:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Dec 8, 2015 at 6:40 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

IMO I want to see the EvalPlanQual fix in the first version for 9.6.

+1.

I think there is still a lot functionality that is offered without

EvalPlanQual fix. As long as we do not push joins when there are RowMarks
involved, implementation of that hook is not required. We won't be able to
push down joins for DMLs and when there are FOR SHARE/UPDATE clauses in the
query. And there are huge number of queries, which will be benefitted by
the push down even without that support. There's nothing in this patch,
which comes in way of implementing the EvalPlanQual fix. It can be easily
added after committing the first version. On the other hand, getting
minimal (it's not really minimal, it's much more than that) support for
postgres_fdw support committed opens up possibility to work on multiple
items (as listed in my mail) in parallel.

I am not saying that we do not need EvalPlanQual fix in 9.6. But it's not
needed in the first cut. If we get the first cut in first couple of months
of 2016, there's plenty of room for the fix to go in 9.6. It would be
really bad situation if we could not get postgres_fdw join pushdown
supported in 9.6 because EvalPlanQual hook could not be committed while the
rest of the code is ready. EvalPlanQual fix in core was being discussed
since April 2015. It took 8 months to get that fixed. Hopefully we won't
need that long to implement the hook in postgres_fdw, but that number says
something about the complexity of the feature.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Fri, Dec 11, 2015 at 3:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Dec 2, 2015 at 6:45 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

It's been a long time since last patch on this thread was posted. I have
started
to work on supporting join pushdown for postgres_fdw. Attached please

find

three
patches
1. pg_fdw_core.patch: changes in core related to user mapping handling,

GUC

enable_foreignjoin
2. pg_fdw_join.patch: postgres_fdw changes for supporting join pushdown

It seems useful to break things up this way. However, I'm not sure we
want an enable_foreignjoin GUC; in fact, I think we probably don't.
If we want to have a way to control this, postgres_fdw can provide a
custom GUC or FDW option for that.

enable_foreignjoin or its FDW counterpart would be useful for debugging
purposes just like enable_hashjoin/enable_mergejoin etc. Foreign join push
down can be viewed as a join strategy just like merge/nest loop/hash join
etc. Having enable_foreignjoin complements that picture. Users find more
usage of the same. A user running multiple FDWs and needing to disable join
pushdown across FDWs for any purpose would find enable_foreignjoin very
useful. Needing to turn on/off multiple GUCs would be cumbersome.

And to be honest, I haven't really been able to understand why join
pushdown needs changes to user mapping handling.

Current join pushdown infrastructure in core allows join to be pushed down
if both the sides of join come from the same server. Those sides may have
different user mappings and thus different user properties/access
permissions/visibility on the foreign server. If FDW chooses either of
these different user mappings to push down the join, it will get wrong
results. So, a join between two foreign relations can not be pushed down if
the user mappings on both sides do not match. This has been already
discussed in this thread. I am pasting your response to Hanada-san back in
May 2015 as hint to the discussion
--
2015-05-21 23:11 GMT+09:00 Robert Haas <robertmhaas@gmail.com>:

On Sat, May 16, 2015 at 9:04 AM, Shigeru Hanada
<shigeru.hanada@gmail.com> wrote:

d) All relations must have the same effective user id
This check is done with userid stored in PgFdwRelationInfo, which is
valid only when underlying relations have the same effective user id.
Here "effective user id" means id of the user executing the query, or
the owner of the view when the foreign table is accessed via view.
Tom suggested that it is necessary to check that user mapping matches
for security reason, and now I think it's better than checking
effective user as current postgres_fdw does.

So, should this be a separate patch?

--
To add to that, checking user mapping is better than checking the effective
user id for multiple reasons. Multiple local users can share same public
user mapping, which implies that they share same permissions/visibility for
objects on the foreign server. Join involving two sides with different
effective local user but same user mapping can be pushed down to the
foreign server as same objects/data is going to visible where or not we
push the join down.

Just hypothetically
speaking, if I put my foot down and said we're not committing any of
that stuff, how and why would that impact our ability to have join

pushdown in 9.6?

In fact, the question would be what user mapping should be used by the
connection on which we are firing join query. Unless we answer that
question we won't have join pushdown in 9.6. If we push down joins without
taking into consideration the user mapping, we will have all sorts of
security/data visibility problems because of wrong user properties used for
connection.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2015/12/11 14:16, Ashutosh Bapat wrote:

On Thu, Dec 10, 2015 at 11:20 PM, Robert Haas <robertmhaas@gmail.com
<mailto:robertmhaas@gmail.com>> wrote:

On Tue, Dec 8, 2015 at 6:40 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>>
wrote:

IMO I want to see the EvalPlanQual fix in the first version for 9.6.

+1.

I think there is still a lot functionality that is offered without
EvalPlanQual fix. As long as we do not push joins when there are
RowMarks involved, implementation of that hook is not required. We won't
be able to push down joins for DMLs and when there are FOR SHARE/UPDATE
clauses in the query. And there are huge number of queries, which will
be benefitted by the push down even without that support. There's
nothing in this patch, which comes in way of implementing the
EvalPlanQual fix. It can be easily added after committing the first
version. On the other hand, getting minimal (it's not really minimal,
it's much more than that) support for postgres_fdw support committed
opens up possibility to work on multiple items (as listed in my mail) in
parallel.

I am not saying that we do not need EvalPlanQual fix in 9.6. But it's
not needed in the first cut. If we get the first cut in first couple of
months of 2016, there's plenty of room for the fix to go in 9.6. It
would be really bad situation if we could not get postgres_fdw join
pushdown supported in 9.6 because EvalPlanQual hook could not be
committed while the rest of the code is ready. EvalPlanQual fix in core
was being discussed since April 2015. It took 8 months to get that
fixed. Hopefully we won't need that long to implement the hook in
postgres_fdw, but that number says something about the complexity of the
feature.

ISTM that further enhancements are of secondary importance. Let's do the
EvalPlanQual fix first. I'll add the RecheckForeignScan callback routine
to your version of the postgres_fdw patch as soon as possible.

Best regards,
Etsuro Fujita

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