Re: Join push-down support for foreign tables

Hanada-san,

I checked the patch, below is the random comments from my side.

* Context variables
-------------------
Sorry, I might give you a wrong suggestion.
The foreign_glob_cxt and deparse_expr_cxt were re-defined as follows:

typedef struct foreign_glob_cxt
{
PlannerInfo *root; /* global planner state */
- RelOptInfo *foreignrel; /* the foreign relation we are planning
+ RelOptInfo *outerrel; /* the foreign relation, or outer child
+ RelOptInfo *innerrel; /* inner child, only set for join */
} foreign_glob_cxt;

/*
@@ -86,9 +89,12 @@ typedef struct foreign_loc_cxt
typedef struct deparse_expr_cxt
{
PlannerInfo *root; /* global planner state */
- RelOptInfo *foreignrel; /* the foreign relation we are planning
+ RelOptInfo *outerrel; /* the foreign relation, or outer child
+ RelOptInfo *innerrel; /* inner child, only set for join */
StringInfo buf; /* output buffer to append to */
List **params_list; /* exprs that will become remote Params
+ ForeignScan *outerplan; /* outer child's ForeignScan node */
+ ForeignScan *innerplan; /* inner child's ForeignScan node */
} deparse_expr_cxt;

However, the outerrel does not need to have double-meaning.

RelOptInfo->reloptkind gives us information whether the target
relation is base-relation or join-relation.
So, foreign_expr_walker() can be implemented as follows:
if (bms_is_member(var->varno, glob_cxt->foreignrel->relids) &&
var->varlevelsup == 0)
{
:
also, deparseVar() can checks relation type using:
if (context->foreignrel->reloptkind == RELOPT_JOINREL)
{
deparseJoinVar(...);

In addition, what we need in deparse_expr_cxt are target-list of
outer and inner relation in deparse_expr_cxt.
How about to add inner_tlist/outer_tlist instead of innerplan and
outerplan in deparse_expr_cxt?
The deparseJoinVar() references these fields, but only targetlist.

* GetForeignJoinPath method of FDW
----------------------------------
It should be portion of the interface patch, so I added these
enhancement of FDW APIs with documentation updates.
Please see the newer version of foreign/custom-join interface patch.

* enable_foreignjoin parameter
------------------------------
I'm uncertain whether we should have this GUC parameter that affects
to all FDW implementation. Rather than a built-in one, my preference
is an individual GUC variable defined with DefineCustomBoolVariable(),
by postgres_fdw.
Pros: It offers user more flexible configuration.
Cons: Each FDW has to implement this GUC by itself?

* syntax violated query if empty targetlist
-------------------------------------------
At least NULL shall be injected if no columns are referenced.
Also, add a dummy entry to fdw_ps_tlist to fit slot tuple descriptor.

postgres=# explain verbose select NULL from ft1,ft2 where aid=bid;
QUERY PLAN
---------------------------------------------------------------------------
Foreign Scan (cost=100.00..129.25 rows=2925 width=0)
Output: NULL::unknown
Remote SQL: SELECT FROM (SELECT bid, NULL FROM public.t2) l (a_0, a_1) INNER
JOIN (SELECT aid, NULL FROM public.t1) r (a_0, a_1) ON ((r.a_0 = l.a_0))

* Bug reported by Thom Brown
-----------------------------
# EXPLAIN VERBOSE SELECT NULL FROM (SELECT people.id FROM people INNER JOIN countries ON people.country_id = countries.id LIMIT 3) x;
ERROR: could not open relation with OID 0

Sorry, it was a problem caused by my portion. The patched setrefs.c
checks fdw_/custom_ps_tlist to determine whether Foreign/CustomScan
node is associated with a certain base relation. If *_ps_tlist is
valid, it also expects scanrelid == 0.
However, things we should check is incorrect. We may have a case
with empty *_ps_tlist if remote join expects no columns.
So, I adjusted the condition to check scanrelid instead.

* make_tuple_from_result_row() call
------------------------------------
The 4th argument (newly added) is referenced without NULL checks,
however, store_returning_result() and analyze_row_processor() put
NULL on this argument, then it leads segmentation fault.
RelationGetDescr(fmstate->rel or astate->rel) should be given on
the caller.

* regression test crash
------------------------
The query below gets crashed:
UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
FROM ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 9;

According to the crash dump, tidin() got a cstring input with
unexpected format. I guess column number is incorrectly assigned,
but have no clear scenario at this moment.

#0 0x00007f9b45a11513 in conversion_error_callback (arg=0x7fffc257ecc0)
at postgres_fdw.c:3293
#1 0x00000000008e51d6 in errfinish (dummy=0) at elog.c:436
#2 0x00000000008935cd in tidin (fcinfo=0x7fffc257e8a0) at tid.c:69
/home/kaigai/repo/sepgsql/src/backend/utils/adt/tid.c:69:1734:beg:0x8935cd
(gdb) p str
$6 = 0x1d17cf7 "foo"

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: Shigeru Hanada [mailto:shigeru(dot)hanada(at)gmail(dot)com]
> Sent: Monday, March 02, 2015 9:48 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Robert Haas; PostgreSQL-development
> Subject: ##freemail## Re: [HACKERS] Join push-down support for foreign tables
>
> Attached is the revised/rebased version of the $SUBJECT.
>
> This patch is based on Kaigai-san's custom/foreign join patch, so
> please apply it before this patch. In this version I changed some
> points from original postgres_fdw.
>
> 1) Disabled SELECT clause optimization
> ~9.4 postgres_fdw lists only columns actually used in SELECT clause,
> but AFAIS it makes SQL generation complex. So I disabled such
> optimization and put "NULL" for unnecessary columns in SELECT clause
> of remote query.
>
> 2) Extended deparse context
> To allow deparsing based on multiple source relations, I added some
> members to context structure. They are unnecessary for simple query
> with single foreign table, but IMO it should be integrated.
>
> With Kaigai-san's advise, changes for supporting foreign join on
> postgres_fdw is minimized into postgres_fdw itself. But I added new
> FDW API named GetForeignJoinPaths() to keep the policy that all
> interface between core and FDW should be in FdwRoutine, instead of
> using hook function. Now I'm writing document about it, and will post
> it in a day.
>
> 2015-02-19 16:19 GMT+09:00 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> > 2015-02-17 10:39 GMT+09:00 Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> >> Let me put some comments in addition to where you're checking now.
> >>
> >> [design issues]
> >> * Cost estimation
> >> Estimation and evaluation of cost for remote join query is not an
> >> obvious issue. In principle, local side cannot determine the cost
> >> to run remote join without remote EXPLAIN, because local side has
> >> no information about JOIN logic applied on the remote side.
> >> Probably, we have to put an assumption for remote join algorithm,
> >> because local planner has no idea about remote planner's choice
> >> unless foreign-join don't take "use_remote_estimate".
> >> I think, it is reasonable assumption (even if it is incorrect) to
> >> calculate remote join cost based on local hash-join algorithm.
> >> If user wants more correct estimation, remote EXPLAIN will make
> >> more reliable cost estimation.
> >
> > Hm, I guess that you chose hash-join as "least-costed join". In the
> > pgbench model, most combination between two tables generate hash join
> > as cheapest path. Remote EXPLAIN is very expensive in the context of
> > planning, so it would easily make the plan optimization meaningless.
> > But giving an option to users is good, I agree.
> >
> >>
> >> It also needs a consensus whether cost for remote CPU execution is
> >> equivalent to local CPU. If we think local CPU is rare resource
> >> than remote one, a discount rate will make planner more preferable
> >> to choose remote join than local one
> >
> > Something like cpu_cost_ratio as a new server-level FDW option?
> >
> >>
> >> Once we assume a join algorithm for remote join, unit cost for
> >> remote CPU, we can calculate a cost for foreign join based on
> >> the local join logic plus cost for network translation (maybe
> >> fdw_tuple_cost?).
> >
> > Yes, sum of these costs is the total cost of a remote join.
> > o fdw_startup_cost
> > o hash-join cost, estimated as a local join
> > o fdw_tuple_cost * rows * width
> >
> >> * FDW options
> >> Unlike table scan, FDW options we should refer is unclear.
> >> Table level FDW options are associated with a foreign table as
> >> literal. I think we have two options here:
> >> 1. Foreign-join refers FDW options for foreign-server, but ones
> >> for foreign-tables are ignored.
> >> 2. Foreign-join is prohibited when both of relations don't have
> >> identical FDW options.
> >> My preference is 2. Even though N-way foreign join, it ensures
> >> all the tables involved with (N-1)-way foreign join has identical
> >> FDW options, thus it leads we can make N-way foreign join with
> >> all identical FDW options.
> >> One exception is "updatable" flag of postgres_fdw. It does not
> >> make sense on remote join, so I think mixture of updatable and
> >> non-updatable foreign tables should be admitted, however, it is
> >> a decision by FDW driver.
> >>
> >> Probably, above points need to take time for getting consensus.
> >> I'd like to see your opinion prior to editing your patch.
> >
> > postgres_fdw can't push down a join which contains foreign tables on
> > multiple servers, so use_remote_estimate and fdw_startup_cost are the
> > only FDW options to consider. So we have options for each option.
> >
> > 1-a. If all foreign tables in the join has identical
> > use_remote_estimate, allow pushing down.
> > 1-b. If any of foreign table in the join has true as
> > use_remote_estimate, use remote estimate.
> >
> > 2-a. If all foreign tables in the join has identical fdw_startup_cost,
> > allow pushing down.
> > 2-b. Always use max value in the join. (cost would be more expensive)
> > 2-c. Always use min value in the join. (cost would be cheaper)
> >
> > I prefer 1-a and 2-b, so more joins avoid remote EXPLAIN but have
> > reasonable cost about startup.
> >
> > I agree about "updatable" option.
> >
> >>
> >> [implementation issues]
> >> The interface does not intend to add new Path/Plan type for each scan
> >> that replaces foreign joins. What postgres_fdw should do is, adding
> >> ForeignPath towards a particular joinrel, then it populates ForeignScan
> >> with remote join query once it got chosen by the planner.
> >
> > That idea is interesting, and make many things simpler. Please let me consider.
> >
> >>
> >> A few functions added in src/backend/foreign/foreign.c are not
> >> called by anywhere, at this moment.
> >>
> >> create_plan_recurse() is reverted to static. It is needed for custom-
> >> join enhancement, if no other infrastructure can support.
> >
> > I made it back to static because I thought that create_plan_recurse
> > can be called by core before giving control to FDWs. But I'm not sure
> > it can be applied to custom scans. I'll recheck that part.
> >
> >
> > --
> > Shigeru HANADA
>
>
>
> --
> Shigeru HANADA

Thanks for the detailed comments.

2015-03-03 18:01 GMT+09:00 Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Hanada-san,
>
> I checked the patch, below is the random comments from my side.
>
> * Context variables
> -------------------
> Sorry, I might give you a wrong suggestion.
> The foreign_glob_cxt and deparse_expr_cxt were re-defined as follows:
>
> typedef struct foreign_glob_cxt
> {
> PlannerInfo *root; /* global planner state */
> - RelOptInfo *foreignrel; /* the foreign relation we are planning
> + RelOptInfo *outerrel; /* the foreign relation, or outer child
> + RelOptInfo *innerrel; /* inner child, only set for join */
> } foreign_glob_cxt;
>
> /*
> @@ -86,9 +89,12 @@ typedef struct foreign_loc_cxt
> typedef struct deparse_expr_cxt
> {
> PlannerInfo *root; /* global planner state */
> - RelOptInfo *foreignrel; /* the foreign relation we are planning
> + RelOptInfo *outerrel; /* the foreign relation, or outer child
> + RelOptInfo *innerrel; /* inner child, only set for join */
> StringInfo buf; /* output buffer to append to */
> List **params_list; /* exprs that will become remote Params
> + ForeignScan *outerplan; /* outer child's ForeignScan node */
> + ForeignScan *innerplan; /* inner child's ForeignScan node */
> } deparse_expr_cxt;
>
> However, the outerrel does not need to have double-meaning.
>
> RelOptInfo->reloptkind gives us information whether the target
> relation is base-relation or join-relation.
> So, foreign_expr_walker() can be implemented as follows:
> if (bms_is_member(var->varno, glob_cxt->foreignrel->relids) &&
> var->varlevelsup == 0)
> {
> :
> also, deparseVar() can checks relation type using:
> if (context->foreignrel->reloptkind == RELOPT_JOINREL)
> {
> deparseJoinVar(...);
>
> In addition, what we need in deparse_expr_cxt are target-list of
> outer and inner relation in deparse_expr_cxt.
> How about to add inner_tlist/outer_tlist instead of innerplan and
> outerplan in deparse_expr_cxt?
> The deparseJoinVar() references these fields, but only target list.

Ah, I've totally misunderstood your suggestion. Now I reverted my
changes and use target lists to know whether the var came from either
of the relations.

> * GetForeignJoinPath method of FDW
> ----------------------------------
> It should be portion of the interface patch, so I added these
> enhancement of FDW APIs with documentation updates.
> Please see the newer version of foreign/custom-join interface patch.

Agreed.

> * enable_foreignjoin parameter
> ------------------------------
> I'm uncertain whether we should have this GUC parameter that affects
> to all FDW implementation. Rather than a built-in one, my preference
> is an individual GUC variable defined with DefineCustomBoolVariable(),
> by postgres_fdw.
> Pros: It offers user more flexible configuration.
> Cons: Each FDW has to implement this GUC by itself?

Hum...
In a sense, I added this GUC parameter for debugging purpose. As you
pointed out, users might want to control join push-down feature
per-FDW. I'd like to hear others' opinion.

> * syntax violated query if empty targetlist
> -------------------------------------------
> At least NULL shall be injected if no columns are referenced.
> Also, add a dummy entry to fdw_ps_tlist to fit slot tuple descriptor.
>
> postgres=# explain verbose select NULL from ft1,ft2 where aid=bid;
> QUERY PLAN
> ---------------------------------------------------------------------------
> Foreign Scan (cost=100.00..129.25 rows=2925 width=0)
> Output: NULL::unknown
> Remote SQL: SELECT FROM (SELECT bid, NULL FROM public.t2) l (a_0, a_1) INNER
> JOIN (SELECT aid, NULL FROM public.t1) r (a_0, a_1) ON ((r.a_0 = l.a_0))

Fixed.

> * Bug reported by Thom Brown
> -----------------------------
> # EXPLAIN VERBOSE SELECT NULL FROM (SELECT people.id FROM people INNER JOIN countries ON people.country_id = countries.id LIMIT 3) x;
> ERROR: could not open relation with OID 0
>
> Sorry, it was a problem caused by my portion. The patched setrefs.c
> checks fdw_/custom_ps_tlist to determine whether Foreign/CustomScan
> node is associated with a certain base relation. If *_ps_tlist is
> valid, it also expects scanrelid == 0.
> However, things we should check is incorrect. We may have a case
> with empty *_ps_tlist if remote join expects no columns.
> So, I adjusted the condition to check scanrelid instead.

Is this issue fixed by v5 custom/foreign join patch?

> * make_tuple_from_result_row() call
> ------------------------------------
> The 4th argument (newly added) is referenced without NULL checks,
> however, store_returning_result() and analyze_row_processor() put
> NULL on this argument, then it leads segmentation fault.
> RelationGetDescr(fmstate->rel or astate->rel) should be given on
> the caller.

Fixed.

>
> * regression test crash
> ------------------------
> The query below gets crashed:
> UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
> FROM ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 9;
>
> According to the crash dump, tidin() got a cstring input with
> unexpected format. I guess column number is incorrectly assigned,
> but have no clear scenario at this moment.
>
> #0 0x00007f9b45a11513 in conversion_error_callback (arg=0x7fffc257ecc0)
> at postgres_fdw.c:3293
> #1 0x00000000008e51d6 in errfinish (dummy=0) at elog.c:436
> #2 0x00000000008935cd in tidin (fcinfo=0x7fffc257e8a0) at tid.c:69
> /home/kaigai/repo/sepgsql/src/backend/utils/adt/tid.c:69:1734:beg:0x8935cd
> (gdb) p str
> $6 = 0x1d17cf7 "foo"

Join push-down underlying UPDATE or DELETE requires ctid as its
output, but it seems not fully supported. I'm fixing this issue now.

Regards,
--
Shigeru HANADA

> > * Bug reported by Thom Brown
> > -----------------------------
> > # EXPLAIN VERBOSE SELECT NULL FROM (SELECT people.id FROM people INNER JOIN
> countries ON people.country_id = countries.id LIMIT 3) x;
> > ERROR: could not open relation with OID 0
> >
> > Sorry, it was a problem caused by my portion. The patched setrefs.c
> > checks fdw_/custom_ps_tlist to determine whether Foreign/CustomScan
> > node is associated with a certain base relation. If *_ps_tlist is
> > valid, it also expects scanrelid == 0.
> > However, things we should check is incorrect. We may have a case
> > with empty *_ps_tlist if remote join expects no columns.
> > So, I adjusted the condition to check scanrelid instead.
>
> Is this issue fixed by v5 custom/foreign join patch?
>
Yes, please rebase it.

--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
v6 patch. I posted some comments to v6 patch in this post:

http://www.postgresql.org/message-id/CAEZqfEcNvjqq-P=jxnW1Pb4T9wvpcPoRCN7G6cc46JGuB7dY8w@mail.gmail.com

Before applying my v3 patch, please apply Kaigai-san's v6 patch and my
mod_cjv6.patch.
Sorry for complex patch combination. Those patches will be arranged
soon by Kaigai-san and me.

I fixed the issues pointed out by Thom and Kohei, but still the patch
has an issue about joins underlying UPDATE or DELETE. Now I'm working
on fixing this issue. Besides this issue, existing regression test
passed.

2015-03-03 19:48 GMT+09:00 Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> > * Bug reported by Thom Brown
>> > -----------------------------
>> > # EXPLAIN VERBOSE SELECT NULL FROM (SELECT people.id FROM people INNER JOIN
>> countries ON people.country_id = countries.id LIMIT 3) x;
>> > ERROR: could not open relation with OID 0
>> >
>> > Sorry, it was a problem caused by my portion. The patched setrefs.c
>> > checks fdw_/custom_ps_tlist to determine whether Foreign/CustomScan
>> > node is associated with a certain base relation. If *_ps_tlist is
>> > valid, it also expects scanrelid == 0.
>> > However, things we should check is incorrect. We may have a case
>> > with empty *_ps_tlist if remote join expects no columns.
>> > So, I adjusted the condition to check scanrelid instead.
>>
>> Is this issue fixed by v5 custom/foreign join patch?
>>
> Yes, please rebase it.
>
> --
> NEC OSS Promotion Center / PG-Strom Project
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
>

--
Shigeru HANADA

On 3 March 2015 at 12:34, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> wrote:
> I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
> v6 patch. I posted some comments to v6 patch in this post:
>
> http://www.postgresql.org/message-id/CAEZqfEcNvjqq-P=jxnW1Pb4T9wvpcPoRCN7G6cc46JGuB7dY8w@mail.gmail.com
>
> Before applying my v3 patch, please apply Kaigai-san's v6 patch and my
> mod_cjv6.patch.
> Sorry for complex patch combination. Those patches will be arranged
> soon by Kaigai-san and me.
>
> I fixed the issues pointed out by Thom and Kohei, but still the patch
> has an issue about joins underlying UPDATE or DELETE. Now I'm working
> on fixing this issue. Besides this issue, existing regression test
> passed.

Re-tested the broken query and it works for me now.

--
Thom

On 2015/03/03 21:34, Shigeru Hanada wrote:
> I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
> v6 patch.

Thanks for the work, Hanada-san and KaiGai-san!

Maybe I'm missing something, but did we agree to take this approach, ie,
"join push-down" on top of custom join? There is a comment ahout that
[1]. I just thought it'd be better to achieve a consensus before
implementing the feature further.

> but still the patch
> has an issue about joins underlying UPDATE or DELETE. Now I'm working
> on fixing this issue.

Is that something like "UPDATE foo ... FROM bar ..." where both foo and
bar are remote? If so, I think it'd be better to push such an update
down to the remote, as discussed in [2], and I'd like to work on that
together!

Sorry for having been late for the party.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/23343.1418658355@sss.pgh.pa.us
[2] http://www.postgresql.org/message-id/31942.1410534785@sss.pgh.pa.us

> On 2015/03/03 21:34, Shigeru Hanada wrote:
> > I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
> > v6 patch.
>
> Thanks for the work, Hanada-san and KaiGai-san!
>
> Maybe I'm missing something, but did we agree to take this approach, ie,
> "join push-down" on top of custom join? There is a comment ahout that
> [1]. I just thought it'd be better to achieve a consensus before
> implementing the feature further.
>
It is not correct. The join push-down feature is not implemented
on top of the custom-join feature, however, both of them are 99%
similar on both of the concept and implementation.
So, we're working to enhance foreign/custom-join interface together,
according to Robert's suggestion [3], using postgres_fdw extension
as a minimum worthwhile example for both of foreign/custom-scan.

[3] http://bit.ly/1w1PoDU

> > but still the patch
> > has an issue about joins underlying UPDATE or DELETE. Now I'm working
> > on fixing this issue.
>
> Is that something like "UPDATE foo ... FROM bar ..." where both foo and
> bar are remote? If so, I think it'd be better to push such an update
> down to the remote, as discussed in [2], and I'd like to work on that
> together!
>
Hanada-san, could you give us test query to reproduce the problem
above? I and Fujita-san can help to investigate the problem from
different standpoints for each.

> Sorry for having been late for the party.
>
We are still in the party.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/03/04 17:31, Kouhei Kaigai wrote:
>> On 2015/03/03 21:34, Shigeru Hanada wrote:
>>> I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
>>> v6 patch.

>> Maybe I'm missing something, but did we agree to take this approach, ie,
>> "join push-down" on top of custom join? There is a comment ahout that
>> [1]. I just thought it'd be better to achieve a consensus before
>> implementing the feature further.

> It is not correct. The join push-down feature is not implemented
> on top of the custom-join feature, however, both of them are 99%
> similar on both of the concept and implementation.
> So, we're working to enhance foreign/custom-join interface together,
> according to Robert's suggestion [3], using postgres_fdw extension
> as a minimum worthwhile example for both of foreign/custom-scan.

OK, thanks for the explanation!

>>> but still the patch
>>> has an issue about joins underlying UPDATE or DELETE. Now I'm working
>>> on fixing this issue.

>> Is that something like "UPDATE foo ... FROM bar ..." where both foo and
>> bar are remote? If so, I think it'd be better to push such an update
>> down to the remote, as discussed in [2], and I'd like to work on that
>> together!

> Hanada-san, could you give us test query to reproduce the problem
> above? I and Fujita-san can help to investigate the problem from
> different standpoints for each.

Yeah, will do.

Best regards,
Etsuro Fujita

2015-03-04 17:00 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> On 2015/03/03 21:34, Shigeru Hanada wrote:
>>
>> I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
>> v6 patch.
>
>
> Thanks for the work, Hanada-san and KaiGai-san!
>
> Maybe I'm missing something, but did we agree to take this approach, ie,
> "join push-down" on top of custom join? There is a comment ahout that [1].
> I just thought it'd be better to achieve a consensus before implementing the
> feature further.

As Kaigai-san says, foreign join push-down is beside custom scan, and
they are on the custom/foreign join api patch.

>
>> but still the patch
>> has an issue about joins underlying UPDATE or DELETE. Now I'm working
>> on fixing this issue.
>
>
> Is that something like "UPDATE foo ... FROM bar ..." where both foo and bar
> are remote? If so, I think it'd be better to push such an update down to
> the remote, as discussed in [2], and I'd like to work on that together!

A part of it, perhaps. But at the moment I see many issues to solve
around pushing down complex UPDATE/DELETE. So I once tightened the
restriction, that joins between foreign tables are pushed down only if
they are part of SELECT statement. Please see next v4 patch I'll post
soon.

>
> Sorry for having been late for the party.
>
> Best regards,
> Etsuro Fujita
>
> [1] http://www.postgresql.org/message-id/23343.1418658355@sss.pgh.pa.us
> [2] http://www.postgresql.org/message-id/31942.1410534785@sss.pgh.pa.us

--
Shigeru HANADA

On 2015/03/04 17:57, Shigeru Hanada wrote:
> 2015-03-04 17:00 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
>> On 2015/03/03 21:34, Shigeru Hanada wrote:
>>> I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
>>> v6 patch.

>>> but still the patch
>>> has an issue about joins underlying UPDATE or DELETE. Now I'm working
>>> on fixing this issue.

>> Is that something like "UPDATE foo ... FROM bar ..." where both foo and bar
>> are remote? If so, I think it'd be better to push such an update down to
>> the remote, as discussed in [2], and I'd like to work on that together!

> A part of it, perhaps. But at the moment I see many issues to solve
> around pushing down complex UPDATE/DELETE. So I once tightened the
> restriction, that joins between foreign tables are pushed down only if
> they are part of SELECT statement. Please see next v4 patch I'll post
> soon.

OK, thanks for the reply!

Best regards,
Etsuro Fujita

Here is v4 patch of Join push-down support for foreign tables. This
patch requires Custom/Foreign join patch v7 posted by Kaigai-san.

In this version I added check about query type which gives up pushing
down joins when the join is a part of an underlying query of
UPDATE/DELETE.

As of now postgres_fdw builds a proper remote query but it can't bring
ctid value up to postgresExecForeignUpdate()...

I'm still working on supporting such query, but I'm not sure that
supporting UPDATE/DELETE is required in the first version. I attached
a patch foreign_join_update.patch to sure WIP for supporting
update/delete as top of foreign joins.

How to reproduce the error, please execute query below after running
attached init_fdw.sql for building test environment. Note that the
script drops "user1", and creates database "fdw" and "pgbench".

fdw=# explain (verbose) update pgbench_branches b set filler = 'foo'
from pgbench_tellers t where t.bid = b.bid and t.tid < 10;

QUERY
PLAN

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
---------------------------------------------------------------------------------------------------------------
Update on public.pgbench_branches b (cost=100.00..100.67 rows=67 width=390)
Remote SQL: UPDATE public.pgbench_branches SET filler = $2 WHERE ctid = $1
-> Foreign Scan (cost=100.00..100.67 rows=67 width=390)
Output: b.bid, b.bbalance, 'foo

'::character(88), b.ctid, *
Remote SQL: SELECT r.a_0, r.a_1, r.a_2, l FROM (SELECT tid,
bid, tbalance, filler FROM public.pgbench_tellers WHERE ((tid < 10)))
l (a_0, a_1) INNER JOIN (SELECT b
id, bbalance, NULL, ctid FROM public.pgbench_branches FOR UPDATE) r
(a_0, a_1, a_2, a_3) ON ((r.a_0 = l.a_1))
(5 rows)
fdw=# explain (analyze, verbose) update pgbench_branches b set filler
= 'foo' from pgbench_tellers t where t.bid = b.bid and t.tid < 10;
ERROR: ctid is NULL

2015-03-03 21:34 GMT+09:00 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
> v6 patch. I posted some comments to v6 patch in this post:
>
> http://www.postgresql.org/message-id/CAEZqfEcNvjqq-P=jxnW1Pb4T9wvpcPoRCN7G6cc46JGuB7dY8w@mail.gmail.com
>
> Before applying my v3 patch, please apply Kaigai-san's v6 patch and my
> mod_cjv6.patch.
> Sorry for complex patch combination. Those patches will be arranged
> soon by Kaigai-san and me.
>
> I fixed the issues pointed out by Thom and Kohei, but still the patch
> has an issue about joins underlying UPDATE or DELETE. Now I'm working
> on fixing this issue. Besides this issue, existing regression test
> passed.
>
> 2015-03-03 19:48 GMT+09:00 Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> > * Bug reported by Thom Brown
>>> > -----------------------------
>>> > # EXPLAIN VERBOSE SELECT NULL FROM (SELECT people.id FROM people INNER JOIN
>>> countries ON people.country_id = countries.id LIMIT 3) x;
>>> > ERROR: could not open relation with OID 0
>>> >
>>> > Sorry, it was a problem caused by my portion. The patched setrefs.c
>>> > checks fdw_/custom_ps_tlist to determine whether Foreign/CustomScan
>>> > node is associated with a certain base relation. If *_ps_tlist is
>>> > valid, it also expects scanrelid == 0.
>>> > However, things we should check is incorrect. We may have a case
>>> > with empty *_ps_tlist if remote join expects no columns.
>>> > So, I adjusted the condition to check scanrelid instead.
>>>
>>> Is this issue fixed by v5 custom/foreign join patch?
>>>
>> Yes, please rebase it.
>>
>> --
>> NEC OSS Promotion Center / PG-Strom Project
>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
>>
>
>
>
> --
> Shigeru HANADA

--
Shigeru HANADA

On Wed, Mar 4, 2015 at 4:26 AM, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> wrote:
> Here is v4 patch of Join push-down support for foreign tables. This
> patch requires Custom/Foreign join patch v7 posted by Kaigai-san.

Hi,

I just want to point out to the folks on this thread that the action
in this area is happening on the other thread, about the
custom/foreign join patch, and that Tom and I are suspecting that we
do not have the right design here. Your input is needed.

From my end, I am quite skeptical about the way
postgresGetForeignJoinPath in this patch works. It looks only at the
cheapest total path of the relations to be joined, which seems like it
could easily be wrong. What about some other path that is more
expensive but provides a convenient sort order? What about something
like A LEFT JOIN (B JOIN C ON B.x = C.x) ON A.y = B.y AND A.z = C.z,
which can't make a legal join until level 3? Tom's proposed hook
placement would instead invoke the FDW once per joinrel, passing root
and the joinrel. Then, you could cost a path based on the idea of
pushing that join entirely to the remote side, or exit without doing
anything if pushdown is not feasible.

Please read the other thread and then respond either there or here
with thoughts on that design. If you don't provide some input on
this, both of these patches are going to get rejected as lacking
consensus, and we'll move on to other things. I'd really rather not
ship yet another release without this important feature, but that's
where we're heading if we can't talk this through.

Thanks,

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

On Mon, Mar 16, 2015 at 9:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Mar 4, 2015 at 4:26 AM, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> wrote:
>> Here is v4 patch of Join push-down support for foreign tables. This
>> patch requires Custom/Foreign join patch v7 posted by Kaigai-san.
>
> Hi,
>
> I just want to point out to the folks on this thread that the action
> in this area is happening on the other thread, about the
> custom/foreign join patch, and that Tom and I are suspecting that we
> do not have the right design here. Your input is needed.
>
> [...]

Moved for now to the next CF as it was in state "Need review".
--
Michael