Re: Foreign join pushdown vs EvalPlanQual

Hi,

While reviewing the foreign join pushdown core patch, I noticed that the
patch doesn't perform an EvalPlanQual recheck properly. The example
that crashes the server will be shown below (it uses the postgres_fdw
patch [1]). I think the reason for that is because the ForeignScan node
performing the foreign join remotely has scanrelid = 0 while
ExecScanFetch assumes that its scan node has scanrelid > 0.

I think this is a bug. I've not figured out how to fix this yet, but I
thought we would also need another plan that evaluates the join locally
for the test tuples for EvalPlanQual. Though I'm missing something though.

Create an environment:

postgres=# create table tab (a int, b int);
CREATE TABLE
postgres=# create foreign table foo (a int) server myserver options
(table_name 'foo');
CREATE FOREIGN TABLE
postgres=# create foreign table bar (a int) server myserver options
(table_name 'bar');
CREATE FOREIGN TABLE
postgres=# insert into tab values (1, 1);
INSERT 0 1
postgres=# insert into foo values (1);
INSERT 0 1
postgres=# insert into bar values (1);
INSERT 0 1
postgres=# analyze tab;
ANALYZE
postgres=# analyze foo;
ANALYZE
postgres=# analyze bar;
ANALYZE

Run the example:

[Terminal 1]
postgres=# begin;
BEGIN
postgres=# update tab set b = b + 1 where a = 1;
UPDATE 1

[Terminal 2]
postgres=# explain verbose select tab.* from tab, foo, bar where tab.a =
foo.a and foo.a = bar.a for update;

QUERY PLAN

--------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------
LockRows (cost=100.00..101.18 rows=4 width=70)
Output: tab.a, tab.b, tab.ctid, foo.*, bar.*
-> Nested Loop (cost=100.00..101.14 rows=4 width=70)
Output: tab.a, tab.b, tab.ctid, foo.*, bar.*
Join Filter: (foo.a = tab.a)
-> Seq Scan on public.tab (cost=0.00..1.01 rows=1 width=14)
Output: tab.a, tab.b, tab.ctid
-> Foreign Scan (cost=100.00..100.08 rows=4 width=64)
Output: foo.*, foo.a, bar.*, bar.a
Relations: (public.foo) INNER JOIN (public.bar)
Remote SQL: SELECT l.a1, l.a2, r.a1, r.a2 FROM (SELECT
ROW(l.a9), l.a9 FROM (SELECT a a9 FROM public.foo FOR UPDATE) l) l (a1,
a2) INNER
JOIN (SELECT ROW(r.a9), r.a9 FROM (SELECT a a9 FROM public.bar FOR
UPDATE) r) r (a1, a2) ON ((l.a2 = r.a2))
(11 rows)

postgres=# select tab.* from tab, foo, bar where tab.a = foo.a and foo.a
= bar.a for update;

[Terminal 1]
postgres=# commit;
COMMIT

[Terminal 2]
(After the commit in Terminal 1, Terminal 2 will show the following.)
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Best regards,
Etsuro Fujita

[1]
http://www.postgresql.org/message-id/CAEZqfEe9KGy=1_waGh2rgZPg0o4pqgD+iauYaj8wTze+CYJUHg@mail.gmail.com

Does it make sense to put the result tuple of remote join on evety
estate->es_epqTupleSet[] slot represented by this ForeignScan if
scanrelid==0?

It allows to recheck qualifier for each LockRow that intends to lock
base foreign table underlying the remote join.
ForeignScan->fdw_relids tells us which rtindexes are represented
by this ForeignScan, so infrastructure side may be able to handle.

Thanks,

2015-06-24 11:40 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> Hi,
>
> While reviewing the foreign join pushdown core patch, I noticed that the
> patch doesn't perform an EvalPlanQual recheck properly. The example
> that crashes the server will be shown below (it uses the postgres_fdw
> patch [1]). I think the reason for that is because the ForeignScan node
> performing the foreign join remotely has scanrelid = 0 while
> ExecScanFetch assumes that its scan node has scanrelid > 0.
>
> I think this is a bug. I've not figured out how to fix this yet, but I
> thought we would also need another plan that evaluates the join locally
> for the test tuples for EvalPlanQual. Though I'm missing something though.
>
> Create an environment:
>
> postgres=# create table tab (a int, b int);
> CREATE TABLE
> postgres=# create foreign table foo (a int) server myserver options
> (table_name 'foo');
> CREATE FOREIGN TABLE
> postgres=# create foreign table bar (a int) server myserver options
> (table_name 'bar');
> CREATE FOREIGN TABLE
> postgres=# insert into tab values (1, 1);
> INSERT 0 1
> postgres=# insert into foo values (1);
> INSERT 0 1
> postgres=# insert into bar values (1);
> INSERT 0 1
> postgres=# analyze tab;
> ANALYZE
> postgres=# analyze foo;
> ANALYZE
> postgres=# analyze bar;
> ANALYZE
>
> Run the example:
>
> [Terminal 1]
> postgres=# begin;
> BEGIN
> postgres=# update tab set b = b + 1 where a = 1;
> UPDATE 1
>
> [Terminal 2]
> postgres=# explain verbose select tab.* from tab, foo, bar where tab.a =
> foo.a and foo.a = bar.a for update;
>
> QUERY PLAN
>
> --------------------------------------------------------------------------------------------------------------------------------------------------------
> ------------------------------------------------------------------------------------------------------------
> LockRows (cost=100.00..101.18 rows=4 width=70)
> Output: tab.a, tab.b, tab.ctid, foo.*, bar.*
> -> Nested Loop (cost=100.00..101.14 rows=4 width=70)
> Output: tab.a, tab.b, tab.ctid, foo.*, bar.*
> Join Filter: (foo.a = tab.a)
> -> Seq Scan on public.tab (cost=0.00..1.01 rows=1 width=14)
> Output: tab.a, tab.b, tab.ctid
> -> Foreign Scan (cost=100.00..100.08 rows=4 width=64)
> Output: foo.*, foo.a, bar.*, bar.a
> Relations: (public.foo) INNER JOIN (public.bar)
> Remote SQL: SELECT l.a1, l.a2, r.a1, r.a2 FROM (SELECT
> ROW(l.a9), l.a9 FROM (SELECT a a9 FROM public.foo FOR UPDATE) l) l (a1,
> a2) INNER
> JOIN (SELECT ROW(r.a9), r.a9 FROM (SELECT a a9 FROM public.bar FOR
> UPDATE) r) r (a1, a2) ON ((l.a2 = r.a2))
> (11 rows)
>
> postgres=# select tab.* from tab, foo, bar where tab.a = foo.a and foo.a
> = bar.a for update;
>
> [Terminal 1]
> postgres=# commit;
> COMMIT
>
> [Terminal 2]
> (After the commit in Terminal 1, Terminal 2 will show the following.)
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> !>
>
> Best regards,
> Etsuro Fujita
>
> [1]
> http://www.postgresql.org/message-id/CAEZqfEe9KGy=1_waGh2rgZPg0o4pqgD+iauYaj8wTze+CYJUHg@mail.gmail.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Fujita-san,

> Does it make sense to put the result tuple of remote join on evety
> estate->es_epqTupleSet[] slot represented by this ForeignScan if
> scanrelid==0?
>
Sorry, I misunderstood behavior of the es_epqTupleSet[].

I'd like to suggest a solution that re-construct remote tuple according
to the fdw_scan_tlist on ExecScanFetch, if given scanrelid == 0.
It enables to run local qualifier associated with the ForeignScan node,
and it will also work for the case when tuple in es_epqTupleSet[] was
local heap.

For details:
The es_epqTuple[] is set by EvalPlanQualSetTuple(). It put a tuple
exactly reflects a particular base relation (that has positive rtindex).
Even if it is a foreign-table, ExecLockRows() put a tuple dynamically
constructed via whole-row-reference at EvalPlanQualFetchRowMarks().
So, regardless of copy or reference to heap, we can expect es_epqTuple[]
keeps tuples of the base relations for each.

On the other hands, ForeignScan that replaced local join by remote
join has a valid fdw_scan_tlist list. It contains expression node
to construct individual attribute of the pseudo scan target-list.

So, all we need to do is, (1) if scanrelid == 0 on ExecScanFetch(),
(2) it should be ForeignScan or CustomScan, with *_scan_tlist.
(3) then, we reconstruct a tuple of the pseudo scan based on the
*_scan_tlist, instead of simple reference to es_epqTupleSet[],
(4) and, evaluate local qualifiers of the node.

How about your thought?

BTW, if you try newer version of postgres_fdw foreign join patch,
please provide me to reproduce the problem/

Also, as an aside, postgres_fdw does not implement RefetchForeignRow()
at this moment. Does it make a problem?

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Kohei KaiGai
> Sent: Wednesday, June 24, 2015 10:02 PM
> To: Etsuro Fujita
> Cc: PostgreSQL-development
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> Does it make sense to put the result tuple of remote join on evety
> estate->es_epqTupleSet[] slot represented by this ForeignScan if
> scanrelid==0?
>
> It allows to recheck qualifier for each LockRow that intends to lock
> base foreign table underlying the remote join.
> ForeignScan->fdw_relids tells us which rtindexes are represented
> by this ForeignScan, so infrastructure side may be able to handle.
>
> Thanks,
>
>
> 2015-06-24 11:40 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> > Hi,
> >
> > While reviewing the foreign join pushdown core patch, I noticed that the
> > patch doesn't perform an EvalPlanQual recheck properly. The example
> > that crashes the server will be shown below (it uses the postgres_fdw
> > patch [1]). I think the reason for that is because the ForeignScan node
> > performing the foreign join remotely has scanrelid = 0 while
> > ExecScanFetch assumes that its scan node has scanrelid > 0.
> >
> > I think this is a bug. I've not figured out how to fix this yet, but I
> > thought we would also need another plan that evaluates the join locally
> > for the test tuples for EvalPlanQual. Though I'm missing something though.
> >
> > Create an environment:
> >
> > postgres=# create table tab (a int, b int);
> > CREATE TABLE
> > postgres=# create foreign table foo (a int) server myserver options
> > (table_name 'foo');
> > CREATE FOREIGN TABLE
> > postgres=# create foreign table bar (a int) server myserver options
> > (table_name 'bar');
> > CREATE FOREIGN TABLE
> > postgres=# insert into tab values (1, 1);
> > INSERT 0 1
> > postgres=# insert into foo values (1);
> > INSERT 0 1
> > postgres=# insert into bar values (1);
> > INSERT 0 1
> > postgres=# analyze tab;
> > ANALYZE
> > postgres=# analyze foo;
> > ANALYZE
> > postgres=# analyze bar;
> > ANALYZE
> >
> > Run the example:
> >
> > [Terminal 1]
> > postgres=# begin;
> > BEGIN
> > postgres=# update tab set b = b + 1 where a = 1;
> > UPDATE 1
> >
> > [Terminal 2]
> > postgres=# explain verbose select tab.* from tab, foo, bar where tab.a =
> > foo.a and foo.a = bar.a for update;
> >
> > QUERY PLAN
> >
> >
> ----------------------------------------------------------------------------
> ----------------------------------------------------------------------------
> >
> ----------------------------------------------------------------------------
> --------------------------------
> > LockRows (cost=100.00..101.18 rows=4 width=70)
> > Output: tab.a, tab.b, tab.ctid, foo.*, bar.*
> > -> Nested Loop (cost=100.00..101.14 rows=4 width=70)
> > Output: tab.a, tab.b, tab.ctid, foo.*, bar.*
> > Join Filter: (foo.a = tab.a)
> > -> Seq Scan on public.tab (cost=0.00..1.01 rows=1 width=14)
> > Output: tab.a, tab.b, tab.ctid
> > -> Foreign Scan (cost=100.00..100.08 rows=4 width=64)
> > Output: foo.*, foo.a, bar.*, bar.a
> > Relations: (public.foo) INNER JOIN (public.bar)
> > Remote SQL: SELECT l.a1, l.a2, r.a1, r.a2 FROM (SELECT
> > ROW(l.a9), l.a9 FROM (SELECT a a9 FROM public.foo FOR UPDATE) l) l (a1,
> > a2) INNER
> > JOIN (SELECT ROW(r.a9), r.a9 FROM (SELECT a a9 FROM public.bar FOR
> > UPDATE) r) r (a1, a2) ON ((l.a2 = r.a2))
> > (11 rows)
> >
> > postgres=# select tab.* from tab, foo, bar where tab.a = foo.a and foo.a
> > = bar.a for update;
> >
> > [Terminal 1]
> > postgres=# commit;
> > COMMIT
> >
> > [Terminal 2]
> > (After the commit in Terminal 1, Terminal 2 will show the following.)
> > server closed the connection unexpectedly
> > This probably means the server terminated abnormally
> > before or while processing the request.
> > The connection to the server was lost. Attempting reset: Failed.
> > !>
> >
> > Best regards,
> > Etsuro Fujita
> >
> > [1]
> >
> http://www.postgresql.org/message-id/CAEZqfEe9KGy=1_waGh2rgZPg0o4pqgD+iauYaj
> 8wTze+CYJUHg(at)mail(dot)gmail(dot)com
> >
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
>
>
>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Hi KaiGai-san,

I'd like to work on this issue with you!

On 2015/06/25 10:48, Kouhei Kaigai wrote:
> I'd like to suggest a solution that re-construct remote tuple according
> to the fdw_scan_tlist on ExecScanFetch, if given scanrelid == 0.
> It enables to run local qualifier associated with the ForeignScan node,
> and it will also work for the case when tuple in es_epqTupleSet[] was
> local heap.

Maybe I'm missing something, but I don't think your proposal works
properly because we don't have any component ForeignScan state node or
subsidiary join state node once we've replaced the entire join with the
ForeignScan performing the join remotely, IIUC. So, my image was to
have another subplan for EvalPlanQual as well as the ForeignScan, to do
the entire join locally for the component test tuples if we are inside
an EvalPlanQual recheck.

> BTW, if you try newer version of postgres_fdw foreign join patch,
> please provide me to reproduce the problem/

OK

> Also, as an aside, postgres_fdw does not implement RefetchForeignRow()
> at this moment. Does it make a problem?

I don't think so, though I think it would be better to test that the
foreign join pushdown API patch also allows late row locking using the
postgres_fdw.

Best regards,
Etsuro Fujita

Fujita-san,

> > BTW, if you try newer version of postgres_fdw foreign join patch,
> > please provide me to reproduce the problem/
>
> OK
>
Did you forget to attach the patch, or v17 is in use?

> > I'd like to suggest a solution that re-construct remote tuple according
> > to the fdw_scan_tlist on ExecScanFetch, if given scanrelid == 0.
> > It enables to run local qualifier associated with the ForeignScan node,
> > and it will also work for the case when tuple in es_epqTupleSet[] was
> > local heap.
>
> Maybe I'm missing something, but I don't think your proposal works
> properly because we don't have any component ForeignScan state node or
> subsidiary join state node once we've replaced the entire join with the
> ForeignScan performing the join remotely, IIUC. So, my image was to
> have another subplan for EvalPlanQual as well as the ForeignScan, to do
> the entire join locally for the component test tuples if we are inside
> an EvalPlanQual recheck.
>
Hmm... Probably, we have two standpoints to tackle the problem.

The first standpoint tries to handle the base foreign table as
a prime relation for locking. Thus, we have to provide a way to
fetch a remote tuple identified with the supplied ctid.
The advantage of this approach is the way to fetch tuples from
base relation is quite similar to the existing form, however,
its disadvantage is another side of the same coin, because the
ForeignScan node with scanrelid==0 (that represents remote join
query) may have local qualifiers which shall run on the tuple
according to fdw_scan_tlist.

One other standpoint tries to handle a bunch of base foreign
tables as a unit. That means, if any of base foreign table is
the target of locking, it prompts FDW driver to fetch the latest
"joined" tuple identified by "ctid", even if this join contains
multiple base relations to be locked.
The advantage of this approach is that we can use qualifiers of
the ForeignScan node with scanrelid==0 and no need to pay attention
of remote qualifier and/or join condition individually.
Its disadvantage is, we may extend EState structure to keep the
"joined" tuples, in addition to es_epqTupleSet[].

I'm inclined to think the later standpoint works well, because
it does not need to reproduce an alternative execution path in
local side again, even if a ForeignScan node represents much
complicated remote query.
If we would fetch tuples of individual base relations, we need
to reconstruct identical join path to be executed on remote-
side, don't it?

IIUC, the purpose of EvalPlanQual() is to ensure the tuples to
be locked is still visible, so it is not an essential condition
to fetch base tuples individually.

Just an aside, please tell me if someone know, does EvalPlanQual
logic work correctly even if the tuple to be locked located in
the right tree of HashJoin?
In this case, it seems to me ExecHashJoin does not refresh Hash
table again even if ExecProcNode() is invoked with es_epqTupleSet[],
thus, old tuple is already visible and checked, isn't it?

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> Sent: Thursday, June 25, 2015 3:12 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: PostgreSQL-development
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> Hi KaiGai-san,
>
> I'd like to work on this issue with you!
>
> On 2015/06/25 10:48, Kouhei Kaigai wrote:
> > I'd like to suggest a solution that re-construct remote tuple according
> > to the fdw_scan_tlist on ExecScanFetch, if given scanrelid == 0.
> > It enables to run local qualifier associated with the ForeignScan node,
> > and it will also work for the case when tuple in es_epqTupleSet[] was
> > local heap.
>
> Maybe I'm missing something, but I don't think your proposal works
> properly because we don't have any component ForeignScan state node or
> subsidiary join state node once we've replaced the entire join with the
> ForeignScan performing the join remotely, IIUC. So, my image was to
> have another subplan for EvalPlanQual as well as the ForeignScan, to do
> the entire join locally for the component test tuples if we are inside
> an EvalPlanQual recheck.
>
> > BTW, if you try newer version of postgres_fdw foreign join patch,
> > please provide me to reproduce the problem/
>
> OK
>
> > Also, as an aside, postgres_fdw does not implement RefetchForeignRow()
> > at this moment. Does it make a problem?
>
> I don't think so, though I think it would be better to test that the
> foreign join pushdown API patch also allows late row locking using the
> postgres_fdw.
>
> Best regards,
> Etsuro Fujita
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Hi KaiGai-san,

On 2015/06/27 21:09, Kouhei Kaigai wrote:
>>> BTW, if you try newer version of postgres_fdw foreign join patch,
>>> please provide me to reproduce the problem/

>> OK

> Did you forget to attach the patch, or v17 is in use?

Sorry, I made a mistake. The problem was produced using v16 [1].

>>> I'd like to suggest a solution that re-construct remote tuple according
>>> to the fdw_scan_tlist on ExecScanFetch, if given scanrelid == 0.
>>> It enables to run local qualifier associated with the ForeignScan node,
>>> and it will also work for the case when tuple in es_epqTupleSet[] was
>>> local heap.

>> Maybe I'm missing something, but I don't think your proposal works
>> properly because we don't have any component ForeignScan state node or
>> subsidiary join state node once we've replaced the entire join with the
>> ForeignScan performing the join remotely, IIUC. So, my image was to
>> have another subplan for EvalPlanQual as well as the ForeignScan, to do
>> the entire join locally for the component test tuples if we are inside
>> an EvalPlanQual recheck.

> Hmm... Probably, we have two standpoints to tackle the problem.
>
> The first standpoint tries to handle the base foreign table as
> a prime relation for locking. Thus, we have to provide a way to
> fetch a remote tuple identified with the supplied ctid.
> The advantage of this approach is the way to fetch tuples from
> base relation is quite similar to the existing form, however,
> its disadvantage is another side of the same coin, because the
> ForeignScan node with scanrelid==0 (that represents remote join
> query) may have local qualifiers which shall run on the tuple
> according to fdw_scan_tlist.

IIUC, I think this approach would also need to evaluate join conditions
and remote qualifiers in addition to local qualifiers in the local, for
component tuples that were re-fetched from the remote (and remaining
component tuples that were copied from whole-row vars, if any), in cases
where the re-fetched tuples were updated versions of those tuples rather
than the same versions priviously obtained.

> One other standpoint tries to handle a bunch of base foreign
> tables as a unit. That means, if any of base foreign table is
> the target of locking, it prompts FDW driver to fetch the latest
> "joined" tuple identified by "ctid", even if this join contains
> multiple base relations to be locked.
> The advantage of this approach is that we can use qualifiers of
> the ForeignScan node with scanrelid==0 and no need to pay attention
> of remote qualifier and/or join condition individually.
> Its disadvantage is, we may extend EState structure to keep the
> "joined" tuples, in addition to es_epqTupleSet[].

That is an idea. However, ISTM there is another disadvantage; that is
not efficient because that would need to perform another remote join
query having such additional conditions during an EvalPlanQual check, as
you proposed.

> I'm inclined to think the later standpoint works well, because
> it does not need to reproduce an alternative execution path in
> local side again, even if a ForeignScan node represents much
> complicated remote query.
> If we would fetch tuples of individual base relations, we need
> to reconstruct identical join path to be executed on remote-
> side, don't it?

Yeah, that was my image for fixing this issue.

> IIUC, the purpose of EvalPlanQual() is to ensure the tuples to
> be locked is still visible, so it is not an essential condition
> to fetch base tuples individually.

I think so too, but taking the similarity and/or efficiency of
processing into consideration, I would vote for the idea of having an
alternative execution path in the local. That would also allow FDW
authors to write the foreign join pushdown functionality in their FDWs
by smaller efforts.

Best regards,
Etsuro Fujita

[1]
http://www.postgresql.org/message-id/CAEZqfEe9KGy=1_waGh2rgZPg0o4pqgD+iauYaj8wTze+CYJUHg@mail.gmail.com

Hi Fujita-san,

Sorry for my late.

> On 2015/06/27 21:09, Kouhei Kaigai wrote:
> >>> BTW, if you try newer version of postgres_fdw foreign join patch,
> >>> please provide me to reproduce the problem/
>
> >> OK
>
> > Did you forget to attach the patch, or v17 is in use?
>
> Sorry, I made a mistake. The problem was produced using v16 [1].
>
> >>> I'd like to suggest a solution that re-construct remote tuple according
> >>> to the fdw_scan_tlist on ExecScanFetch, if given scanrelid == 0.
> >>> It enables to run local qualifier associated with the ForeignScan node,
> >>> and it will also work for the case when tuple in es_epqTupleSet[] was
> >>> local heap.
>
> >> Maybe I'm missing something, but I don't think your proposal works
> >> properly because we don't have any component ForeignScan state node or
> >> subsidiary join state node once we've replaced the entire join with the
> >> ForeignScan performing the join remotely, IIUC. So, my image was to
> >> have another subplan for EvalPlanQual as well as the ForeignScan, to do
> >> the entire join locally for the component test tuples if we are inside
> >> an EvalPlanQual recheck.
>
> > Hmm... Probably, we have two standpoints to tackle the problem.
> >
> > The first standpoint tries to handle the base foreign table as
> > a prime relation for locking. Thus, we have to provide a way to
> > fetch a remote tuple identified with the supplied ctid.
> > The advantage of this approach is the way to fetch tuples from
> > base relation is quite similar to the existing form, however,
> > its disadvantage is another side of the same coin, because the
> > ForeignScan node with scanrelid==0 (that represents remote join
> > query) may have local qualifiers which shall run on the tuple
> > according to fdw_scan_tlist.
>
> IIUC, I think this approach would also need to evaluate join conditions
> and remote qualifiers in addition to local qualifiers in the local, for
> component tuples that were re-fetched from the remote (and remaining
> component tuples that were copied from whole-row vars, if any), in cases
> where the re-fetched tuples were updated versions of those tuples rather
> than the same versions priviously obtained.
>
> > One other standpoint tries to handle a bunch of base foreign
> > tables as a unit. That means, if any of base foreign table is
> > the target of locking, it prompts FDW driver to fetch the latest
> > "joined" tuple identified by "ctid", even if this join contains
> > multiple base relations to be locked.
> > The advantage of this approach is that we can use qualifiers of
> > the ForeignScan node with scanrelid==0 and no need to pay attention
> > of remote qualifier and/or join condition individually.
> > Its disadvantage is, we may extend EState structure to keep the
> > "joined" tuples, in addition to es_epqTupleSet[].
>
> That is an idea. However, ISTM there is another disadvantage; that is
> not efficient because that would need to perform another remote join
> query having such additional conditions during an EvalPlanQual check, as
> you proposed.
>
> > I'm inclined to think the later standpoint works well, because
> > it does not need to reproduce an alternative execution path in
> > local side again, even if a ForeignScan node represents much
> > complicated remote query.
> > If we would fetch tuples of individual base relations, we need
> > to reconstruct identical join path to be executed on remote-
> > side, don't it?
>
> Yeah, that was my image for fixing this issue.
>
> > IIUC, the purpose of EvalPlanQual() is to ensure the tuples to
> > be locked is still visible, so it is not an essential condition
> > to fetch base tuples individually.
>
> I think so too, but taking the similarity and/or efficiency of
> processing into consideration, I would vote for the idea of having an
> alternative execution path in the local. That would also allow FDW
> authors to write the foreign join pushdown functionality in their FDWs
> by smaller efforts.
>
Even though I'd like to see committer's opinion, I could not come up
with the idea better than what you proposed; foreign-/custom-scan
has alternative plan if scanrelid==0.

Let me introduce a few cases we should pay attention.

Foreign/CustomScan node may stack; that means a Foreign/CustomScan node
may have child node that includes another Foreign/CustomScan node with
scanrelid==0.
(At this moment, ForeignScan cannot have child node, however, more
aggressive push-down [1] will need same feature to fetch tuples from
local relation and construct VALUES() clause.)
In this case, the highest Foreign/CustomScan node (that is also nearest
to LockRows or ModifyTuples) run the alternative sub-plan that includes
scan/join plans dominated by fdw_relids or custom_relids.

For example:
LockRows
-> HashJoin
-> CustomScan (AliceJoin)
-> SeqScan on t1
-> CustomScan (CarolJoin)
-> SeqScan on t2
-> SeqScan on t3
-> Hash
-> CustomScan (BobJoin)
-> SeqScan on t4
-> ForeignScan (remote join involves ft5, ft6)

In this case, AliceJoin will have alternative sub-plan to join t1, t2
and t3, then it shall be used on EvalPlanQual(). Also, BobJoin will
have alternative sub-plan to join t4, ft5 and ft6. CarolJoin and the
ForeignScan will also have alternative sub-plan, however, these are
not used in this case.
Probably, it works fine.

Do we have potential scenario if foreign-/custom-join is located over
LockRows node. (Subquery expansion may give such a case?)
Anyway, doesn't it make a problem, does it?

On the next step, how do we implement this design?
I guess that planner needs to keep a path that contains neither
foreign-join nor custom-join with scanrelid==0.
Probably, "cheapest_builtin_path" of RelOptInfo is needed that
never contains these remote/custom join logic, as a seed of
alternative sub-plan.

create_foreignscan_plan() or create_customscan_plan() will be
able to construct these alternative plan, regardless of the
extensions. So, individual FDW/CSP don't need to care about
this alternative sub-plan, do it?

After that, once ExecScanFetch() is called under EvalPlanQual(),
these Foreign/CustomScan with scanrelid==0 runs the alternative
sub-plan, to validate the latest tuple.

Hmm... It looks to me a workable approach.

Fujita-san, are you available to make a patch with this approach?
If so, I'd like to volunteer its reviewing.

[1] http://www.postgresql.org/message-id/9A28C8860F777E439AA12E8AEA7694F8010F20AD@BPXM15GP.gisp.nec.co.jp

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Hi KaiGai-san,

On 2015/07/02 18:31, Kouhei Kaigai wrote:
> Even though I'd like to see committer's opinion, I could not come up
> with the idea better than what you proposed; foreign-/custom-scan
> has alternative plan if scanrelid==0.

I'd also like to hear the opinion!

> Let me introduce a few cases we should pay attention.
>
> Foreign/CustomScan node may stack; that means a Foreign/CustomScan node
> may have child node that includes another Foreign/CustomScan node with
> scanrelid==0.
> (At this moment, ForeignScan cannot have child node, however, more
> aggressive push-down [1] will need same feature to fetch tuples from
> local relation and construct VALUES() clause.)
> In this case, the highest Foreign/CustomScan node (that is also nearest
> to LockRows or ModifyTuples) run the alternative sub-plan that includes
> scan/join plans dominated by fdw_relids or custom_relids.
>
> For example:
> LockRows
> -> HashJoin
> -> CustomScan (AliceJoin)
> -> SeqScan on t1
> -> CustomScan (CarolJoin)
> -> SeqScan on t2
> -> SeqScan on t3
> -> Hash
> -> CustomScan (BobJoin)
> -> SeqScan on t4
> -> ForeignScan (remote join involves ft5, ft6)
>
> In this case, AliceJoin will have alternative sub-plan to join t1, t2
> and t3, then it shall be used on EvalPlanQual(). Also, BobJoin will
> have alternative sub-plan to join t4, ft5 and ft6. CarolJoin and the
> ForeignScan will also have alternative sub-plan, however, these are
> not used in this case.
> Probably, it works fine.

Yeah, I think so too.

> Do we have potential scenario if foreign-/custom-join is located over
> LockRows node. (Subquery expansion may give such a case?)
> Anyway, doesn't it make a problem, does it?

IIUC, I don't think we have such a case.

> On the next step, how do we implement this design?
> I guess that planner needs to keep a path that contains neither
> foreign-join nor custom-join with scanrelid==0.
> Probably, "cheapest_builtin_path" of RelOptInfo is needed that
> never contains these remote/custom join logic, as a seed of
> alternative sub-plan.

Yeah, I think so too, but I've not fugiured out how to implement this yet.

To be honest, ISTM that it's difficult to do that simply and efficiently
for the foreign/custom-join-pushdown API that we have for 9.5. It's a
little late, but what I started thinking is to redesign that API so that
that API is called at standard_join_search, as discussed in [2]; (1) to
place that API call *after* the set_cheapest call and (2) to place
another set_cheapest call after that API call for each joinrel. By the
first set_cheapest call, I think we could probably save an alternative
path that we need in "cheapest_builtin_path". By the second
set_cheapest call following that API call, we could consider
foreign/custom-join-pushdown paths also. What do you think about this idea?

> create_foreignscan_plan() or create_customscan_plan() will be
> able to construct these alternative plan, regardless of the
> extensions. So, individual FDW/CSP don't need to care about
> this alternative sub-plan, do it?
>
> After that, once ExecScanFetch() is called under EvalPlanQual(),
> these Foreign/CustomScan with scanrelid==0 runs the alternative
> sub-plan, to validate the latest tuple.
>
> Hmm... It looks to me a workable approach.

Year, I think so too.

> Fujita-san, are you available to make a patch with this approach?
> If so, I'd like to volunteer its reviewing.

Yeah, I'm willing to make a patch if we obtain the consensus! And I'd
be happy if you help me doing the work!

Best regards,
Etsuro Fujita

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

> > Let me introduce a few cases we should pay attention.
> >
> > Foreign/CustomScan node may stack; that means a Foreign/CustomScan node
> > may have child node that includes another Foreign/CustomScan node with
> > scanrelid==0.
> > (At this moment, ForeignScan cannot have child node, however, more
> > aggressive push-down [1] will need same feature to fetch tuples from
> > local relation and construct VALUES() clause.)
> > In this case, the highest Foreign/CustomScan node (that is also nearest
> > to LockRows or ModifyTuples) run the alternative sub-plan that includes
> > scan/join plans dominated by fdw_relids or custom_relids.
> >
> > For example:
> > LockRows
> > -> HashJoin
> > -> CustomScan (AliceJoin)
> > -> SeqScan on t1
> > -> CustomScan (CarolJoin)
> > -> SeqScan on t2
> > -> SeqScan on t3
> > -> Hash
> > -> CustomScan (BobJoin)
> > -> SeqScan on t4
> > -> ForeignScan (remote join involves ft5, ft6)
> >
> > In this case, AliceJoin will have alternative sub-plan to join t1, t2
> > and t3, then it shall be used on EvalPlanQual(). Also, BobJoin will
> > have alternative sub-plan to join t4, ft5 and ft6. CarolJoin and the
> > ForeignScan will also have alternative sub-plan, however, these are
> > not used in this case.
> > Probably, it works fine.
>
> Yeah, I think so too.
>
Sorry, I need to adjust my explanation above a bit:

In this case, AliceJoin will have alternative sub-plan to join t1 and
CarolJoin, then CarolJoin will have alternative sub-plan to join t2 and
t3. Also, BobJoin will have alternative sub-plan to join t4 and the
ForeignScan with remote join, and this ForeignScan node will have
alternative sub-plan to join ft5 and ft6.

Why this recursive design is better? Because it makes planner enhancement
much simple than overall approach. Please see my explanation in the
section below.

> > On the next step, how do we implement this design?
> > I guess that planner needs to keep a path that contains neither
> > foreign-join nor custom-join with scanrelid==0.
> > Probably, "cheapest_builtin_path" of RelOptInfo is needed that
> > never contains these remote/custom join logic, as a seed of
> > alternative sub-plan.
>
> Yeah, I think so too, but I've not fugiured out how to implement this yet.
>
> To be honest, ISTM that it's difficult to do that simply and efficiently
> for the foreign/custom-join-pushdown API that we have for 9.5. It's a
> little late, but what I started thinking is to redesign that API so that
> that API is called at standard_join_search, as discussed in [2]; (1) to
> place that API call *after* the set_cheapest call and (2) to place
> another set_cheapest call after that API call for each joinrel. By the
> first set_cheapest call, I think we could probably save an alternative
> path that we need in "cheapest_builtin_path". By the second
> set_cheapest call following that API call, we could consider
> foreign/custom-join-pushdown paths also. What do you think about this idea?
>
Disadvantage is larger than advantage, sorry.
The reason why we put foreign/custom-join hook on add_paths_to_joinrel()
is that the source relations (inner/outer) were not obvious, thus,
we cannot reproduce which relations are the source of this join.
So, I had to throw a spoon when I tried this approach before.

My idea is that we save the cheapest_total_path of RelOptInfo onto the
new cheapest_builtin_path just before the GetForeignJoinPaths() hook.
Why? It should be a built-in join logic, never be a foreign/custom-join,
because of the hook location; only built-in logic shall be added here.
Even if either/both of join sub-trees contains foreign/custom-join,
these path have own alternative sub-plan at their level, no need to
care about at current level. (It is the reason why I adjust my explanation
above.)
Once this built-in path is kept and foreign/custom-join get chosen by
set_cheapest(), it is easy to attach this sub-plan to ForeignScan or
CustomScan node.
I don't find any significant down-side in this approach.
How about your opinion?

Regarding to the development timeline, I prefer to put something
workaround not to kick Assert() on ExecScanFetch().
We may add a warning in the documentation not to replace built-in
join if either/both of sub-trees are target of UPDATE/DELETE or
FOR SHARE/UPDATE.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/07/02 23:13, Kouhei Kaigai wrote:
>> To be honest, ISTM that it's difficult to do that simply and efficiently
>> for the foreign/custom-join-pushdown API that we have for 9.5. It's a
>> little late, but what I started thinking is to redesign that API so that
>> that API is called at standard_join_search, as discussed in [2]; (1) to
>> place that API call *after* the set_cheapest call and (2) to place
>> another set_cheapest call after that API call for each joinrel. By the
>> first set_cheapest call, I think we could probably save an alternative
>> path that we need in "cheapest_builtin_path". By the second
>> set_cheapest call following that API call, we could consider
>> foreign/custom-join-pushdown paths also. What do you think about this idea?

> Disadvantage is larger than advantage, sorry.
> The reason why we put foreign/custom-join hook on add_paths_to_joinrel()
> is that the source relations (inner/outer) were not obvious, thus,
> we cannot reproduce which relations are the source of this join.
> So, I had to throw a spoon when I tried this approach before.

Maybe I'm missing something, but my image about this approach is that if
base relations for a given joinrel are all foreign tables and belong to
the same foreign server, then by calling that API there, we consider the
remote join over all the foreign tables, and that if not, we give up to
consider the remote join.

> My idea is that we save the cheapest_total_path of RelOptInfo onto the
> new cheapest_builtin_path just before the GetForeignJoinPaths() hook.
> Why? It should be a built-in join logic, never be a foreign/custom-join,
> because of the hook location; only built-in logic shall be added here.

My concern about your idea is that since that (a) add_paths_to_joinrel
is called multiple times per joinrel and that (b) repetitive add_path
calls through GetForeignJoinPaths in add_paths_to_joinrel might remove
old paths that are builtin, it's possible to save a path that is not
builtin onto the cheapest_total_path and thus to save that path wrongly
onto the cheapest_builtin_path. There might be a good way to cope with
that, though.

> Regarding to the development timeline, I prefer to put something
> workaround not to kick Assert() on ExecScanFetch().
> We may add a warning in the documentation not to replace built-in
> join if either/both of sub-trees are target of UPDATE/DELETE or
> FOR SHARE/UPDATE.

I'm not sure that that is a good idea, but anyway, I think we need to
hurry fixing this issue.

Best regards,
Etsuro Fujita

> On 2015/07/02 23:13, Kouhei Kaigai wrote:
> >> To be honest, ISTM that it's difficult to do that simply and efficiently
> >> for the foreign/custom-join-pushdown API that we have for 9.5. It's a
> >> little late, but what I started thinking is to redesign that API so that
> >> that API is called at standard_join_search, as discussed in [2]; (1) to
> >> place that API call *after* the set_cheapest call and (2) to place
> >> another set_cheapest call after that API call for each joinrel. By the
> >> first set_cheapest call, I think we could probably save an alternative
> >> path that we need in "cheapest_builtin_path". By the second
> >> set_cheapest call following that API call, we could consider
> >> foreign/custom-join-pushdown paths also. What do you think about this idea?
>
> > Disadvantage is larger than advantage, sorry.
> > The reason why we put foreign/custom-join hook on add_paths_to_joinrel()
> > is that the source relations (inner/outer) were not obvious, thus,
> > we cannot reproduce which relations are the source of this join.
> > So, I had to throw a spoon when I tried this approach before.
>
> Maybe I'm missing something, but my image about this approach is that if
> base relations for a given joinrel are all foreign tables and belong to
> the same foreign server, then by calling that API there, we consider the
> remote join over all the foreign tables, and that if not, we give up to
> consider the remote join.
>
Your understanding is correct, but missing a point. Once foreign tables
to be joined are informed as a bitmap (joinrel->relids), it is not obvious
for extensions which relations are joined with INNER JOIN, and which ones
are joined with OUTER JOIN.
I tried to implement a common utility function under the v9.5 cycle,
however, it was suspicious whether we can make a reliable logic.

Also, I don't want to stick on the assumption that relations involved in
remote join are all managed by same foreign-server no longer.
The following two ideas introduce possible enhancement of remote join
feature that involved local relations; replicated table or transformed
to VALUES() clause.

http://www.postgresql.org/message-id/CA+Tgmoai_VUF5h6qVLNLU+FKp0aeBCbnnMT3SCvL-HvOpBR=Xw@mail.gmail.com
http://www.postgresql.org/message-id/9A28C8860F777E439AA12E8AEA7694F8010F20AD@BPXM15GP.gisp.nec.co.jp

Once we have to pay attention to the case of local/foreign relations
mixed, we have to care about the path of underlying local or foreign
relations managed by other foreign server.

I think add_paths_to_joinrel() is the best location for foreign-join,
not only custom-join. Relocation to standard_join_search() has larger
disadvantage than its advantage.

> > My idea is that we save the cheapest_total_path of RelOptInfo onto the
> > new cheapest_builtin_path just before the GetForeignJoinPaths() hook.
> > Why? It should be a built-in join logic, never be a foreign/custom-join,
> > because of the hook location; only built-in logic shall be added here.
>
> My concern about your idea is that since that (a) add_paths_to_joinrel
> is called multiple times per joinrel and that (b) repetitive add_path
> calls through GetForeignJoinPaths in add_paths_to_joinrel might remove
> old paths that are builtin, it's possible to save a path that is not
> builtin onto the cheapest_total_path and thus to save that path wrongly
> onto the cheapest_builtin_path. There might be a good way to cope with
> that, though.
>
For the concern (a), FDW driver can reference RelOptInfo->fdw_private
that shall be initialized to NULL, then FDW driver will set valid data
if it preliminary adds something. IIRC, postgres_fdw also skips to
add same path multiple times.

For the concern (b), yep, we may enhance add_path() to retain built-in
path, instead of the add_paths_to_joinrel().
Let's adjust the logic a bit. The add_path() can know whether the given
path is usual or exceptional (ForeignPath/CustomPath towards none base
relation) one. If path is exceptional, the cheapest_builtin_path shall
be retained unconditionally. Elsewhere, the cheapest one replace here,
then the cheapest built-in path will survive.

Is it still problematic?

> > Regarding to the development timeline, I prefer to put something
> > workaround not to kick Assert() on ExecScanFetch().
> > We may add a warning in the documentation not to replace built-in
> > join if either/both of sub-trees are target of UPDATE/DELETE or
> > FOR SHARE/UPDATE.
>
> I'm not sure that that is a good idea, but anyway, I think we need to
> hurry fixing this issue.
>
My approach is not fix, but avoid. :-)

It may be an idea to implement the above fixup even though it may be
too large/late to apply v9.5 features, but we can understand how many
changes are needed to fixup this problem.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/07/03 15:32, Kouhei Kaigai wrote:
>> On 2015/07/02 23:13, Kouhei Kaigai wrote:
>>>> To be honest, ISTM that it's difficult to do that simply and efficiently
>>>> for the foreign/custom-join-pushdown API that we have for 9.5. It's a
>>>> little late, but what I started thinking is to redesign that API so that
>>>> that API is called at standard_join_search, as discussed in [2];

>>> Disadvantage is larger than advantage, sorry.
>>> The reason why we put foreign/custom-join hook on add_paths_to_joinrel()
>>> is that the source relations (inner/outer) were not obvious, thus,
>>> we cannot reproduce which relations are the source of this join.
>>> So, I had to throw a spoon when I tried this approach before.

>> Maybe I'm missing something, but my image about this approach is that if
>> base relations for a given joinrel are all foreign tables and belong to
>> the same foreign server, then by calling that API there, we consider the
>> remote join over all the foreign tables, and that if not, we give up to
>> consider the remote join.

> Your understanding is correct, but missing a point. Once foreign tables
> to be joined are informed as a bitmap (joinrel->relids), it is not obvious
> for extensions which relations are joined with INNER JOIN, and which ones
> are joined with OUTER JOIN.

Can't FDWs get the join information through the root, which I think we
would pass to the API as the argument?

> Also, I don't want to stick on the assumption that relations involved in
> remote join are all managed by same foreign-server no longer.
> The following two ideas introduce possible enhancement of remote join
> feature that involved local relations; replicated table or transformed
> to VALUES() clause.
>
> http://www.postgresql.org/message-id/CA+Tgmoai_VUF5h6qVLNLU+FKp0aeBCbnnMT3SCvL-HvOpBR=Xw@mail.gmail.com
> http://www.postgresql.org/message-id/9A28C8860F777E439AA12E8AEA7694F8010F20AD@BPXM15GP.gisp.nec.co.jp

Interesting!

> I think add_paths_to_joinrel() is the best location for foreign-join,
> not only custom-join. Relocation to standard_join_search() has larger
> disadvantage than its advantage.

I agree with you that it's important to ensure the expandability, and my
question is, is it possible that the API call from standard_join_search
also realize those idea if FDWs can get the join information through the
root or something like that?

Best regards,
Etsuro Fujita

> > Also, I don't want to stick on the assumption that relations involved in
> > remote join are all managed by same foreign-server no longer.
> > The following two ideas introduce possible enhancement of remote join
> > feature that involved local relations; replicated table or transformed
> > to VALUES() clause.
> >
> >
> http://www.postgresql.org/message-id/CA+Tgmoai_VUF5h6qVLNLU+FKp0aeBCbnnMT3SC
> vL-HvOpBR=Xw(at)mail(dot)gmail(dot)com
> >
> http://www.postgresql.org/message-id/9A28C8860F777E439AA12E8AEA7694F8010F20A
> D(at)BPXM15GP(dot)gisp(dot)nec(dot)co(dot)jp
>
> Interesting!
>
> > I think add_paths_to_joinrel() is the best location for foreign-join,
> > not only custom-join. Relocation to standard_join_search() has larger
> > disadvantage than its advantage.
>
> I agree with you that it's important to ensure the expandability, and my
> question is, is it possible that the API call from standard_join_search
> also realize those idea if FDWs can get the join information through the
> root or something like that?
>
I don't deny its possibility, even though I once gave up to implement to
reproduce join information - which relations and other ones are joined in
this level - using PlannerInfo and RelOptInfo.
However, we need to pay attention on advantages towards the alternatives.
Hooks on add_paths_to_joinrel() enables to implement identical stuff, with
less complicated logic to reproduce left / right relations from RelOptInfo
of the joinrel. (Note that RelOptInfo->fdw_private enables to avoid path-
construction multiple times.)
I'm uncertain why this API change is necessary to fix up the problem
around EvalPlanQual.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/07/06 9:42, Kouhei Kaigai wrote:
>>> Also, I don't want to stick on the assumption that relations involved in
>>> remote join are all managed by same foreign-server no longer.
>>> The following two ideas introduce possible enhancement of remote join
>>> feature that involved local relations; replicated table or transformed
>>> to VALUES() clause.

>>> I think add_paths_to_joinrel() is the best location for foreign-join,
>>> not only custom-join. Relocation to standard_join_search() has larger
>>> disadvantage than its advantage.

>> I agree with you that it's important to ensure the expandability, and my
>> question is, is it possible that the API call from standard_join_search
>> also realize those idea if FDWs can get the join information through the
>> root or something like that?

> I don't deny its possibility, even though I once gave up to implement to
> reproduce join information - which relations and other ones are joined in
> this level - using PlannerInfo and RelOptInfo.

OK

> However, we need to pay attention on advantages towards the alternatives.
> Hooks on add_paths_to_joinrel() enables to implement identical stuff, with
> less complicated logic to reproduce left / right relations from RelOptInfo
> of the joinrel. (Note that RelOptInfo->fdw_private enables to avoid path-
> construction multiple times.)
> I'm uncertain why this API change is necessary to fix up the problem
> around EvalPlanQual.

Yeah, maybe we wouldn't need any API change. I think we would be able
to fix this by complicating add_path as you pointed out upthread. I'm
not sure that complicating it is a good idea, though. I think that it
might be possible that the callback in standard_join_search would allow
us to fix this without complicating the core path-cost-comparison stuff
such as add_path. I noticed that what I proposed upthread doesn't work
properly, though.

Actually, I have another concern about the callback location that you
proposed; that might meaninglessly increase planning time in the
postgres_fdw case when using remote estimates, which the proposed
postgres_fdw patch doesn't support currently IIUC, but I think it should
support that. Let me explain about that. If you have A JOIN B JOIN C
all on the same foreign server, for example, we'll have only to perform
a remote EXPLAIN for A-B-C for the estimates (when adopting a strategy
that we push down a join as large as possible into the remote server).
However, ISTM that the callback in add_paths_to_joinrel would perform
remote EXPLAINs not only for A-B-C but for A-B, A-C and B-C according to
the dynamic programming algorithm. (Duplicated remote EXPLAINs for
A-B-C can be eliminated using a way you proposed.) Thus the remote
EXPLAINs for A-B, A-C and B-C seem to me meaningless while incurring
performance degradation in query planning. Maybe I'm missing something,
though.

Best regards,
Etsuro Fujita

On 2015/07/07 19:15, Etsuro Fujita wrote:
> On 2015/07/06 9:42, Kouhei Kaigai wrote:
>> However, we need to pay attention on advantages towards the alternatives.
>> Hooks on add_paths_to_joinrel() enables to implement identical stuff,
>> with
>> less complicated logic to reproduce left / right relations from
>> RelOptInfo
>> of the joinrel. (Note that RelOptInfo->fdw_private enables to avoid path-
>> construction multiple times.)
>> I'm uncertain why this API change is necessary to fix up the problem
>> around EvalPlanQual.
>
> Yeah, maybe we wouldn't need any API change. I think we would be able
> to fix this by complicating add_path as you pointed out upthread. I'm
> not sure that complicating it is a good idea, though. I think that it
> might be possible that the callback in standard_join_search would allow
> us to fix this without complicating the core path-cost-comparison stuff
> such as add_path. I noticed that what I proposed upthread doesn't work
> properly, though.

To resolve this issue, I tried to make the core create an alternative
plan that will be used in an EvalPlanQual recheck, instead of a foreign
scan that performs a foreign join remotely (ie, scanrelid = 0). But I
changed that idea. Instead, I'd like to propose that it's the FDW's
responsibility to provide such a plan. Specifically, I'd propose that
(1) we add a new Path field, say subpath, to the ForeignPath data
structure and that (2) when generating a ForeignPath node for a foreign
join, an FDW must provide the subpath Path node by itself. As before,
it'd be recommended to use

ForeignPath *
create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel,
double rows, Cost startup_cost, Cost total_cost,
List *pathkeys,
Relids required_outer,
Path *subpath,
List *fdw_private)

where subpath is the subpath Path node that has the pathkeys and the
required_outer rels. (subpath is NULL if scanning a base relation.)
Also, it'd be recommended that an FDW generates such ForeignPath nodes
by considering, for each of paths in the rel's pathlist, whether to push
down that path (to generate a ForeignPath node for a foreign join that
has the same pathkeys and parameterization as that path). So, if
generating the ForeignPath node, that path could be used as the subpath
Path node.

(I think the current postgres_fdw patch only considers an unsorted,
unparameterized path for performing a foreign join remotely, but I think
we should also consider presorted and/or parameterized paths.)

I think this idea would apply to the API location that you proposed.
However, ISTM that this idea would work better for the API call from
standard_join_search because the rel's pathlist at that point has more
paths worthy of consideration, in view of not only costs and sizes but
pathkeys and parameterization.

I think the subplan created from the subpath Path node could be used in
an EvalPlanQual recheck, instead of a foreign scan that performs a
foreign join remotely, as discussed previously.

Comments welcome!

Best regards,
Etsuro Fujita

On Fri, Jul 3, 2015 at 6:25 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Can't FDWs get the join information through the root, which I think we would
> pass to the API as the argument?

This is exactly what Tom suggested originally, and it has some appeal,
but neither KaiGai nor I could see how to make it work . Do you have
an idea? It's not too late to go back and change the API.

The problem that was bothering us (or at least what was bothering me)
is that the PlannerInfo provides only a list of SpecialJoinInfo
structures, which don't directly give you the original join order. In
fact, min_righthand and min_lefthand are intended to constraint the
*possible* join orders, and are deliberately designed *not* to specify
a single join order. If you're sending a query to a remote PostgreSQL
node, you don't want to know what all the possible join orders are;
it's the remote side's job to plan the query. You do, however, need
an easy way to identify one join order that you can use to construct a
query. It didn't seem easy to do that without duplicating
make_join_rel(), which seemed like a bad idea.

But maybe there's a good way to do it. Tom wasn't crazy about this
hook both because of the frequency of calls and also because of the
long argument list. I think those concerns are legitimate; I just
couldn't see how to make the other way work.

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

> On Fri, Jul 3, 2015 at 6:25 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > Can't FDWs get the join information through the root, which I think we would
> > pass to the API as the argument?
>
> This is exactly what Tom suggested originally, and it has some appeal,
> but neither KaiGai nor I could see how to make it work . Do you have
> an idea? It's not too late to go back and change the API.
>
> The problem that was bothering us (or at least what was bothering me)
> is that the PlannerInfo provides only a list of SpecialJoinInfo
> structures, which don't directly give you the original join order. In
> fact, min_righthand and min_lefthand are intended to constraint the
> *possible* join orders, and are deliberately designed *not* to specify
> a single join order. If you're sending a query to a remote PostgreSQL
> node, you don't want to know what all the possible join orders are;
> it's the remote side's job to plan the query. You do, however, need
> an easy way to identify one join order that you can use to construct a
> query. It didn't seem easy to do that without duplicating
> make_join_rel(), which seemed like a bad idea.
>
> But maybe there's a good way to do it. Tom wasn't crazy about this
> hook both because of the frequency of calls and also because of the
> long argument list. I think those concerns are legitimate; I just
> couldn't see how to make the other way work.
>
I could have a discussion with Fujita-san about this topic.
He has a little bit tricky, but I didn't have a clear reason to deny,
idea to tackle this matter.
At the line just above set_cheapest() of the standard_join_search(),
at least one built-in join logic are already added to the RelOptInfo,
thus, FDW driver can reference the cheapest path by built-in logic
and its lefttree and righttree that construct a joinrel.
Its assumption is, the best paths by built-in logic are at least
enough reasonable join order than other potential ones.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> The problem that was bothering us (or at least what was bothering me)
> is that the PlannerInfo provides only a list of SpecialJoinInfo
> structures, which don't directly give you the original join order. In
> fact, min_righthand and min_lefthand are intended to constraint the
> *possible* join orders, and are deliberately designed *not* to specify
> a single join order. If you're sending a query to a remote PostgreSQL
> node, you don't want to know what all the possible join orders are;
> it's the remote side's job to plan the query. You do, however, need
> an easy way to identify one join order that you can use to construct a
> query. It didn't seem easy to do that without duplicating
> make_join_rel(), which seemed like a bad idea.

In principle it seems like you could traverse root->parse->jointree
as a guide to reconstructing the original syntactic structure; though
I'm not sure how hard it would be to ignore the parts of that tree
that correspond to relations you're not shipping.

> But maybe there's a good way to do it. Tom wasn't crazy about this
> hook both because of the frequency of calls and also because of the
> long argument list. I think those concerns are legitimate; I just
> couldn't see how to make the other way work.

In my vision you probably really only want one call per build_join_rel
event (that is, per construction of a new RelOptInfo), not per
make_join_rel event.

It's possible that an FDW that wants to handle joins but is not talking to
a remote query planner would need to grovel through all the join ordering
possibilities individually, and then maybe hooking at make_join_rel is
sensible rather than having to reinvent that logic. But I'd want to see a
concrete use-case first, and I certainly don't think that that's the main
case to design the API around.

regards, tom lane

> I could have a discussion with Fujita-san about this topic.
>
Also, let me share with the discussion towards entire solution.

The primitive reason of this problem is, Scan node with scanrelid==0
represents a relation join that can involve multiple relations, thus,
its TupleDesc of the records will not fit base relations, however,
ExecScanFetch() was not updated when scanrelid==0 gets supported.

FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
to generate records according to the fdw_/custom_scan_tlist that
reflects the definition of relation join, and only FDW/CSP know how
to combine these base relations.
In addition, host-side expressions (like Plan->qual) are initialized
to reference the records generated by FDW/CSP, so the least invasive
approach is to allow FDW/CSP to have own logic to recheck, I think.

Below is the structure of ExecScanFetch().

ExecScanFetch(ScanState *node,
ExecScanAccessMtd accessMtd,
ExecScanRecheckMtd recheckMtd)
{
EState *estate = node->ps.state;

if (estate->es_epqTuple != NULL)
{
/*
* We are inside an EvalPlanQual recheck. Return the test tuple if
* one is available, after rechecking any access-method-specific
* conditions.
*/
Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;

Assert(scanrelid > 0);
if (estate->es_epqTupleSet[scanrelid - 1])
{
TupleTableSlot *slot = node->ss_ScanTupleSlot;
:
return slot;
}
}
return (*accessMtd) (node);
}

When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
then ExecScan() applies its qualifiers by ExecQual().
So, as long as FDW/CSP can return a record that satisfies the TupleDesc
of this relation, made by the tuples in es_epqTuple[] array, rest of the
code paths are common.

I have an idea to solve the problem.
It adds recheckMtd() call if scanrelid==0 just before the assertion above,
and add a callback of FDW on ForeignRecheck().
The role of this new callback is to set up the supplied TupleTableSlot
and check its visibility, but does not define how to do this.
It is arbitrarily by FDW driver, like invocation of alternative plan
consists of only built-in logic.

Invocation of alternative plan is one of the most feasible way to
implement EPQ logic on FDW, so I think FDW also needs a mechanism
that takes child path-nodes like custom_paths in CustomPath node.
Once a valid path node is linked to this list, createplan.c transform
them to relevant plan node, then FDW can initialize and invoke this
plan node during execution, like ForeignRecheck().

This design can solve another problem Fujita-san has also mentioned.
If scan qualifier is pushed-down to the remote query and its expression
node is saved in the private area of ForeignScan, the callback on
ForeignRecheck() can evaluate the qualifier by itself. (Note that only
FDW driver can know where and how expression node being pushed-down
is saved in the private area.)

In the summary, the following three enhancements are a straightforward
way to fix up the problem he reported.
1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
2. Add a callback of FDW in ForeignRecheck() - to construct a record
according to the fdw_scan_tlist definition and to evaluate its
visibility, or to evaluate qualifier pushed-down if base relation.
3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
to construct plan nodes for EPQ evaluation.

On the other hands, we also need to pay attention the development
timeline. It is a really problem of v9.5, however, it looks to me
the straight forward solution needs enhancement of FDW APIs.

I'd like to see people's comment.
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Kouhei Kaigai
> Sent: Saturday, August 01, 2015 10:35 PM
> To: Robert Haas; Etsuro Fujita
> Cc: PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> > On Fri, Jul 3, 2015 at 6:25 AM, Etsuro Fujita
> > <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > > Can't FDWs get the join information through the root, which I think we would
> > > pass to the API as the argument?
> >
> > This is exactly what Tom suggested originally, and it has some appeal,
> > but neither KaiGai nor I could see how to make it work . Do you have
> > an idea? It's not too late to go back and change the API.
> >
> > The problem that was bothering us (or at least what was bothering me)
> > is that the PlannerInfo provides only a list of SpecialJoinInfo
> > structures, which don't directly give you the original join order. In
> > fact, min_righthand and min_lefthand are intended to constraint the
> > *possible* join orders, and are deliberately designed *not* to specify
> > a single join order. If you're sending a query to a remote PostgreSQL
> > node, you don't want to know what all the possible join orders are;
> > it's the remote side's job to plan the query. You do, however, need
> > an easy way to identify one join order that you can use to construct a
> > query. It didn't seem easy to do that without duplicating
> > make_join_rel(), which seemed like a bad idea.
> >
> > But maybe there's a good way to do it. Tom wasn't crazy about this
> > hook both because of the frequency of calls and also because of the
> > long argument list. I think those concerns are legitimate; I just
> > couldn't see how to make the other way work.
> >
> I could have a discussion with Fujita-san about this topic.
> He has a little bit tricky, but I didn't have a clear reason to deny,
> idea to tackle this matter.
> At the line just above set_cheapest() of the standard_join_search(),
> at least one built-in join logic are already added to the RelOptInfo,
> thus, FDW driver can reference the cheapest path by built-in logic
> and its lefttree and righttree that construct a joinrel.
> Its assumption is, the best paths by built-in logic are at least
> enough reasonable join order than other potential ones.
>
> Thanks,
> --
> NEC Business Creation Division / PG-Strom Project
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Aug 7, 2015 at 3:37 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>> I could have a discussion with Fujita-san about this topic.
>>
> Also, let me share with the discussion towards entire solution.
>
> The primitive reason of this problem is, Scan node with scanrelid==0
> represents a relation join that can involve multiple relations, thus,
> its TupleDesc of the records will not fit base relations, however,
> ExecScanFetch() was not updated when scanrelid==0 gets supported.
>
> FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
> to generate records according to the fdw_/custom_scan_tlist that
> reflects the definition of relation join, and only FDW/CSP know how
> to combine these base relations.
> In addition, host-side expressions (like Plan->qual) are initialized
> to reference the records generated by FDW/CSP, so the least invasive
> approach is to allow FDW/CSP to have own logic to recheck, I think.
>
> Below is the structure of ExecScanFetch().
>
> ExecScanFetch(ScanState *node,
> ExecScanAccessMtd accessMtd,
> ExecScanRecheckMtd recheckMtd)
> {
> EState *estate = node->ps.state;
>
> if (estate->es_epqTuple != NULL)
> {
> /*
> * We are inside an EvalPlanQual recheck. Return the test tuple if
> * one is available, after rechecking any access-method-specific
> * conditions.
> */
> Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
>
> Assert(scanrelid > 0);
> if (estate->es_epqTupleSet[scanrelid - 1])
> {
> TupleTableSlot *slot = node->ss_ScanTupleSlot;
> :
> return slot;
> }
> }
> return (*accessMtd) (node);
> }
>
> When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
> checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
> then ExecScan() applies its qualifiers by ExecQual().
> So, as long as FDW/CSP can return a record that satisfies the TupleDesc
> of this relation, made by the tuples in es_epqTuple[] array, rest of the
> code paths are common.
>
> I have an idea to solve the problem.
> It adds recheckMtd() call if scanrelid==0 just before the assertion above,
> and add a callback of FDW on ForeignRecheck().
> The role of this new callback is to set up the supplied TupleTableSlot
> and check its visibility, but does not define how to do this.
> It is arbitrarily by FDW driver, like invocation of alternative plan
> consists of only built-in logic.
>
> Invocation of alternative plan is one of the most feasible way to
> implement EPQ logic on FDW, so I think FDW also needs a mechanism
> that takes child path-nodes like custom_paths in CustomPath node.
> Once a valid path node is linked to this list, createplan.c transform
> them to relevant plan node, then FDW can initialize and invoke this
> plan node during execution, like ForeignRecheck().
>
> This design can solve another problem Fujita-san has also mentioned.
> If scan qualifier is pushed-down to the remote query and its expression
> node is saved in the private area of ForeignScan, the callback on
> ForeignRecheck() can evaluate the qualifier by itself. (Note that only
> FDW driver can know where and how expression node being pushed-down
> is saved in the private area.)
>
> In the summary, the following three enhancements are a straightforward
> way to fix up the problem he reported.
> 1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
> 2. Add a callback of FDW in ForeignRecheck() - to construct a record
> according to the fdw_scan_tlist definition and to evaluate its
> visibility, or to evaluate qualifier pushed-down if base relation.
> 3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
> to construct plan nodes for EPQ evaluation.
>
> On the other hands, we also need to pay attention the development
> timeline. It is a really problem of v9.5, however, it looks to me
> the straight forward solution needs enhancement of FDW APIs.
>
> I'd like to see people's comment.

I'm not an expert in this area, but this plan does not seem unreasonable to me.

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

On 2015/08/12 7:21, Robert Haas wrote:
> On Fri, Aug 7, 2015 at 3:37 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>>> I could have a discussion with Fujita-san about this topic.
>>>
>> Also, let me share with the discussion towards entire solution.
>>
>> The primitive reason of this problem is, Scan node with scanrelid==0
>> represents a relation join that can involve multiple relations, thus,
>> its TupleDesc of the records will not fit base relations, however,
>> ExecScanFetch() was not updated when scanrelid==0 gets supported.
>>
>> FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
>> to generate records according to the fdw_/custom_scan_tlist that
>> reflects the definition of relation join, and only FDW/CSP know how
>> to combine these base relations.
>> In addition, host-side expressions (like Plan->qual) are initialized
>> to reference the records generated by FDW/CSP, so the least invasive
>> approach is to allow FDW/CSP to have own logic to recheck, I think.
>>
>> Below is the structure of ExecScanFetch().
>>
>> ExecScanFetch(ScanState *node,
>> ExecScanAccessMtd accessMtd,
>> ExecScanRecheckMtd recheckMtd)
>> {
>> EState *estate = node->ps.state;
>>
>> if (estate->es_epqTuple != NULL)
>> {
>> /*
>> * We are inside an EvalPlanQual recheck. Return the test tuple if
>> * one is available, after rechecking any access-method-specific
>> * conditions.
>> */
>> Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
>>
>> Assert(scanrelid > 0);
>> if (estate->es_epqTupleSet[scanrelid - 1])
>> {
>> TupleTableSlot *slot = node->ss_ScanTupleSlot;
>> :
>> return slot;
>> }
>> }
>> return (*accessMtd) (node);
>> }
>>
>> When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
>> checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
>> then ExecScan() applies its qualifiers by ExecQual().
>> So, as long as FDW/CSP can return a record that satisfies the TupleDesc
>> of this relation, made by the tuples in es_epqTuple[] array, rest of the
>> code paths are common.
>>
>> I have an idea to solve the problem.
>> It adds recheckMtd() call if scanrelid==0 just before the assertion above,
>> and add a callback of FDW on ForeignRecheck().
>> The role of this new callback is to set up the supplied TupleTableSlot
>> and check its visibility, but does not define how to do this.
>> It is arbitrarily by FDW driver, like invocation of alternative plan
>> consists of only built-in logic.
>>
>> Invocation of alternative plan is one of the most feasible way to
>> implement EPQ logic on FDW, so I think FDW also needs a mechanism
>> that takes child path-nodes like custom_paths in CustomPath node.
>> Once a valid path node is linked to this list, createplan.c transform
>> them to relevant plan node, then FDW can initialize and invoke this
>> plan node during execution, like ForeignRecheck().
>>
>> This design can solve another problem Fujita-san has also mentioned.
>> If scan qualifier is pushed-down to the remote query and its expression
>> node is saved in the private area of ForeignScan, the callback on
>> ForeignRecheck() can evaluate the qualifier by itself. (Note that only
>> FDW driver can know where and how expression node being pushed-down
>> is saved in the private area.)
>>
>> In the summary, the following three enhancements are a straightforward
>> way to fix up the problem he reported.
>> 1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
>> 2. Add a callback of FDW in ForeignRecheck() - to construct a record
>> according to the fdw_scan_tlist definition and to evaluate its
>> visibility, or to evaluate qualifier pushed-down if base relation.
>> 3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
>> to construct plan nodes for EPQ evaluation.

> I'm not an expert in this area, but this plan does not seem unreasonable to me.

IIRC the discussion with KaiGai-san, I think that that would work. I
think that that would be more suitable for CSPs, though. Correct me if
I'm wrong, KaiGai-san. In either case, I'm not sure that the idea of
transferring both processing to a single callback routine hooked in
ForeignRecheck is a good idea: (a) check to see if the test tuple for
each component foreign table satisfies the remote qual condition and (b)
check to see if those tuples satisfy the remote join condition. I think
that that would be too complicated, probably making the callback routine
bug-prone. So, I'd still propose that *the core* processes (a) and (b)
*separately*.

* As for (a), the core checks the remote qual condition as in [1].

* As for (b), the core executes an alternative subplan locally if inside
an EPQ recheck. The subplan is created as described in [2].

Attached is a WIP patch for that against 9.5
(fdw-eval-plan-qual-0.1.patch), which includes an updated version of the
patch in [1]. I haven't done anything about custom joins yet. Also, I
left the join pushdown API as-is. But I still think that it would be
better that we hook that API in standard_join_search. So, I plan to
modify the patch so in the next version.

For tests, I did a very basic update of the latest postgres_fdw patch in
[3] and attach that (foreign_join_v16_efujita.patch). You can apply the
patches in the following order:

fdw-eval-plan-qual-0.1.patch
usermapping_matching.patch (in [3])
add_GetUserMappingById.patch (in [3])
foreign_join_v16_efujita.patch

(Note that you cannot do tests of [1]. For that, apply
fdw-eval-plan-qual-0.1.patch and the postgres_fdw patch in [1] in this
order.)

Comments welcome!

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/55B204A0.1080507@lab.ntt.co.jp
[2] http://www.postgresql.org/message-id/55B9F95F.5060506@lab.ntt.co.jp
[3]
http://www.postgresql.org/message-id/CAEZqfEe9KGy=1_waGh2rgZPg0o4pqgD+iauYaj8wTze+CYJUHg@mail.gmail.com

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> Sent: Wednesday, August 12, 2015 8:26 PM
> To: Robert Haas; Kaigai Kouhei(海外 浩平)
> Cc: PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> On 2015/08/12 7:21, Robert Haas wrote:
> > On Fri, Aug 7, 2015 at 3:37 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> >>> I could have a discussion with Fujita-san about this topic.
> >>>
> >> Also, let me share with the discussion towards entire solution.
> >>
> >> The primitive reason of this problem is, Scan node with scanrelid==0
> >> represents a relation join that can involve multiple relations, thus,
> >> its TupleDesc of the records will not fit base relations, however,
> >> ExecScanFetch() was not updated when scanrelid==0 gets supported.
> >>
> >> FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
> >> to generate records according to the fdw_/custom_scan_tlist that
> >> reflects the definition of relation join, and only FDW/CSP know how
> >> to combine these base relations.
> >> In addition, host-side expressions (like Plan->qual) are initialized
> >> to reference the records generated by FDW/CSP, so the least invasive
> >> approach is to allow FDW/CSP to have own logic to recheck, I think.
> >>
> >> Below is the structure of ExecScanFetch().
> >>
> >> ExecScanFetch(ScanState *node,
> >> ExecScanAccessMtd accessMtd,
> >> ExecScanRecheckMtd recheckMtd)
> >> {
> >> EState *estate = node->ps.state;
> >>
> >> if (estate->es_epqTuple != NULL)
> >> {
> >> /*
> >> * We are inside an EvalPlanQual recheck. Return the test tuple
> if
> >> * one is available, after rechecking any access-method-specific
> >> * conditions.
> >> */
> >> Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
> >>
> >> Assert(scanrelid > 0);
> >> if (estate->es_epqTupleSet[scanrelid - 1])
> >> {
> >> TupleTableSlot *slot = node->ss_ScanTupleSlot;
> >> :
> >> return slot;
> >> }
> >> }
> >> return (*accessMtd) (node);
> >> }
> >>
> >> When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
> >> checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
> >> then ExecScan() applies its qualifiers by ExecQual().
> >> So, as long as FDW/CSP can return a record that satisfies the TupleDesc
> >> of this relation, made by the tuples in es_epqTuple[] array, rest of the
> >> code paths are common.
> >>
> >> I have an idea to solve the problem.
> >> It adds recheckMtd() call if scanrelid==0 just before the assertion above,
> >> and add a callback of FDW on ForeignRecheck().
> >> The role of this new callback is to set up the supplied TupleTableSlot
> >> and check its visibility, but does not define how to do this.
> >> It is arbitrarily by FDW driver, like invocation of alternative plan
> >> consists of only built-in logic.
> >>
> >> Invocation of alternative plan is one of the most feasible way to
> >> implement EPQ logic on FDW, so I think FDW also needs a mechanism
> >> that takes child path-nodes like custom_paths in CustomPath node.
> >> Once a valid path node is linked to this list, createplan.c transform
> >> them to relevant plan node, then FDW can initialize and invoke this
> >> plan node during execution, like ForeignRecheck().
> >>
> >> This design can solve another problem Fujita-san has also mentioned.
> >> If scan qualifier is pushed-down to the remote query and its expression
> >> node is saved in the private area of ForeignScan, the callback on
> >> ForeignRecheck() can evaluate the qualifier by itself. (Note that only
> >> FDW driver can know where and how expression node being pushed-down
> >> is saved in the private area.)
> >>
> >> In the summary, the following three enhancements are a straightforward
> >> way to fix up the problem he reported.
> >> 1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
> >> 2. Add a callback of FDW in ForeignRecheck() - to construct a record
> >> according to the fdw_scan_tlist definition and to evaluate its
> >> visibility, or to evaluate qualifier pushed-down if base relation.
> >> 3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
> >> to construct plan nodes for EPQ evaluation.
>
> > I'm not an expert in this area, but this plan does not seem unreasonable to
> me.
>
> IIRC the discussion with KaiGai-san, I think that that would work. I
> think that that would be more suitable for CSPs, though. Correct me if
> I'm wrong, KaiGai-san. In either case, I'm not sure that the idea of
> transferring both processing to a single callback routine hooked in
> ForeignRecheck is a good idea: (a) check to see if the test tuple for
> each component foreign table satisfies the remote qual condition and (b)
> check to see if those tuples satisfy the remote join condition. I think
> that that would be too complicated, probably making the callback routine
> bug-prone. So, I'd still propose that *the core* processes (a) and (b)
> *separately*.
>
> * As for (a), the core checks the remote qual condition as in [1].
>
> * As for (b), the core executes an alternative subplan locally if inside
> an EPQ recheck. The subplan is created as described in [2].
>
I don't think it is "too" complicated because (a) visibility check of
the base tuples (saved in es_epqTuple[]) shall be done in the underlying
base foreign-scan node, executed as a part of alternative plan, and
(b) evaluation of remote qual is done with ExecQual() call.

I seems to me your proposition tends to assume a particular design
towards FDW drivers, however, we already have various kind of FDW
drivers not only wrapper of remote RDBMS.
https://wiki.postgresql.org/wiki/Foreign_data_wrappers

Is the [1] and [2] suitable for "all" of them, actually?

Let's assume a FDW module that implements own columnar storage,
has a special JOIN capability if both side are its columnar storage.
Does it need alternative sub-plan for EPQ rechecks? Probably no,
because it has own capability to run JOIN by itself.
It is inconvenience for this FDW if core automatically kicks sub-
plan in spite of its own functionality/capability.

If potential bugs are concerned, a common part shall be cut down
and provided as a utility function. FDW can determine whether it
shall be used, but never enforced.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Fujita-san,

The attached patch enhanced the FDW interface according to the direction
below (but not tested yet).

>> In the summary, the following three enhancements are a straightforward
>> way to fix up the problem he reported.
>> 1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
>> 2. Add a callback of FDW in ForeignRecheck() - to construct a record
>> according to the fdw_scan_tlist definition and to evaluate its
>> visibility, or to evaluate qualifier pushed-down if base relation.
>> 3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
>> to construct plan nodes for EPQ evaluation.

Likely, what you need to do are...
1. Save the alternative path on fdw_paths when foreign join push-down.
GetForeignJoinPaths() may be called multiple times towards a particular
joinrel according to the combination of innerrel/outerrel.
RelOptInfo->fdw_private allows to avoid construction of same remote
join path multiple times. On the second or later invocation, it may be
a good tactics to reference cheapest_startup_path and replace the saved
one if later invocation have cheaper one, prior to exit.
2. Save the alternative Plan nodes on fdw_plans or lefttree/righttree
somewhere you like at the GetForeignPlan()
3. Makes BeginForeignScan() to call ExecInitNode() towards the plan node
saved at (2), then save the PlanState on fdw_ps, lefttree/righttree,
or somewhere private area if not displayed on EXPLAIN.
4. Implement ForeignRecheck() routine. If scanrelid==0, it kicks the
planstate node saved at (3) to generate tuple slot. Then, call the
ExecQual() to check qualifiers being pushed down.
5. Makes EndForeignScab() to call ExecEndNode() towards the PlanState
saved at (3).

I never think above steps are "too" complicated for people who can write
FDW drivers. It is what developer usually does.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: Kaigai Kouhei(海外 浩平)
> Sent: Wednesday, August 12, 2015 11:17 PM
> To: 'Etsuro Fujita'; Robert Haas
> Cc: PostgreSQL-development; 花田茂
> Subject: RE: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> > -----Original Message-----
> > From: pgsql-hackers-owner(at)postgresql(dot)org
> > [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> > Sent: Wednesday, August 12, 2015 8:26 PM
> > To: Robert Haas; Kaigai Kouhei(海外 浩平)
> > Cc: PostgreSQL-development; 花田茂
> > Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> >
> > On 2015/08/12 7:21, Robert Haas wrote:
> > > On Fri, Aug 7, 2015 at 3:37 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> > >>> I could have a discussion with Fujita-san about this topic.
> > >>>
> > >> Also, let me share with the discussion towards entire solution.
> > >>
> > >> The primitive reason of this problem is, Scan node with scanrelid==0
> > >> represents a relation join that can involve multiple relations, thus,
> > >> its TupleDesc of the records will not fit base relations, however,
> > >> ExecScanFetch() was not updated when scanrelid==0 gets supported.
> > >>
> > >> FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
> > >> to generate records according to the fdw_/custom_scan_tlist that
> > >> reflects the definition of relation join, and only FDW/CSP know how
> > >> to combine these base relations.
> > >> In addition, host-side expressions (like Plan->qual) are initialized
> > >> to reference the records generated by FDW/CSP, so the least invasive
> > >> approach is to allow FDW/CSP to have own logic to recheck, I think.
> > >>
> > >> Below is the structure of ExecScanFetch().
> > >>
> > >> ExecScanFetch(ScanState *node,
> > >> ExecScanAccessMtd accessMtd,
> > >> ExecScanRecheckMtd recheckMtd)
> > >> {
> > >> EState *estate = node->ps.state;
> > >>
> > >> if (estate->es_epqTuple != NULL)
> > >> {
> > >> /*
> > >> * We are inside an EvalPlanQual recheck. Return the test tuple
> > if
> > >> * one is available, after rechecking any access-method-specific
> > >> * conditions.
> > >> */
> > >> Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
> > >>
> > >> Assert(scanrelid > 0);
> > >> if (estate->es_epqTupleSet[scanrelid - 1])
> > >> {
> > >> TupleTableSlot *slot = node->ss_ScanTupleSlot;
> > >> :
> > >> return slot;
> > >> }
> > >> }
> > >> return (*accessMtd) (node);
> > >> }
> > >>
> > >> When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
> > >> checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
> > >> then ExecScan() applies its qualifiers by ExecQual().
> > >> So, as long as FDW/CSP can return a record that satisfies the TupleDesc
> > >> of this relation, made by the tuples in es_epqTuple[] array, rest of the
> > >> code paths are common.
> > >>
> > >> I have an idea to solve the problem.
> > >> It adds recheckMtd() call if scanrelid==0 just before the assertion above,
> > >> and add a callback of FDW on ForeignRecheck().
> > >> The role of this new callback is to set up the supplied TupleTableSlot
> > >> and check its visibility, but does not define how to do this.
> > >> It is arbitrarily by FDW driver, like invocation of alternative plan
> > >> consists of only built-in logic.
> > >>
> > >> Invocation of alternative plan is one of the most feasible way to
> > >> implement EPQ logic on FDW, so I think FDW also needs a mechanism
> > >> that takes child path-nodes like custom_paths in CustomPath node.
> > >> Once a valid path node is linked to this list, createplan.c transform
> > >> them to relevant plan node, then FDW can initialize and invoke this
> > >> plan node during execution, like ForeignRecheck().
> > >>
> > >> This design can solve another problem Fujita-san has also mentioned.
> > >> If scan qualifier is pushed-down to the remote query and its expression
> > >> node is saved in the private area of ForeignScan, the callback on
> > >> ForeignRecheck() can evaluate the qualifier by itself. (Note that only
> > >> FDW driver can know where and how expression node being pushed-down
> > >> is saved in the private area.)
> > >>
> > >> In the summary, the following three enhancements are a straightforward
> > >> way to fix up the problem he reported.
> > >> 1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
> > >> 2. Add a callback of FDW in ForeignRecheck() - to construct a record
> > >> according to the fdw_scan_tlist definition and to evaluate its
> > >> visibility, or to evaluate qualifier pushed-down if base relation.
> > >> 3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
> > >> to construct plan nodes for EPQ evaluation.
> >
> > > I'm not an expert in this area, but this plan does not seem unreasonable to
> > me.
> >
> > IIRC the discussion with KaiGai-san, I think that that would work. I
> > think that that would be more suitable for CSPs, though. Correct me if
> > I'm wrong, KaiGai-san. In either case, I'm not sure that the idea of
> > transferring both processing to a single callback routine hooked in
> > ForeignRecheck is a good idea: (a) check to see if the test tuple for
> > each component foreign table satisfies the remote qual condition and (b)
> > check to see if those tuples satisfy the remote join condition. I think
> > that that would be too complicated, probably making the callback routine
> > bug-prone. So, I'd still propose that *the core* processes (a) and (b)
> > *separately*.
> >
> > * As for (a), the core checks the remote qual condition as in [1].
> >
> > * As for (b), the core executes an alternative subplan locally if inside
> > an EPQ recheck. The subplan is created as described in [2].
> >
> I don't think it is "too" complicated because (a) visibility check of
> the base tuples (saved in es_epqTuple[]) shall be done in the underlying
> base foreign-scan node, executed as a part of alternative plan, and
> (b) evaluation of remote qual is done with ExecQual() call.
>
> I seems to me your proposition tends to assume a particular design
> towards FDW drivers, however, we already have various kind of FDW
> drivers not only wrapper of remote RDBMS.
> https://wiki.postgresql.org/wiki/Foreign_data_wrappers
>
> Is the [1] and [2] suitable for "all" of them, actually?
>
> Let's assume a FDW module that implements own columnar storage,
> has a special JOIN capability if both side are its columnar storage.
> Does it need alternative sub-plan for EPQ rechecks? Probably no,
> because it has own capability to run JOIN by itself.
> It is inconvenience for this FDW if core automatically kicks sub-
> plan in spite of its own functionality/capability.
>
> If potential bugs are concerned, a common part shall be cut down
> and provided as a utility function. FDW can determine whether it
> shall be used, but never enforced.
>
> Thanks,
> --
> NEC Business Creation Division / PG-Strom Project
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Fujita-san,

How about your opinion towards the solution?
CF:Sep will start next week, so I'd like to make a consensus of
the direction, at least.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Kouhei Kaigai
> Sent: Thursday, August 13, 2015 10:13 AM
> To: Etsuro Fujita; Robert Haas
> Cc: PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> Fujita-san,
>
> The attached patch enhanced the FDW interface according to the direction
> below (but not tested yet).
>
> >> In the summary, the following three enhancements are a straightforward
> >> way to fix up the problem he reported.
> >> 1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
> >> 2. Add a callback of FDW in ForeignRecheck() - to construct a record
> >> according to the fdw_scan_tlist definition and to evaluate its
> >> visibility, or to evaluate qualifier pushed-down if base relation.
> >> 3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
> >> to construct plan nodes for EPQ evaluation.
>
> Likely, what you need to do are...
> 1. Save the alternative path on fdw_paths when foreign join push-down.
> GetForeignJoinPaths() may be called multiple times towards a particular
> joinrel according to the combination of innerrel/outerrel.
> RelOptInfo->fdw_private allows to avoid construction of same remote
> join path multiple times. On the second or later invocation, it may be
> a good tactics to reference cheapest_startup_path and replace the saved
> one if later invocation have cheaper one, prior to exit.
> 2. Save the alternative Plan nodes on fdw_plans or lefttree/righttree
> somewhere you like at the GetForeignPlan()
> 3. Makes BeginForeignScan() to call ExecInitNode() towards the plan node
> saved at (2), then save the PlanState on fdw_ps, lefttree/righttree,
> or somewhere private area if not displayed on EXPLAIN.
> 4. Implement ForeignRecheck() routine. If scanrelid==0, it kicks the
> planstate node saved at (3) to generate tuple slot. Then, call the
> ExecQual() to check qualifiers being pushed down.
> 5. Makes EndForeignScab() to call ExecEndNode() towards the PlanState
> saved at (3).
>
> I never think above steps are "too" complicated for people who can write
> FDW drivers. It is what developer usually does.
>
> Thanks,
> --
> NEC Business Creation Division / PG-Strom Project
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
>
>
> > -----Original Message-----
> > From: Kaigai Kouhei(海外 浩平)
> > Sent: Wednesday, August 12, 2015 11:17 PM
> > To: 'Etsuro Fujita'; Robert Haas
> > Cc: PostgreSQL-development; 花田茂
> > Subject: RE: [HACKERS] Foreign join pushdown vs EvalPlanQual
> >
> > > -----Original Message-----
> > > From: pgsql-hackers-owner(at)postgresql(dot)org
> > > [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> > > Sent: Wednesday, August 12, 2015 8:26 PM
> > > To: Robert Haas; Kaigai Kouhei(海外 浩平)
> > > Cc: PostgreSQL-development; 花田茂
> > > Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> > >
> > > On 2015/08/12 7:21, Robert Haas wrote:
> > > > On Fri, Aug 7, 2015 at 3:37 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> > > >>> I could have a discussion with Fujita-san about this topic.
> > > >>>
> > > >> Also, let me share with the discussion towards entire solution.
> > > >>
> > > >> The primitive reason of this problem is, Scan node with scanrelid==0
> > > >> represents a relation join that can involve multiple relations, thus,
> > > >> its TupleDesc of the records will not fit base relations, however,
> > > >> ExecScanFetch() was not updated when scanrelid==0 gets supported.
> > > >>
> > > >> FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
> > > >> to generate records according to the fdw_/custom_scan_tlist that
> > > >> reflects the definition of relation join, and only FDW/CSP know how
> > > >> to combine these base relations.
> > > >> In addition, host-side expressions (like Plan->qual) are initialized
> > > >> to reference the records generated by FDW/CSP, so the least invasive
> > > >> approach is to allow FDW/CSP to have own logic to recheck, I think.
> > > >>
> > > >> Below is the structure of ExecScanFetch().
> > > >>
> > > >> ExecScanFetch(ScanState *node,
> > > >> ExecScanAccessMtd accessMtd,
> > > >> ExecScanRecheckMtd recheckMtd)
> > > >> {
> > > >> EState *estate = node->ps.state;
> > > >>
> > > >> if (estate->es_epqTuple != NULL)
> > > >> {
> > > >> /*
> > > >> * We are inside an EvalPlanQual recheck. Return the test tuple
> > > if
> > > >> * one is available, after rechecking any
> access-method-specific
> > > >> * conditions.
> > > >> */
> > > >> Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
> > > >>
> > > >> Assert(scanrelid > 0);
> > > >> if (estate->es_epqTupleSet[scanrelid - 1])
> > > >> {
> > > >> TupleTableSlot *slot = node->ss_ScanTupleSlot;
> > > >> :
> > > >> return slot;
> > > >> }
> > > >> }
> > > >> return (*accessMtd) (node);
> > > >> }
> > > >>
> > > >> When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
> > > >> checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
> > > >> then ExecScan() applies its qualifiers by ExecQual().
> > > >> So, as long as FDW/CSP can return a record that satisfies the TupleDesc
> > > >> of this relation, made by the tuples in es_epqTuple[] array, rest of the
> > > >> code paths are common.
> > > >>
> > > >> I have an idea to solve the problem.
> > > >> It adds recheckMtd() call if scanrelid==0 just before the assertion above,
> > > >> and add a callback of FDW on ForeignRecheck().
> > > >> The role of this new callback is to set up the supplied TupleTableSlot
> > > >> and check its visibility, but does not define how to do this.
> > > >> It is arbitrarily by FDW driver, like invocation of alternative plan
> > > >> consists of only built-in logic.
> > > >>
> > > >> Invocation of alternative plan is one of the most feasible way to
> > > >> implement EPQ logic on FDW, so I think FDW also needs a mechanism
> > > >> that takes child path-nodes like custom_paths in CustomPath node.
> > > >> Once a valid path node is linked to this list, createplan.c transform
> > > >> them to relevant plan node, then FDW can initialize and invoke this
> > > >> plan node during execution, like ForeignRecheck().
> > > >>
> > > >> This design can solve another problem Fujita-san has also mentioned.
> > > >> If scan qualifier is pushed-down to the remote query and its expression
> > > >> node is saved in the private area of ForeignScan, the callback on
> > > >> ForeignRecheck() can evaluate the qualifier by itself. (Note that only
> > > >> FDW driver can know where and how expression node being pushed-down
> > > >> is saved in the private area.)
> > > >>
> > > >> In the summary, the following three enhancements are a straightforward
> > > >> way to fix up the problem he reported.
> > > >> 1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
> > > >> 2. Add a callback of FDW in ForeignRecheck() - to construct a record
> > > >> according to the fdw_scan_tlist definition and to evaluate its
> > > >> visibility, or to evaluate qualifier pushed-down if base relation.
> > > >> 3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
> > > >> to construct plan nodes for EPQ evaluation.
> > >
> > > > I'm not an expert in this area, but this plan does not seem unreasonable
> to
> > > me.
> > >
> > > IIRC the discussion with KaiGai-san, I think that that would work. I
> > > think that that would be more suitable for CSPs, though. Correct me if
> > > I'm wrong, KaiGai-san. In either case, I'm not sure that the idea of
> > > transferring both processing to a single callback routine hooked in
> > > ForeignRecheck is a good idea: (a) check to see if the test tuple for
> > > each component foreign table satisfies the remote qual condition and (b)
> > > check to see if those tuples satisfy the remote join condition. I think
> > > that that would be too complicated, probably making the callback routine
> > > bug-prone. So, I'd still propose that *the core* processes (a) and (b)
> > > *separately*.
> > >
> > > * As for (a), the core checks the remote qual condition as in [1].
> > >
> > > * As for (b), the core executes an alternative subplan locally if inside
> > > an EPQ recheck. The subplan is created as described in [2].
> > >
> > I don't think it is "too" complicated because (a) visibility check of
> > the base tuples (saved in es_epqTuple[]) shall be done in the underlying
> > base foreign-scan node, executed as a part of alternative plan, and
> > (b) evaluation of remote qual is done with ExecQual() call.
> >
> > I seems to me your proposition tends to assume a particular design
> > towards FDW drivers, however, we already have various kind of FDW
> > drivers not only wrapper of remote RDBMS.
> > https://wiki.postgresql.org/wiki/Foreign_data_wrappers
> >
> > Is the [1] and [2] suitable for "all" of them, actually?
> >
> > Let's assume a FDW module that implements own columnar storage,
> > has a special JOIN capability if both side are its columnar storage.
> > Does it need alternative sub-plan for EPQ rechecks? Probably no,
> > because it has own capability to run JOIN by itself.
> > It is inconvenience for this FDW if core automatically kicks sub-
> > plan in spite of its own functionality/capability.
> >
> > If potential bugs are concerned, a common part shall be cut down
> > and provided as a utility function. FDW can determine whether it
> > shall be used, but never enforced.
> >
> > Thanks,
> > --
> > NEC Business Creation Division / PG-Strom Project
> > KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Hi KaiGai-san,

On 2015/08/25 10:18, Kouhei Kaigai wrote:
> How about your opinion towards the solution?

>> Likely, what you need to do are...
>> 1. Save the alternative path on fdw_paths when foreign join push-down.
>> GetForeignJoinPaths() may be called multiple times towards a particular
>> joinrel according to the combination of innerrel/outerrel.
>> RelOptInfo->fdw_private allows to avoid construction of same remote
>> join path multiple times. On the second or later invocation, it may be
>> a good tactics to reference cheapest_startup_path and replace the saved
>> one if later invocation have cheaper one, prior to exit.

I'm not sure that the tactics is a good one. I think you probably
assume that GetForeignJoinPaths executes set_cheapest each time that
gets called, but ISTM that that would be expensive. (That is one of the
reason why I think it would be better to hook that routine in
standard_join_search.)

>> 2. Save the alternative Plan nodes on fdw_plans or lefttree/righttree
>> somewhere you like at the GetForeignPlan()
>> 3. Makes BeginForeignScan() to call ExecInitNode() towards the plan node
>> saved at (2), then save the PlanState on fdw_ps, lefttree/righttree,
>> or somewhere private area if not displayed on EXPLAIN.
>> 4. Implement ForeignRecheck() routine. If scanrelid==0, it kicks the
>> planstate node saved at (3) to generate tuple slot. Then, call the
>> ExecQual() to check qualifiers being pushed down.
>> 5. Makes EndForeignScab() to call ExecEndNode() towards the PlanState
>> saved at (3).

>> I never think above steps are "too" complicated for people who can write
>> FDW drivers. It is what developer usually does.

Sorry, my explanation was not accurate, but the design that you proposed
looks complicated beyond necessity. I think we should add an FDW API
for doing something if FDWs have more knowledge about doing that than
the core, but in your proposal, instead of the core, an FDW has to
eventually do a lot of the core's work: ExecInitNode, ExecProcNode,
ExecQual, ExecEndNode and so on.

Thank you for the comments!

Best regards,
Etsuro Fujita

> On 2015/08/25 10:18, Kouhei Kaigai wrote:
> > How about your opinion towards the solution?
>
> >> Likely, what you need to do are...
> >> 1. Save the alternative path on fdw_paths when foreign join push-down.
> >> GetForeignJoinPaths() may be called multiple times towards a particular
> >> joinrel according to the combination of innerrel/outerrel.
> >> RelOptInfo->fdw_private allows to avoid construction of same remote
> >> join path multiple times. On the second or later invocation, it may be
> >> a good tactics to reference cheapest_startup_path and replace the saved
> >> one if later invocation have cheaper one, prior to exit.
>
> I'm not sure that the tactics is a good one. I think you probably
> assume that GetForeignJoinPaths executes set_cheapest each time that
> gets called, but ISTM that that would be expensive. (That is one of the
> reason why I think it would be better to hook that routine in
> standard_join_search.)
>
Here is two different problems. I'd like to identify whether the problem
is "must be solved" or "nice to have". Obviously, failure on EPQ check
is a problem must be solved, however, hook location is nice to have.

In addition, you may misunderstand the proposition of mine above.
You can check RelOptInfo->fdw_private on top of the GetForeignJoinPaths,
then, if it is second or later invocation, you can check cost of the
alternative path kept in the ForeignPath node previously constructed.
If cheapest_total_path at the moment of GetForeignJoinPaths invocation
is cheaper than the saved alternative path, you can adjust the node to
replace the alternative path node.

> >> 2. Save the alternative Plan nodes on fdw_plans or lefttree/righttree
> >> somewhere you like at the GetForeignPlan()
> >> 3. Makes BeginForeignScan() to call ExecInitNode() towards the plan node
> >> saved at (2), then save the PlanState on fdw_ps, lefttree/righttree,
> >> or somewhere private area if not displayed on EXPLAIN.
> >> 4. Implement ForeignRecheck() routine. If scanrelid==0, it kicks the
> >> planstate node saved at (3) to generate tuple slot. Then, call the
> >> ExecQual() to check qualifiers being pushed down.
> >> 5. Makes EndForeignScab() to call ExecEndNode() towards the PlanState
> >> saved at (3).
>
> >> I never think above steps are "too" complicated for people who can write
> >> FDW drivers. It is what developer usually does.
>
> Sorry, my explanation was not accurate, but the design that you proposed
> looks complicated beyond necessity. I think we should add an FDW API
> for doing something if FDWs have more knowledge about doing that than
> the core, but in your proposal, instead of the core, an FDW has to
> eventually do a lot of the core's work: ExecInitNode, ExecProcNode,
> ExecQual, ExecEndNode and so on.
>
It is a trade-off problem between interface flexibility and code smallness
of FDW extension if it fits scope of the core support.
I stand on the viewpoint that gives highest priority on the flexibility,
especially, in case when unpredictable type of modules are expected.
Your proposition is comfortable to FDW on behalf of RDBMS, however, nobody
can promise it is beneficial to FDW on behalf of columnar-store for example.

If you stick on the code smallness of FDW on behalf of RDBMS, we can add
utility functions on foreign.c or somewhere. It will be able to provide
equivalent functionality, but FDW can determine whether it use the routines.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/08/26 13:49, Kouhei Kaigai wrote:
>> On 2015/08/25 10:18, Kouhei Kaigai wrote:
>>>> Likely, what you need to do are...
>>>> 1. Save the alternative path on fdw_paths when foreign join push-down.
>>>> GetForeignJoinPaths() may be called multiple times towards a particular
>>>> joinrel according to the combination of innerrel/outerrel.
>>>> RelOptInfo->fdw_private allows to avoid construction of same remote
>>>> join path multiple times. On the second or later invocation, it may be
>>>> a good tactics to reference cheapest_startup_path and replace the saved
>>>> one if later invocation have cheaper one, prior to exit.

>> I'm not sure that the tactics is a good one. I think you probably
>> assume that GetForeignJoinPaths executes set_cheapest each time that
>> gets called, but ISTM that that would be expensive. (That is one of the
>> reason why I think it would be better to hook that routine in
>> standard_join_search.)

> Here is two different problems. I'd like to identify whether the problem
> is "must be solved" or "nice to have". Obviously, failure on EPQ check
> is a problem must be solved, however, hook location is nice to have.

OK I'll focus on the "must be solved" problem at least on this thread.

> In addition, you may misunderstand the proposition of mine above.
> You can check RelOptInfo->fdw_private on top of the GetForeignJoinPaths,
> then, if it is second or later invocation, you can check cost of the
> alternative path kept in the ForeignPath node previously constructed.
> If cheapest_total_path at the moment of GetForeignJoinPaths invocation
> is cheaper than the saved alternative path, you can adjust the node to
> replace the alternative path node.

To get the (probably unparameterized) cheapest_total_path, IIUC, we need
to do set_cheapest during GetForeignJoinPaths in each subsequent
invocation of that routine, don't we? And set_cheapest is expensive,
isn't it?

>>>> 2. Save the alternative Plan nodes on fdw_plans or lefttree/righttree
>>>> somewhere you like at the GetForeignPlan()
>>>> 3. Makes BeginForeignScan() to call ExecInitNode() towards the plan node
>>>> saved at (2), then save the PlanState on fdw_ps, lefttree/righttree,
>>>> or somewhere private area if not displayed on EXPLAIN.
>>>> 4. Implement ForeignRecheck() routine. If scanrelid==0, it kicks the
>>>> planstate node saved at (3) to generate tuple slot. Then, call the
>>>> ExecQual() to check qualifiers being pushed down.
>>>> 5. Makes EndForeignScab() to call ExecEndNode() towards the PlanState
>>>> saved at (3).

>> but the design that you proposed
>> looks complicated beyond necessity. I think we should add an FDW API
>> for doing something if FDWs have more knowledge about doing that than
>> the core, but in your proposal, instead of the core, an FDW has to
>> eventually do a lot of the core's work: ExecInitNode, ExecProcNode,
>> ExecQual, ExecEndNode and so on.

> It is a trade-off problem between interface flexibility and code smallness
> of FDW extension if it fits scope of the core support.
> I stand on the viewpoint that gives highest priority on the flexibility,
> especially, in case when unpredictable type of modules are expected.
> Your proposition is comfortable to FDW on behalf of RDBMS, however, nobody
> can promise it is beneficial to FDW on behalf of columnar-store for example.

Maybe I'm missing something, but why do we need such a flexiblity for
the columnar-stores?

> If you stick on the code smallness of FDW on behalf of RDBMS, we can add
> utility functions on foreign.c or somewhere. It will be able to provide
> equivalent functionality, but FDW can determine whether it use the routines.

That might be an idea, but I'd like to hear the opinions of others.

Best regards,
Etsuro Fujita

> > In addition, you may misunderstand the proposition of mine above.
> > You can check RelOptInfo->fdw_private on top of the GetForeignJoinPaths,
> > then, if it is second or later invocation, you can check cost of the
> > alternative path kept in the ForeignPath node previously constructed.
> > If cheapest_total_path at the moment of GetForeignJoinPaths invocation
> > is cheaper than the saved alternative path, you can adjust the node to
> > replace the alternative path node.
>
> To get the (probably unparameterized) cheapest_total_path, IIUC, we need
> to do set_cheapest during GetForeignJoinPaths in each subsequent
> invocation of that routine, don't we? And set_cheapest is expensive,
> isn't it?
>
add_path() usually drop paths that are obviously lesser than others,
so walk on join->pathlist shall have reasonable length.
Even though it has hundreds items on the pathlist, you CAN implement
EPQ fallback using alternative built-in logic.

> >>>> 2. Save the alternative Plan nodes on fdw_plans or lefttree/righttree
> >>>> somewhere you like at the GetForeignPlan()
> >>>> 3. Makes BeginForeignScan() to call ExecInitNode() towards the plan node
> >>>> saved at (2), then save the PlanState on fdw_ps, lefttree/righttree,
> >>>> or somewhere private area if not displayed on EXPLAIN.
> >>>> 4. Implement ForeignRecheck() routine. If scanrelid==0, it kicks the
> >>>> planstate node saved at (3) to generate tuple slot. Then, call the
> >>>> ExecQual() to check qualifiers being pushed down.
> >>>> 5. Makes EndForeignScab() to call ExecEndNode() towards the PlanState
> >>>> saved at (3).
>
> >> but the design that you proposed
> >> looks complicated beyond necessity. I think we should add an FDW API
> >> for doing something if FDWs have more knowledge about doing that than
> >> the core, but in your proposal, instead of the core, an FDW has to
> >> eventually do a lot of the core's work: ExecInitNode, ExecProcNode,
> >> ExecQual, ExecEndNode and so on.
>
> > It is a trade-off problem between interface flexibility and code smallness
> > of FDW extension if it fits scope of the core support.
> > I stand on the viewpoint that gives highest priority on the flexibility,
> > especially, in case when unpredictable type of modules are expected.
> > Your proposition is comfortable to FDW on behalf of RDBMS, however, nobody
> > can promise it is beneficial to FDW on behalf of columnar-store for example.
>
> Maybe I'm missing something, but why do we need such a flexiblity for
> the columnar-stores?
>
We have various kind of FDW drivers, some of use cases were unpredictable
preliminary. Our community knows 86 kind of FDW drivers in total, and only
15 of them are for RDBMS but 71 of them for other data source.
https://wiki.postgresql.org/wiki/Foreign_data_wrappers

Even if we enforce them a new interface specification comfortable to RDBMS,
we cannot guarantee it is also comfortable to other type of FDW drivers.

If module-X wants to implement the EPQ fallback routine by itself, without
alternative plan, too rich interface design prevents what module-X really
wants to do.

> > If you stick on the code smallness of FDW on behalf of RDBMS, we can add
> > utility functions on foreign.c or somewhere. It will be able to provide
> > equivalent functionality, but FDW can determine whether it use the routines.
>
> That might be an idea, but I'd like to hear the opinions of others.
>

--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/08/26 16:07, Kouhei Kaigai wrote:
I wrote:
>> Maybe I'm missing something, but why do we need such a flexiblity for
>> the columnar-stores?

> Even if we enforce them a new interface specification comfortable to RDBMS,
> we cannot guarantee it is also comfortable to other type of FDW drivers.

Specifically, what kind of points about the patch are specific to RDBMS?

> If module-X wants to implement the EPQ fallback routine by itself, without
> alternative plan, too rich interface design prevents what module-X really
> wants to do.

Sorry, I fail to see the need or advantage for module-X to do so, in
practice because I think EPQ testing is only execute a subplan for a
*single* set of component test tuples. Maybe I'm missing something, though.

Best regards,
Etsuro Fujita

> On 2015/08/26 16:07, Kouhei Kaigai wrote:
> I wrote:
> >> Maybe I'm missing something, but why do we need such a flexiblity for
> >> the columnar-stores?
>
> > Even if we enforce them a new interface specification comfortable to RDBMS,
> > we cannot guarantee it is also comfortable to other type of FDW drivers.
>
> Specifically, what kind of points about the patch are specific to RDBMS?
>

*** 88,93 **** ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
--- 99,122 ----
TupleTableSlot *
ExecForeignScan(ForeignScanState *node)
{
+ EState *estate = node->ss.ps.state;
+
+ if (estate->es_epqTuple != NULL)
+ {
+ /*
+ * We are inside an EvalPlanQual recheck. If foreign join, get next
+ * tuple from subplan.
+ */
+ Index scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
+
+ if (scanrelid == 0)
+ {
+ PlanState *outerPlan = outerPlanState(node);
+
+ return ExecProcNode(outerPlan);
+ }
+ }
+
return ExecScan((ScanState *) node,
(ExecScanAccessMtd) ForeignNext,
(ExecScanRecheckMtd) ForeignRecheck);

It might not be specific to RDBMS, however, we cannot guarantee all the FDW are
comfortable to run the alternative plan node on EPQ recheck.
This design does not allow FDW drivers to implement own EPQ recheck, possibly
more efficient than built-in logic.

I never deny to run the alternative plan to implement EPQ recheck, according
to the decision by FDW driver, however, it is unacceptable pain to enforce all
the FDW driver to use alternative plan as a solution of EPQ check.

> > If module-X wants to implement the EPQ fallback routine by itself, without
> > alternative plan, too rich interface design prevents what module-X really
> > wants to do.
>
> Sorry, I fail to see the need or advantage for module-X to do so, in
> practice because I think EPQ testing is only execute a subplan for a
> *single* set of component test tuples. Maybe I'm missing something, though.
>
You may think execution of alternative plan is the best way for EPQ rechecks,
however, other folks may think their own implementation is the best for EPQ
rechecks. I never argue which approach is better.
What I point out is freedom/flexibility of implementation choice.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/08/26 17:05, Kouhei Kaigai wrote:
>> On 2015/08/26 16:07, Kouhei Kaigai wrote:
>>> Even if we enforce them a new interface specification comfortable to RDBMS,
>>> we cannot guarantee it is also comfortable to other type of FDW drivers.

>> Specifically, what kind of points about the patch are specific to RDBMS?

> TupleTableSlot *
> ExecForeignScan(ForeignScanState *node)
> {
> + EState *estate = node->ss.ps.state;
> +
> + if (estate->es_epqTuple != NULL)
> + {
> + /*
> + * We are inside an EvalPlanQual recheck. If foreign join, get next
> + * tuple from subplan.
> + */
> + Index scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
> +
> + if (scanrelid == 0)
> + {
> + PlanState *outerPlan = outerPlanState(node);
> +
> + return ExecProcNode(outerPlan);
> + }
> + }

> It might not be specific to RDBMS, however, we cannot guarantee all the FDW are
> comfortable to run the alternative plan node on EPQ recheck.
> This design does not allow FDW drivers to implement own EPQ recheck, possibly
> more efficient than built-in logic.

As I said below, EPQ testing is only execute a subplan for a *single*
set of component test tuples, so I think the performance gain by its own
EPQ testing implemented by an FDW would be probably negligible in
practice. No?

>>> If module-X wants to implement the EPQ fallback routine by itself, without
>>> alternative plan, too rich interface design prevents what module-X really
>>> wants to do.
>>
>> Sorry, I fail to see the need or advantage for module-X to do so, in
>> practice because I think EPQ testing is only execute a subplan for a
>> *single* set of component test tuples. Maybe I'm missing something, though.
>>
> You may think execution of alternative plan is the best way for EPQ rechecks,
> however, other folks may think their own implementation is the best for EPQ
> rechecks. I never argue which approach is better.
> What I point out is freedom/flexibility of implementation choice.

No, I just want to know the need or advantage for that specifically.

Best regards,
Etsuro Fujita

> -----Original Message-----
> From: Etsuro Fujita [mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp]
> Sent: Wednesday, August 26, 2015 5:38 PM
> To: Kaigai Kouhei(海外 浩平); Robert Haas
> Cc: PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> On 2015/08/26 17:05, Kouhei Kaigai wrote:
> >> On 2015/08/26 16:07, Kouhei Kaigai wrote:
> >>> Even if we enforce them a new interface specification comfortable to RDBMS,
> >>> we cannot guarantee it is also comfortable to other type of FDW drivers.
>
> >> Specifically, what kind of points about the patch are specific to RDBMS?
>
> > TupleTableSlot *
> > ExecForeignScan(ForeignScanState *node)
> > {
> > + EState *estate = node->ss.ps.state;
> > +
> > + if (estate->es_epqTuple != NULL)
> > + {
> > + /*
> > + * We are inside an EvalPlanQual recheck. If foreign join, get
> next
> > + * tuple from subplan.
> > + */
> > + Index scanrelid = ((Scan *)
> node->ss.ps.plan)->scanrelid;
> > +
> > + if (scanrelid == 0)
> > + {
> > + PlanState *outerPlan = outerPlanState(node);
> > +
> > + return ExecProcNode(outerPlan);
> > + }
> > + }
>
> > It might not be specific to RDBMS, however, we cannot guarantee all the FDW
> are
> > comfortable to run the alternative plan node on EPQ recheck.
> > This design does not allow FDW drivers to implement own EPQ recheck, possibly
> > more efficient than built-in logic.
>
> As I said below, EPQ testing is only execute a subplan for a *single*
> set of component test tuples, so I think the performance gain by its own
> EPQ testing implemented by an FDW would be probably negligible in
> practice. No?
>
> >>> If module-X wants to implement the EPQ fallback routine by itself, without
> >>> alternative plan, too rich interface design prevents what module-X really
> >>> wants to do.
> >>
> >> Sorry, I fail to see the need or advantage for module-X to do so, in
> >> practice because I think EPQ testing is only execute a subplan for a
> >> *single* set of component test tuples. Maybe I'm missing something, though.
> >>
> > You may think execution of alternative plan is the best way for EPQ rechecks,
> > however, other folks may think their own implementation is the best for EPQ
> > rechecks. I never argue which approach is better.
> > What I point out is freedom/flexibility of implementation choice.
>
> No, I just want to know the need or advantage for that specifically.
>
I'm not interested in advantage / disadvantage of individual FDW driver's
implementation. It is matter of FDW drivers, not a matter of core PostgreSQL.

The only and significant point I repeatedly emphasized is, it is developer's
choice thus it is important to provide options for developers.
If they want, FDW developer can follow the manner of alternative plan
execution for EPQ rechecks. I never deny your idea, but should be one of
the options we can take.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/08/26 18:01, Kouhei Kaigai wrote:
>>> You may think execution of alternative plan is the best way for EPQ rechecks,
>>> however, other folks may think their own implementation is the best for EPQ
>>> rechecks. I never argue which approach is better.
>>> What I point out is freedom/flexibility of implementation choice.

Maybe my explanation was not accurate, but I just want to know use
cases, to understand the need to provide the flexiblity.

> The only and significant point I repeatedly emphasized is, it is developer's
> choice thus it is important to provide options for developers.
> If they want, FDW developer can follow the manner of alternative plan
> execution for EPQ rechecks. I never deny your idea, but should be one of
> the options we can take.

I don't object about your idea either, but I have a concern about that;
it looks like that the more flexiblity we provide, the more the FDWs
implementing their own EPQ would be subject to an internal change in the
core.

Best regards,
Etsuro Fujita

> On 2015/08/26 18:01, Kouhei Kaigai wrote:
> >>> You may think execution of alternative plan is the best way for EPQ rechecks,
> >>> however, other folks may think their own implementation is the best for EPQ
> >>> rechecks. I never argue which approach is better.
> >>> What I point out is freedom/flexibility of implementation choice.
>
> Maybe my explanation was not accurate, but I just want to know use
> cases, to understand the need to provide the flexiblity.
>
Let's assume the following situation:

Someone wants to implement FPGA acceleration feature on top of FDW.
(You may know the earliest PG-Strom was built on FDW interface)
It enables to run SQL join workloads on FPGA device, but has equivalent
fallback routines to be executed if FPGA returned an error.
On EPQ check case, it is quite natural that he wants to re-use this
fallback routine to validate EPQ tuple. Alternative plan may consume
additional (at least not zero) memory and other system resource.

As I have said repeatedly, it is software design decision by the author
of extension. Even if it consumes 100 times larger memory and 1000 times
slower, it is his decision and responsibility.
Why he has to be forced to use a particular logic despite his intension?

> > The only and significant point I repeatedly emphasized is, it is developer's
> > choice thus it is important to provide options for developers.
> > If they want, FDW developer can follow the manner of alternative plan
> > execution for EPQ rechecks. I never deny your idea, but should be one of
> > the options we can take.
>
> I don't object about your idea either, but I have a concern about that;
> it looks like that the more flexiblity we provide, the more the FDWs
> implementing their own EPQ would be subject to an internal change in the
> core.
>
We never guarantee interface compatibility across major versions. All we
can say is 'best efforts'. So, it is always role of extension owner, as
long as he continue to maintain his module.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/08/27 11:08, Kouhei Kaigai wrote:
>> On 2015/08/26 18:01, Kouhei Kaigai wrote:
>>>>> You may think execution of alternative plan is the best way for EPQ rechecks,
>>>>> however, other folks may think their own implementation is the best for EPQ
>>>>> rechecks. I never argue which approach is better.
>>>>> What I point out is freedom/flexibility of implementation choice.

>> Maybe my explanation was not accurate, but I just want to know use
>> cases, to understand the need to provide the flexiblity.

> Let's assume the following situation:

> Someone wants to implement FPGA acceleration feature on top of FDW.

> It enables to run SQL join workloads on FPGA device, but has equivalent
> fallback routines to be executed if FPGA returned an error.
> On EPQ check case, it is quite natural that he wants to re-use this
> fallback routine to validate EPQ tuple. Alternative plan may consume
> additional (at least not zero) memory and other system resource.

Thanks for the answer, but I'm not still convinced. I think the EPQ
testing shown in that use-case would probably not efficient, compared to
the core's.

> As I have said repeatedly, it is software design decision by the author
> of extension. Even if it consumes 100 times larger memory and 1000 times
> slower, it is his decision and responsibility.
> Why he has to be forced to use a particular logic despite his intension?

I don't understand what you proposed, but ISTM that your proposal is
more like a feature, rather than a bugfix. For what you proposed, I
think we should also improve the existing EPQ mechanism including the
corresponding FDW routines. One possible improvement is the behavior of
late row locking. Currently, we do that by 1) re-fetching each
component tuple from the foreign table after locking it by
RefetchForeignRow and then 2) if necessary, doing an EPQ recheck, ie,
re-running the query locally for such component tuples by the core. So,
if we could re-run the join part of the query remotely without
tranferring such component tuples from the foreign tables, we would be
able to not only avoid useless data transfer but improve concurrency
when the join fails.

So, how about addressing this issue in two steps; first, work on the
bugfix patch in [1], and then, work on what you propsed. The latter
would need more discussion/work, so I think it would be better to take
that in 9.6. If it's OK, I'll update the patch in [1] and add it to the
upcoming CF.

>> I don't object about your idea either, but I have a concern about that;
>> it looks like that the more flexiblity we provide, the more the FDWs
>> implementing their own EPQ would be subject to an internal change in the
>> core.

> We never guarantee interface compatibility across major versions. All we
> can say is 'best efforts'. So, it is always role of extension owner, as
> long as he continue to maintain his module.

I think we cannot 100% guarantee the compatibility. That is why I think
we should avoid an FDW improvement that would be subject to an internal
change in the core, unless there is a good reason or use-case for that.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/55CB2D45.7040100@lab.ntt.co.jp

> > As I have said repeatedly, it is software design decision by the author
> > of extension. Even if it consumes 100 times larger memory and 1000 times
> > slower, it is his decision and responsibility.
> > Why he has to be forced to use a particular logic despite his intension?
>
> I don't understand what you proposed,
>
What I'm talking about is philosophy of software/interface design.
I understand EPQ recheck by alternative plan is "one" reasonable way,
however, people often have different ideas and may be better than
your idea depending on its context/environment/prerequisites/etc...
It is always unpredictable, only God can know what is the best solution.

In other words, I didn't talk about taste of restaurant, the problem is
lack of variation on the menu. You may not want, but we have freedom to
eat terrible taste meal.

> but ISTM that your proposal is
> more like a feature, rather than a bugfix.
>
Yes, the problem we are facing is lack of a feature. It might be my
oversight when I designed join pushdown infrastructure. Sorry.
So, it is quite natural to add the missing piece to fix up the bug.

> For what you proposed, I
> think we should also improve the existing EPQ mechanism including the
> corresponding FDW routines. One possible improvement is the behavior of
> late row locking. Currently, we do that by 1) re-fetching each
> component tuple from the foreign table after locking it by
> RefetchForeignRow and then 2) if necessary, doing an EPQ recheck, ie,
> re-running the query locally for such component tuples by the core. So,
> if we could re-run the join part of the query remotely without
> tranferring such component tuples from the foreign tables, we would be
> able to not only avoid useless data transfer but improve concurrency
> when the join fails.
>
> So, how about addressing this issue in two steps; first, work on the
> bugfix patch in [1], and then, work on what you propsed. The latter
> would need more discussion/work, so I think it would be better to take
> that in 9.6. If it's OK, I'll update the patch in [1] and add it to the
> upcoming CF.
>
It seems to me too invasive for bugfix, and assumes a particular solution.
Please do the rechecking part in the extension, not in the core.

> >> I don't object about your idea either, but I have a concern about that;
> >> it looks like that the more flexiblity we provide, the more the FDWs
> >> implementing their own EPQ would be subject to an internal change in the
> >> core.
>
> > We never guarantee interface compatibility across major versions. All we
> > can say is 'best efforts'. So, it is always role of extension owner, as
> > long as he continue to maintain his module.
>
> I think we cannot 100% guarantee the compatibility. That is why I think
> we should avoid an FDW improvement that would be subject to an internal
> change in the core, unless there is a good reason or use-case for that.
>
It does not make sense unless we don't provide stable and well specified
interface, because developers will have validation and adjustment of their
extension to new major versions.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/08/27 16:52, Kouhei Kaigai wrote:
I wrote:
>> I don't understand what you proposed,

> What I'm talking about is philosophy of software/interface design.
> I understand EPQ recheck by alternative plan is "one" reasonable way,
> however, people often have different ideas and may be better than
> your idea depending on its context/environment/prerequisites/etc...
> It is always unpredictable, only God can know what is the best solution.
>
> In other words, I didn't talk about taste of restaurant, the problem is
> lack of variation on the menu. You may not want, but we have freedom to
> eat terrible taste meal.

>> but ISTM that your proposal is
>> more like a feature, rather than a bugfix.

> Yes, the problem we are facing is lack of a feature. It might be my
> oversight when I designed join pushdown infrastructure. Sorry.
> So, it is quite natural to add the missing piece to fix up the bug.

>> For what you proposed, I
>> think we should also improve the existing EPQ mechanism including the
>> corresponding FDW routines. One possible improvement is the behavior of
>> late row locking. Currently, we do that by 1) re-fetching each
>> component tuple from the foreign table after locking it by
>> RefetchForeignRow and then 2) if necessary, doing an EPQ recheck, ie,
>> re-running the query locally for such component tuples by the core. So,
>> if we could re-run the join part of the query remotely without
>> tranferring such component tuples from the foreign tables, we would be
>> able to not only avoid useless data transfer but improve concurrency
>> when the join fails.
>>
>> So, how about addressing this issue in two steps; first, work on the
>> bugfix patch in [1], and then, work on what you propsed. The latter
>> would need more discussion/work, so I think it would be better to take
>> that in 9.6. If it's OK, I'll update the patch in [1] and add it to the
>> upcoming CF.

> It seems to me too invasive for bugfix, and assumes a particular solution.
> Please do the rechecking part in the extension, not in the core.

I think we would probably need others' opinions about this issue.

Best regards,
Etsuro Fujita

On 2015/08/27 17:30, Etsuro Fujita wrote:
> I think we would probably need others' opinions about this issue.

Attached is an updated version of the patch [1]. I'd be happy if it
helps people discuss about this issue.

Changes:
* rebased to HEAD.
* add some more docs and comments.
* fix a bug in handling tlist of a ForeignScan node when the node is the
top node.
* fix a bug in doing ExecAssignScanTypeFromOuterPlan at the top of a
ForeignScan node.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/55CB2D45.7040100@lab.ntt.co.jp

On 2015/08/01 23:25, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> The problem that was bothering us (or at least what was bothering me)
>> is that the PlannerInfo provides only a list of SpecialJoinInfo
>> structures, which don't directly give you the original join order. In
>> fact, min_righthand and min_lefthand are intended to constraint the
>> *possible* join orders, and are deliberately designed *not* to specify
>> a single join order. If you're sending a query to a remote PostgreSQL
>> node, you don't want to know what all the possible join orders are;
>> it's the remote side's job to plan the query. You do, however, need
>> an easy way to identify one join order that you can use to construct a
>> query. It didn't seem easy to do that without duplicating
>> make_join_rel(), which seemed like a bad idea.

> In principle it seems like you could traverse root->parse->jointree
> as a guide to reconstructing the original syntactic structure; though
> I'm not sure how hard it would be to ignore the parts of that tree
> that correspond to relations you're not shipping.

I'll investigate this.

>> But maybe there's a good way to do it. Tom wasn't crazy about this
>> hook both because of the frequency of calls and also because of the
>> long argument list. I think those concerns are legitimate; I just
>> couldn't see how to make the other way work.

> In my vision you probably really only want one call per build_join_rel
> event (that is, per construction of a new RelOptInfo), not per
> make_join_rel event.
>
> It's possible that an FDW that wants to handle joins but is not talking to
> a remote query planner would need to grovel through all the join ordering
> possibilities individually, and then maybe hooking at make_join_rel is
> sensible rather than having to reinvent that logic. But I'd want to see a
> concrete use-case first, and I certainly don't think that that's the main
> case to design the API around.

I'd vote for hooking at standard_join_search. Here is a use-case:

* When the callback routine is hooked at that funcition (right after
allpaths.c:1817), an FDW would collect lists of all the available
local-join-path orderings and parameterizations by looking at each path
in rel->pathlist (if the join rel only contains foreign tables that all
belong to the same foreign server).

* Then the FDW would use these as a heuristic to indcate which sort
orderings and parameterizations we should build foreign-join paths for.
(These would be also used as alternative paths for EvalPlanQual
handling, as discussed upthread.) It seems reasonable to me to consider
pushed-down versions of these paths as first candidates, but
foreign-join paths to build are not limited to such ones. The FDW is
allowed to consider any foreign-join paths as long as their alternative
paths are provided.

IMO one thing to consider for the postgres_fdw case would be the
use_remote_estimate option. In the case when the option is true, I
think we should perform remote EXPLAINs for pushed-down-join queries to
obtain cost estimates. But it would require too much time to do that
for each of the possible join rel. So, I think it would be better to
put off the callback routine's work as long as possible. I think that
that could probably be done by looking at rel->joininfo,
root->join_info_list and/or something like that. (When considering a
join rel A JOIN B both on the same foreign server, for example, we can
skip the routine's work if the join rel proved to be joined with C on
the same foreign server by looking at rel->joininfo, for example.)
Maybe I'm missing something, though.

Best regards,
Etsuro Fujita

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> On 2015/08/01 23:25, Tom Lane wrote:
>> In my vision you probably really only want one call per build_join_rel
>> event (that is, per construction of a new RelOptInfo), not per
>> make_join_rel event.

> I'd vote for hooking at standard_join_search.

I think that method would require the FDW to duplicate a whole lot of the
join search mechanism, for not a whole lot of benefit. It's possible that
there'd be value in doing some initial reconnaissance once we've examined
all the baserels, so I'm not necessarily against providing a hook there.
But if you have in mind that typical FDWs would actually create join paths
at that point, consider that

1. The FDW would have to find all the combinations of its supplied
relations (unless you are only intending to generate one path for the
union of all such rels, which seems pretty narrow-minded from here).

2. The FDW would have to account for join_is_legal considerations.

3. The FDW would have to arrange for creation of joinrel RelOptInfo
structures. While that's possible, the available infrastructure for it
assumes that joinrels are built up from pairs of simpler joinrels, so
you couldn't go directly to the union of all the FDW's rels anyway.

So I still think that the most directly useful infrastructure here
would involve, when build_join_rel() first creates a given joinrel,
noticing whether both sides belong to the same foreign server and
if so giving the FDW a callback to consider producing pushed-down
joins. That would be extremely cheap to do and it would not involve
adding overhead for an FDW to discover what the valid sets of joins
are. In a large join problem, that's *not* going to be a cheap
thing to duplicate. If there are multiple FDWs involved, the idea
that each one of them would do its own join search is particularly
horrid.

One other problem with the proposal is that we might never call
standard_join_search at all: GEQO overrides it, and so can external
users of join_search_hook.

regards, tom lane

On Wed, Sep 2, 2015 at 10:30 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> But if you have in mind that typical FDWs would actually create join paths
> at that point, consider that
>
> 1. The FDW would have to find all the combinations of its supplied
> relations (unless you are only intending to generate one path for the
> union of all such rels, which seems pretty narrow-minded from here).

Well, if the remote end is another database server, presumably we can
leave it to optimize the query, so why would we need more than one
path? I can see that we need more than one path because of sort-order
considerations, which would affect the query we ship to the remote
side. But I don't see the point of considering multiple join orders
unless the remote end is dumber than our optimizer, which might be
true in some cases, but not if the remote end is PostgreSQL.

> 2. The FDW would have to account for join_is_legal considerations.

I agree with this.

> 3. The FDW would have to arrange for creation of joinrel RelOptInfo
> structures. While that's possible, the available infrastructure for it
> assumes that joinrels are built up from pairs of simpler joinrels, so
> you couldn't go directly to the union of all the FDW's rels anyway.

And with this.

> So I still think that the most directly useful infrastructure here
> would involve, when build_join_rel() first creates a given joinrel,
> noticing whether both sides belong to the same foreign server and
> if so giving the FDW a callback to consider producing pushed-down
> joins. That would be extremely cheap to do and it would not involve
> adding overhead for an FDW to discover what the valid sets of joins
> are. In a large join problem, that's *not* going to be a cheap
> thing to duplicate. If there are multiple FDWs involved, the idea
> that each one of them would do its own join search is particularly
> horrid.

So, the problem is that I don't think this entirely skirts the
join_is_legal issues, which are a principal point of concern for me.
Say this is a joinrel between (A B) and (C D E). We need to generate
an SQL query for (A B C D E). We know that the outermost syntactic
join can be (A B) to (C D E). But how do we know which join orders
are legal as among (C D E)? Maybe there's a simple way to handle this
that I'm not seeing.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Sep 2, 2015 at 10:30 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> But if you have in mind that typical FDWs would actually create join paths
>> at that point, consider that
>>
>> 1. The FDW would have to find all the combinations of its supplied
>> relations (unless you are only intending to generate one path for the
>> union of all such rels, which seems pretty narrow-minded from here).

> Well, if the remote end is another database server, presumably we can
> leave it to optimize the query, so why would we need more than one
> path?

If you have say 5 relations in the query, 3 of which are foreign, it might
make sense to join all 3 at the remote end, or maybe you should only join
2 of them remotely because it's better to then join to one of the local
rels before joining the last remote rel. Even if you claim that that
would never make sense from a cost standpoint (a claim easily seen to be
silly), there might not be any legal way to join all 3 directly because of
join order constraints.

The larger point is that we can't expect the remote server to be fully
responsible for optimizing, because it will know nothing of what's being
done on our end.

> I can see that we need more than one path because of sort-order
> considerations, which would affect the query we ship to the remote
> side. But I don't see the point of considering multiple join orders
> unless the remote end is dumber than our optimizer, which might be
> true in some cases, but not if the remote end is PostgreSQL.

(1) not all remote ends are Postgres, (2) the remote end doesn't have any
access to info about our end.

> So, the problem is that I don't think this entirely skirts the
> join_is_legal issues, which are a principal point of concern for me.
> Say this is a joinrel between (A B) and (C D E). We need to generate
> an SQL query for (A B C D E). We know that the outermost syntactic
> join can be (A B) to (C D E). But how do we know which join orders
> are legal as among (C D E)? Maybe there's a simple way to handle this
> that I'm not seeing.

Well, if the joins get built up in the way I think should happen, we'd
have already considered (C D E), and we could have recorded the legal join
orders within that at the time. (I imagine that we should allow FDWs to
store some data within RelOptInfo structs that represent foreign joins
belonging entirely to them, so that there'd be a handy place to keep that
data till later.) Or we could trawl through the paths associated with the
child joinrel, which will presumably include instances of every reasonable
sub-join combination. Or the FDW could look at the SpecialJoinInfo data
and determine things for itself (or more likely, ask join_is_legal about
that).

In the case of postgres_fdw, I think the actual requirement will be to be
able to reconstruct a SQL query that correctly expresses the join; that
is, we need to send over something like "from c left join d on (...) full
join e on (...)", not just "from c, d, e", or we'll get totally bogus
estimates as well as bogus execution results. Offhand I think that the
most likely way to build that text will be to examine the query's jointree
to see where c,d,e appear in it. But in any case, that's a separate issue
and I fail to see how plopping the join search problem into the FDW's lap
would make it any easier.

regards, tom lane

I wrote:
> ... I imagine that we should allow FDWs to
> store some data within RelOptInfo structs that represent foreign joins
> belonging entirely to them, so that there'd be a handy place to keep that
> data till later.

Actually, if we do that (ie, provide a "void *fdw_state" field in join
RelOptInfos), then the FDW could use the nullness or not-nullness of
such a field to realize whether or not it had already considered this
join relation. So I'm now thinking that the best API is to call the
FDW at the end of each make_join_rel call, whether it's the first one
for the joinrel or not. If the FDW wants a call for each legal pair of
input sub-relations, it's got one. If it only wants one call per joinrel,
it can just make sure to put something into fdw_state, and then on
subsequent calls for the same joinrel it can just exit immediately if
fdw_state is already non-null. So we have both use-cases covered.
Also, by doing this at the end, the FDW can look at the "regular" (local
join execution) paths that were already generated, should it wish to.

regards, tom lane

, On Wed, Aug 26, 2015 at 4:05 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>> On 2015/08/26 16:07, Kouhei Kaigai wrote:
>> I wrote:
>> >> Maybe I'm missing something, but why do we need such a flexiblity for
>> >> the columnar-stores?
>>
>> > Even if we enforce them a new interface specification comfortable to RDBMS,
>> > we cannot guarantee it is also comfortable to other type of FDW drivers.
>>
>> Specifically, what kind of points about the patch are specific to RDBMS?
>>
>
> *** 88,93 **** ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
> --- 99,122 ----
> TupleTableSlot *
> ExecForeignScan(ForeignScanState *node)
> {
> + EState *estate = node->ss.ps.state;
> +
> + if (estate->es_epqTuple != NULL)
> + {
> + /*
> + * We are inside an EvalPlanQual recheck. If foreign join, get next
> + * tuple from subplan.
> + */
> + Index scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
> +
> + if (scanrelid == 0)
> + {
> + PlanState *outerPlan = outerPlanState(node);
> +
> + return ExecProcNode(outerPlan);
> + }
> + }
> +
> return ExecScan((ScanState *) node,
> (ExecScanAccessMtd) ForeignNext,
> (ExecScanRecheckMtd) ForeignRecheck);
>
> It might not be specific to RDBMS, however, we cannot guarantee all the FDW are
> comfortable to run the alternative plan node on EPQ recheck.
> This design does not allow FDW drivers to implement own EPQ recheck, possibly
> more efficient than built-in logic.

I'm not convinced that this problem is more than hypothetical. EPQ
rechecks should be quite rare, so it shouldn't really matter if we
jump through a few extra hoops when they happen. And really, are
those hoops all that expensive? It's not as if ExecInitNode should be
doing any sort of expensive operation, or ExecEndScan either. And
they will be able to tell if they're being called for an EPQ-recheck
by fishing out the estate, so if there's some processing that they
want to short-circuit for that case, they can. So I'm not seeing the
problem. Do you have any evidence that either the performance cost or
the code complexity cost is significant for PG-Strom or any other
extension?

That having been said, I don't entirely like Fujita-san's patch
either. Much of the new code is called immediately adjacent to an FDW
callback which could pretty trivially do the same thing itself, if
needed. And much of it is contingent on whether estate->es_epqTuple
!= NULL and scanrelid == 0, but perhaps out would be better to check
whether the subplan is actually present instead of checking whether we
think it should be present. Also, the naming is a bit weird:
node->fs_subplan gets shoved into outerPlanState(), which seems like a
kludge.

I'm wondering if there's another approach. If I understand correctly,
there are two reasons why the current situation is untenable. The
first is that ForeignRecheck always returns true, but we could instead
call an FDW-supplied callback routine there. The callback could be
optional, so that we just return true if there is none, which is nice
for already-existing FDWs that then don't need to do anything. The
second is that ExecScanFetch needs scanrelid > 0 so that
estate->es_epqTupleSet[scanrelid - 1] isn't indexing off the beginning
of the array, and similarly estate->es_epqScanDone[scanrelid - 1] and
estate->es_epqTuple[scanrelid - 1]. But, waving my hands wildly, that
also seems like a solvable problem. I mean, we're joining a non-empty
set of relations, so the entries in the EPQ-related arrays for those
RTIs are not getting used for anything, so we can use any of them for
the joinrel. We need some way for this code to decide what RTI to
use, but that shouldn't be too hard to finagle.

Thoughts?

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

On 2015/09/03 9:41, Robert Haas wrote:
> That having been said, I don't entirely like Fujita-san's patch
> either. Much of the new code is called immediately adjacent to an FDW
> callback which could pretty trivially do the same thing itself, if
> needed.

Another idea about that code is to call that code in eg, ExecProcNode,
instead of calling ExecForeignScan there. I think that that might be
much cleaner and resolve the naming problem below.

> And much of it is contingent on whether estate->es_epqTuple
> != NULL and scanrelid == 0, but perhaps out would be better to check
> whether the subplan is actually present instead of checking whether we
> think it should be present.

Agreed with this.

> Also, the naming is a bit weird:
> node->fs_subplan gets shoved into outerPlanState(), which seems like a
> kludge.

And with this. Proposals welcome.

> I'm wondering if there's another approach. If I understand correctly,
> there are two reasons why the current situation is untenable. The
> first is that ForeignRecheck always returns true, but we could instead
> call an FDW-supplied callback routine there. The callback could be
> optional, so that we just return true if there is none, which is nice
> for already-existing FDWs that then don't need to do anything.

My question about this is, is the callback really needed? If there are
any FDWs that want to do the work *in their own way*, instead of just
doing ExecProcNode for executing a local join execution plan in case of
foreign join (or just doing ExecQual for checking remote quals in case
of foreign table), I'd agree with introducing the callback, but if not,
I don't think that that makes much sense.

Best regards,
Etsuro Fujita

On 2015/09/03 14:22, Etsuro Fujita wrote:
> On 2015/09/03 9:41, Robert Haas wrote:
>> That having been said, I don't entirely like Fujita-san's patch
>> either. Much of the new code is called immediately adjacent to an FDW
>> callback which could pretty trivially do the same thing itself, if
>> needed.

> Another idea about that code is to call that code in eg, ExecProcNode,
> instead of calling ExecForeignScan there. I think that that might be
> much cleaner and resolve the naming problem below.

I gave it another thought; the following changes to ExecInitNode would
make the patch much simpler, ie, we would no longer need to call the new
code in ExecInitForeignScan, ExecForeignScan, ExecEndForeignScan, and
ExecReScanForeignScan. I think that would resolve the name problem also.

*** a/src/backend/executor/execProcnode.c
--- b/src/backend/executor/execProcnode.c
***************
*** 247,254 **** ExecInitNode(Plan *node, EState *estate, int eflags)
break;

case T_ForeignScan:
! result = (PlanState *) ExecInitForeignScan((ForeignScan *) node,
! estate, eflags);
break;

case T_CustomScan:
--- 247,269 ----
break;

case T_ForeignScan:
! {
! Index scanrelid = ((ForeignScan *)
node)->scan.scanrelid;
!
! if (estate->es_epqTuple != NULL && scanrelid == 0)
! {
! /*
! * We are in foreign join inside an EvalPlanQual
recheck.
! * Initialize local join execution plan, instead.
! */
! Plan *subplan = ((ForeignScan *)
node)->fs_subplan;
!
! result = ExecInitNode(subplan, estate, eflags);
! }
! else
! result = (PlanState *)
ExecInitForeignScan((ForeignScan *) node,
! estate,
eflags);
! }
break;

Best regards,
Etsuro Fujita

> , On Wed, Aug 26, 2015 at 4:05 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> >> On 2015/08/26 16:07, Kouhei Kaigai wrote:
> >> I wrote:
> >> >> Maybe I'm missing something, but why do we need such a flexiblity for
> >> >> the columnar-stores?
> >>
> >> > Even if we enforce them a new interface specification comfortable to RDBMS,
> >> > we cannot guarantee it is also comfortable to other type of FDW drivers.
> >>
> >> Specifically, what kind of points about the patch are specific to RDBMS?
> >>
> >
> > *** 88,93 **** ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
> > --- 99,122 ----
> > TupleTableSlot *
> > ExecForeignScan(ForeignScanState *node)
> > {
> > + EState *estate = node->ss.ps.state;
> > +
> > + if (estate->es_epqTuple != NULL)
> > + {
> > + /*
> > + * We are inside an EvalPlanQual recheck. If foreign join,
> get next
> > + * tuple from subplan.
> > + */
> > + Index scanrelid = ((Scan *)
> node->ss.ps.plan)->scanrelid;
> > +
> > + if (scanrelid == 0)
> > + {
> > + PlanState *outerPlan = outerPlanState(node);
> > +
> > + return ExecProcNode(outerPlan);
> > + }
> > + }
> > +
> > return ExecScan((ScanState *) node,
> > (ExecScanAccessMtd) ForeignNext,
> > (ExecScanRecheckMtd)
> ForeignRecheck);
> >
> > It might not be specific to RDBMS, however, we cannot guarantee all the FDW
> are
> > comfortable to run the alternative plan node on EPQ recheck.
> > This design does not allow FDW drivers to implement own EPQ recheck, possibly
> > more efficient than built-in logic.
>
> I'm not convinced that this problem is more than hypothetical. EPQ
> rechecks should be quite rare, so it shouldn't really matter if we
> jump through a few extra hoops when they happen. And really, are
> those hoops all that expensive? It's not as if ExecInitNode should be
> doing any sort of expensive operation, or ExecEndScan either. And
> they will be able to tell if they're being called for an EPQ-recheck
> by fishing out the estate, so if there's some processing that they
> want to short-circuit for that case, they can. So I'm not seeing the
> problem. Do you have any evidence that either the performance cost or
> the code complexity cost is significant for PG-Strom or any other
> extension?
>
Even though PG-Strom does not implement EPQ recheck mechanism yet
(and not implemented on top of FDW), I plan to re-use CPU fallback
mechanism (*1) rather than having alternative plan approach.
I also don't care about performance penalty, however, don't want to
have alternative plan because of code complexity.
I don't deny individual extensions have alternative path by their
decision, but should not be enforced.

(*1) GPU often cannot execute expression because of exceptional
data like very long numeric or external toast etc..., but to be
executable. In this case, PG-Strom evaluates this expression in
the CPU side (of course, it is worse than normal execution path
but better than error). This logic is almost same as what we need
on EPQ recheck.

> I'm wondering if there's another approach. If I understand correctly,
> there are two reasons why the current situation is untenable. The
> first is that ForeignRecheck always returns true, but we could instead
> call an FDW-supplied callback routine there. The callback could be
> optional, so that we just return true if there is none, which is nice
> for already-existing FDWs that then don't need to do anything. The
> second is that ExecScanFetch needs scanrelid > 0 so that
> estate->es_epqTupleSet[scanrelid - 1] isn't indexing off the beginning
> of the array, and similarly estate->es_epqScanDone[scanrelid - 1] and
> estate->es_epqTuple[scanrelid - 1]. But, waving my hands wildly, that
> also seems like a solvable problem. I mean, we're joining a non-empty
> set of relations, so the entries in the EPQ-related arrays for those
> RTIs are not getting used for anything, so we can use any of them for
> the joinrel. We need some way for this code to decide what RTI to
> use, but that shouldn't be too hard to finagle.
>
ForeignScan->fs_relids and CustomScan->custom_relids know which RTIs
shall be involved in this joinrel.

However, only extension know how these relations (including the case
of N-way join) shall be joined. FDW drivers may keep joinrestrictinfo
in their comfortable way, like a compiled GPU native binary, so I don't
think core side can do something relevant reasonably.
Even though Fujita-san proposed a new special fields in ForeignScan
to attach expression node that was pushed down, however, it looks to
me interface contract makes more complicated. Rather than various
special purpose fields, it is more straightforward to call back
extension when scanrelid==0. We can provide equivalent feature as
a utility function that has capability Fujita-san wants.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Wed, Sep 2, 2015 at 1:47 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Sep 2, 2015 at 10:30 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> But if you have in mind that typical FDWs would actually create join paths
>>> at that point, consider that
>>>
>>> 1. The FDW would have to find all the combinations of its supplied
>>> relations (unless you are only intending to generate one path for the
>>> union of all such rels, which seems pretty narrow-minded from here).
>
>> Well, if the remote end is another database server, presumably we can
>> leave it to optimize the query, so why would we need more than one
>> path?
>
> If you have say 5 relations in the query, 3 of which are foreign, it might
> make sense to join all 3 at the remote end, or maybe you should only join
> 2 of them remotely because it's better to then join to one of the local
> rels before joining the last remote rel.

True. But that's not the problem I'm concerned about. Suppose the
query looks like this:

SELECT * FROM ft1 LEFT JOIN ft2 ON ft1.x = ft2.x LEFT JOIN t1 ON ft2.y
= t1.y LEFT JOIN ft3 ON ft1.z = ft3.z LEFT JOIN t2 ON ft1.w = t2.w;

Now, no matter where we put the hooks, we'll consider foreign join
paths for all of the various combinations of tables that we could push
down. We'll decide between those various options based on cost, which
is fine. But let's consider just one joinrel, the one that includes
(ft1 ft2 ft3). Assuming that the remote tables have the same name as
the local tables. The path that implements a pushed-down join of all
three tables will send one of these two queries to the remote server:

SELECT * FROM ft1 LEFT JOIN ft2 ON ft1.x = ft2.x LEFT JOIN ft3 ON ft1.z = ft3.z;
SELECT * FROM ft1 LEFT JOIN ft3 ON ft1.z = ft3.z LEFT JOIN ft2 ON
ft1.x = ft2.x ;

We need to generate one of those two queries, and we need to figure
out what the remote server thinks it will cost to execute. We
presumably do not to cost both of them, because if it's legal to
commute the joins, the remote server can and will do that itself. It
would be stupid to cost both possible queries if the remote server is
going to pick the same plan either way. However - and this is the key
point - the one we choose to generate *must represent a legal join
order*. If the ft1-ft2 join were a FULL JOIN instead of a LEFT JOIN,
the second query wouldn't be a legal thing to send to the remote
server. So, the problem I'm worried about is: given that we know we
want to at least consider the path that pushes the whole join to the
remote server, how do we construct an SQL query that embodies a legal
join order of the relations being pushed down?

> Even if you claim that that
> would never make sense from a cost standpoint (a claim easily seen to be
> silly), there might not be any legal way to join all 3 directly because of
> join order constraints.
>
> The larger point is that we can't expect the remote server to be fully
> responsible for optimizing, because it will know nothing of what's being
> done on our end.

No argument with any of that.

>> So, the problem is that I don't think this entirely skirts the
>> join_is_legal issues, which are a principal point of concern for me.
>> Say this is a joinrel between (A B) and (C D E). We need to generate
>> an SQL query for (A B C D E). We know that the outermost syntactic
>> join can be (A B) to (C D E). But how do we know which join orders
>> are legal as among (C D E)? Maybe there's a simple way to handle this
>> that I'm not seeing.
>
> Well, if the joins get built up in the way I think should happen, we'd
> have already considered (C D E), and we could have recorded the legal join
> orders within that at the time. (I imagine that we should allow FDWs to
> store some data within RelOptInfo structs that represent foreign joins
> belonging entirely to them, so that there'd be a handy place to keep that
> data till later.)

Yes, that would help. Can fdw_private serve that purpose, or do we
need something else?

> Or we could trawl through the paths associated with the
> child joinrel, which will presumably include instances of every reasonable
> sub-join combination. Or the FDW could look at the SpecialJoinInfo data
> and determine things for itself (or more likely, ask join_is_legal about
> that).

Yeah, this is the part I'm worried will be complex, which accounts for
the current hook placement. I'm worried that trawling through that
SpecialJoinInfo data will end up needing to duplicate much of
make_join_rel and add_paths_to_joinrel. For example, consider:

SELECT * FROM verysmall v JOIN (bigft1 FULL JOIN bigft2 ON bigft1.x =
bigft2.x) ON v.q = bigft1.q AND v.r = bigft2.r;

The best path for this plan is presumably something like this:

Nested Loop
-> Seq Scan on verysmall v
-> Foreign Scan on bigft1 and bigft2
Remote SQL: SELECT * FROM bigft1 FULL JOIN bigft2 ON bigft1.x =
bigft2.x AND bigft1.q = $1 AND bigft2.r = $2

Now, how is the FDW going to figure out that it needs to generate this
parameterized path without duplicating this code from
add_paths_to_joinrel?

/*
* Decide whether it's sensible to generate parameterized paths for this
* joinrel, and if so, which relations such paths should require. There
* is usually no need to create a parameterized result path unless there
...

Maybe there's a very simple answer to this question and I'm just not
seeing it, but I really don't see how that's going to work.

> In the case of postgres_fdw, I think the actual requirement will be to be
> able to reconstruct a SQL query that correctly expresses the join; that
> is, we need to send over something like "from c left join d on (...) full
> join e on (...)", not just "from c, d, e", or we'll get totally bogus
> estimates as well as bogus execution results.

Agreed.

> Offhand I think that the
> most likely way to build that text will be to examine the query's jointree
> to see where c,d,e appear in it. But in any case, that's a separate issue
> and I fail to see how plopping the join search problem into the FDW's lap
> would make it any easier.

Yeah, I am not advocating for putting the hook in
standard_join_search. I'm explaining why I put it in
add_paths_to_joinrel instead of, as I believe you were advocating, in
make_join_rel prior to the big switch.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Sep 2, 2015 at 1:47 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Offhand I think that the
>> most likely way to build that text will be to examine the query's jointree
>> to see where c,d,e appear in it. But in any case, that's a separate issue
>> and I fail to see how plopping the join search problem into the FDW's lap
>> would make it any easier.

> Yeah, I am not advocating for putting the hook in
> standard_join_search. I'm explaining why I put it in
> add_paths_to_joinrel instead of, as I believe you were advocating, in
> make_join_rel prior to the big switch.

If you had a solution to the how-to-build-the-query-text problem,
and it depended on that hook placement, then your argument might
make some sense. As is, you've entirely failed to convince me
that this placement is not wrong, wasteful, and likely to create
unnecessary API breaks for FDWs.

(Also, per my last message on the subject, *after* the switch
is what I think makes sense.)

regards, tom lane

On 2015/09/03 19:25, Etsuro Fujita wrote:
> On 2015/09/03 14:22, Etsuro Fujita wrote:
>> On 2015/09/03 9:41, Robert Haas wrote:
>>> That having been said, I don't entirely like Fujita-san's patch
>>> either. Much of the new code is called immediately adjacent to an FDW
>>> callback which could pretty trivially do the same thing itself, if
>>> needed.

>> Another idea about that code is to call that code in eg, ExecProcNode,
>> instead of calling ExecForeignScan there. I think that that might be
>> much cleaner and resolve the naming problem below.

> I gave it another thought; the following changes to ExecInitNode would
> make the patch much simpler, ie, we would no longer need to call the new
> code in ExecInitForeignScan, ExecForeignScan, ExecEndForeignScan, and
> ExecReScanForeignScan. I think that would resolve the name problem also.

I'm attaching an updated version of the patch. The patch is based on
the SS_finalize_plan patch that has been recently committed. I'd be
happy if this helps people discuss more about how to fix this issue.

Best regards,
Etsuro Fujita

On 2015/09/04 19:50, Etsuro Fujita wrote:
> I'm attaching an updated version of the patch. The patch is based on
> the SS_finalize_plan patch that has been recently committed. I'd be
> happy if this helps people discuss more about how to fix this issue.

In the updated version, I modified finalize_plan so that initPlans
attached to a ForeignScan node doing a remote join are considered for
the computed params for a local join plan for EvalPlanQual testing. But
I noticed no need for that. The reason is, no initPlans will be
attached to the ForeignScan node due to that the ForeignScan node is
unable to be the topmost plan node for the query level in case of
EvalPlanQual testing. So, I removed that code. Patch attached. (That
no longer depends on the SS_finalize_plan patch.)

Best regards,
Etsuro Fujita

Hello, sorry in advance for possible brought up of past
discussions or pointless discussion.

> I'm attaching an updated version of the patch. The patch is based on
> the SS_finalize_plan patch that has been recently committed. I'd be
> happy if this helps people discuss more about how to fix this issue.

The two patches make a good contrast to clarify the problem for
me, maybe.

> > code in ExecInitForeignScan, ExecForeignScan, ExecEndForeignScan, and
> > ExecReScanForeignScan. I think that would resolve the name problem
> > also.

I found two points in this discussion.

1. Where (or When) to initialize a foreign/custom scan node for
recheck.

Having a new list to hold substitute plans in planner global
(and PlannedStmt) is added, EvalPlanQualStart() looks to be
the best place to initialize them.

Of couse it could not be a solution unless the new member and
related code are not acceptable or rather unreasonable. The
possible timing left for the case would be ExecInitNode() (as
v2.0) or FDW routines (as v1.0).

2. How the core informs fdw/custom scan handlers wheter it is
during recheck.

In v1.0 patch, nodeForignscan.c routines detect the situation
using es_epqTuple and Scan.scanrelid which the core as is
gives, and v2.0 alternatively replaces scan node implicitly
(and maybe irregularly) itself on initialization. The latter
don't looks to me tidy.

I think refining v1.0 would be more desirable, and resolving
the redundancy would be simply a matter of notation.

If I understand there correctly, Exec*ForeignScan() other than
ExecInitForeignScan() can determine the behavior simply
looking outerPlanState(scanstate). (If we continue to use the
member lefttree for the purpose..). Is it right? and does it
eliminate the redundancy?

ExecEndForeignScan()
{
if ((outerplan = outerPlanState(node)) != NULL)
ExecEndNode(outerPlan);
...

regards,

At Fri, 04 Sep 2015 19:50:46 +0900, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <55E97786(dot)30404(at)lab(dot)ntt(dot)co(dot)jp>
> On 2015/09/03 19:25, Etsuro Fujita wrote:
> > On 2015/09/03 14:22, Etsuro Fujita wrote:
> >> On 2015/09/03 9:41, Robert Haas wrote:
> >>> That having been said, I don't entirely like Fujita-san's patch
> >>> either. Much of the new code is called immediately adjacent to an FDW
> >>> callback which could pretty trivially do the same thing itself, if
> >>> needed.
...
> > I gave it another thought; the following changes to ExecInitNode would
> > make the patch much simpler, ie, we would no longer need to call the
> > new
> > code in ExecInitForeignScan, ExecForeignScan, ExecEndForeignScan, and
> > ExecReScanForeignScan. I think that would resolve the name problem
> > also.
>
> I'm attaching an updated version of the patch. The patch is based on
> the SS_finalize_plan patch that has been recently committed. I'd be
> happy if this helps people discuss more about how to fix this issue.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2015/09/02 23:30, Tom Lane wrote:
> Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
>> On 2015/08/01 23:25, Tom Lane wrote:
>>> In my vision you probably really only want one call per build_join_rel
>>> event (that is, per construction of a new RelOptInfo), not per
>>> make_join_rel event.

>> I'd vote for hooking at standard_join_search.

> I think that method would require the FDW to duplicate a whole lot of the
> join search mechanism, for not a whole lot of benefit. It's possible that
> there'd be value in doing some initial reconnaissance once we've examined
> all the baserels, so I'm not necessarily against providing a hook there.
> But if you have in mind that typical FDWs would actually create join paths
> at that point, consider that
>
> 1. The FDW would have to find all the combinations of its supplied
> relations (unless you are only intending to generate one path for the
> union of all such rels, which seems pretty narrow-minded from here).
>
> 2. The FDW would have to account for join_is_legal considerations.
>
> 3. The FDW would have to arrange for creation of joinrel RelOptInfo
> structures. While that's possible, the available infrastructure for it
> assumes that joinrels are built up from pairs of simpler joinrels, so
> you couldn't go directly to the union of all the FDW's rels anyway.

Maybe my explanation was not correct, but the hook placement I think is
just before the set_cheapest call for each joinrel in
standard_join_search, as you proposed in [1]. And I think that if that
joinrel contains only foreign tables that all belong to the same foreign
server, then we give the FDW a chance to consider producing pushed-down
joins for that joinrel, ie, remote joins for all the foreign tables
contained in that joinrel. So, there is no need for #2 and #3. Also I
think that would allow us to consider producing pushed-down joins for
all the legal combinations of foreign tables that belong to the same
foreign server, according to the dynamic-programming method, in
principle. I've not had a solution to the how-to-build-the-query-text
problem yet, though.

Best regards,
Etsuro Fujita

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

On 2015/09/04 0:33, Robert Haas wrote:
> I'm worried that trawling through that
> SpecialJoinInfo data will end up needing to duplicate much of
> make_join_rel and add_paths_to_joinrel. For example, consider:
>
> SELECT * FROM verysmall v JOIN (bigft1 FULL JOIN bigft2 ON bigft1.x =
> bigft2.x) ON v.q = bigft1.q AND v.r = bigft2.r;
>
> The best path for this plan is presumably something like this:
>
> Nested Loop
> -> Seq Scan on verysmall v
> -> Foreign Scan on bigft1 and bigft2
> Remote SQL: SELECT * FROM bigft1 FULL JOIN bigft2 ON bigft1.x =
> bigft2.x AND bigft1.q = $1 AND bigft2.r = $2
>
> Now, how is the FDW going to figure out that it needs to generate this
> parameterized path without duplicating this code from
> add_paths_to_joinrel?
>
> /*
> * Decide whether it's sensible to generate parameterized paths for this
> * joinrel, and if so, which relations such paths should require. There
> * is usually no need to create a parameterized result path unless there
> ...
>
> Maybe there's a very simple answer to this question and I'm just not
> seeing it, but I really don't see how that's going to work.

Why don't you look at the "regular" (local join execution) paths that
were already generated. I think that if we called the FDW at a proper
hook location, the FDW could probably find a regular path in
rel->pathlist of the join rel (bigft1, bigft2) that possibly generates
something like:

Nested Loop
-> Seq Scan on verysmall v
-> Nested Loop
Join Filter: (bigft1.a = bigft2.a)
-> Foreign Scan on bigft1
Remote SQL: SELECT * FROM bigft1 WHERE bigft1.q = $1
-> Foreign Scan on bigft2
Remote SQL: SELECT * FROM bigft2 WHERE bigft2.r = $2

From the parameterization of the regular nestloop path for joining
bigft1 and bigft2 locally, I think that the FDW could find that it's
sensible to generate the foreign-join path for (bigft1, bigft2) with the
parameterization.

Best regards,
Etsuro Fujita

On Tue, Sep 8, 2015 at 5:35 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2015/09/04 0:33, Robert Haas wrote:
>> I'm worried that trawling through that
>> SpecialJoinInfo data will end up needing to duplicate much of
>> make_join_rel and add_paths_to_joinrel. For example, consider:
>>
>> SELECT * FROM verysmall v JOIN (bigft1 FULL JOIN bigft2 ON bigft1.x =
>> bigft2.x) ON v.q = bigft1.q AND v.r = bigft2.r;
>>
>> The best path for this plan is presumably something like this:
>>
>> Nested Loop
>> -> Seq Scan on verysmall v
>> -> Foreign Scan on bigft1 and bigft2
>> Remote SQL: SELECT * FROM bigft1 FULL JOIN bigft2 ON bigft1.x =
>> bigft2.x AND bigft1.q = $1 AND bigft2.r = $2
>>
>> Now, how is the FDW going to figure out that it needs to generate this
>> parameterized path without duplicating this code from
>> add_paths_to_joinrel?
>>
>> /*
>> * Decide whether it's sensible to generate parameterized paths for
>> this
>> * joinrel, and if so, which relations such paths should require.
>> There
>> * is usually no need to create a parameterized result path unless
>> there
>> ...
>>
>> Maybe there's a very simple answer to this question and I'm just not
>> seeing it, but I really don't see how that's going to work.
>
>
> Why don't you look at the "regular" (local join execution) paths that were
> already generated. I think that if we called the FDW at a proper hook
> location, the FDW could probably find a regular path in rel->pathlist of the
> join rel (bigft1, bigft2) that possibly generates something like:
>
> Nested Loop
> -> Seq Scan on verysmall v
> -> Nested Loop
> Join Filter: (bigft1.a = bigft2.a)
> -> Foreign Scan on bigft1
> Remote SQL: SELECT * FROM bigft1 WHERE bigft1.q = $1
> -> Foreign Scan on bigft2
> Remote SQL: SELECT * FROM bigft2 WHERE bigft2.r = $2
>
> From the parameterization of the regular nestloop path for joining bigft1
> and bigft2 locally, I think that the FDW could find that it's sensible to
> generate the foreign-join path for (bigft1, bigft2) with the
> parameterization.

But that path might have already been discarded on the basis of cost.
I think Tom's idea is better: let the FDW consult some state cached
for this purpose in the RelOptInfo.

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

On Thu, Sep 3, 2015 at 11:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Sep 2, 2015 at 1:47 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Offhand I think that the
>>> most likely way to build that text will be to examine the query's jointree
>>> to see where c,d,e appear in it. But in any case, that's a separate issue
>>> and I fail to see how plopping the join search problem into the FDW's lap
>>> would make it any easier.
>
>> Yeah, I am not advocating for putting the hook in
>> standard_join_search. I'm explaining why I put it in
>> add_paths_to_joinrel instead of, as I believe you were advocating, in
>> make_join_rel prior to the big switch.
>
> If you had a solution to the how-to-build-the-query-text problem,
> and it depended on that hook placement, then your argument might
> make some sense. As is, you've entirely failed to convince me
> that this placement is not wrong, wasteful, and likely to create
> unnecessary API breaks for FDWs.
>
> (Also, per my last message on the subject, *after* the switch
> is what I think makes sense.)

After re-reading a few emails, I've realized that I've let myself get
a bit confused here and have unwittingly switched sides in this
argument. <puts brown paper bag over head>

When we originally discussed this back in April, I was arguing for
either make_join_rel() or add_paths_to_joinrel() and you were arguing
for standard_join_search(). See here:

http://www.postgresql.org/message-id/CA+TgmobOADxTbsCt-j+dDVefWGK1WxY4p8AVDp1Pz48_TX4XTA@mail.gmail.com

I thought we were still having the same argument, but we're not.
You're now arguing for make_one_rel(), which back then was perfectly
acceptable to me, and now that I've gotten by thinking un-fuzzed,
really still is, except for the question posed in the closing
paragraph of that email, which is (mostly) whether clients like
postgres_fdw are going to need extra_lateral_rels in order to do the
right thing.

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

On 2015/09/09 3:53, Robert Haas wrote:
> On Tue, Sep 8, 2015 at 5:35 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> On 2015/09/04 0:33, Robert Haas wrote:
>>> I'm worried that trawling through that
>>> SpecialJoinInfo data will end up needing to duplicate much of
>>> make_join_rel and add_paths_to_joinrel. For example, consider:
>>>
>>> SELECT * FROM verysmall v JOIN (bigft1 FULL JOIN bigft2 ON bigft1.x =
>>> bigft2.x) ON v.q = bigft1.q AND v.r = bigft2.r;
>>>
>>> The best path for this plan is presumably something like this:
>>>
>>> Nested Loop
>>> -> Seq Scan on verysmall v
>>> -> Foreign Scan on bigft1 and bigft2
>>> Remote SQL: SELECT * FROM bigft1 FULL JOIN bigft2 ON bigft1.x =
>>> bigft2.x AND bigft1.q = $1 AND bigft2.r = $2
>>>
>>> Now, how is the FDW going to figure out that it needs to generate this
>>> parameterized path without duplicating this code from
>>> add_paths_to_joinrel?
>>>
>>> /*
>>> * Decide whether it's sensible to generate parameterized paths for
>>> this
>>> * joinrel, and if so, which relations such paths should require.
>>> There
>>> * is usually no need to create a parameterized result path unless
>>> there
>>> ...
>>>
>>> Maybe there's a very simple answer to this question and I'm just not
>>> seeing it, but I really don't see how that's going to work.

>> Why don't you look at the "regular" (local join execution) paths that were
>> already generated. I think that if we called the FDW at a proper hook
>> location, the FDW could probably find a regular path in rel->pathlist of the
>> join rel (bigft1, bigft2) that possibly generates something like:
>>
>> Nested Loop
>> -> Seq Scan on verysmall v
>> -> Nested Loop
>> Join Filter: (bigft1.a = bigft2.a)
>> -> Foreign Scan on bigft1
>> Remote SQL: SELECT * FROM bigft1 WHERE bigft1.q = $1
>> -> Foreign Scan on bigft2
>> Remote SQL: SELECT * FROM bigft2 WHERE bigft2.r = $2
>>
>> From the parameterization of the regular nestloop path for joining bigft1
>> and bigft2 locally, I think that the FDW could find that it's sensible to
>> generate the foreign-join path for (bigft1, bigft2) with the
>> parameterization.

> But that path might have already been discarded on the basis of cost.
> I think Tom's idea is better: let the FDW consult some state cached
> for this purpose in the RelOptInfo.

Do you have an idea of what information would be collected into the
state and how the FDW would derive parameterizations to consider
producing pushed-down joins with from that information? What I'm
concerned about that is to reduce the number of parameterizations to
consider, to reduce overhead in costing the corresponding queries. I'm
missing something, though.

Best regards,
Etsuro Fujita

On Thu, Sep 3, 2015 at 6:25 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> I gave it another thought; the following changes to ExecInitNode would make
> the patch much simpler, ie, we would no longer need to call the new code in
> ExecInitForeignScan, ExecForeignScan, ExecEndForeignScan, and
> ExecReScanForeignScan. I think that would resolve the name problem also.
>
> *** a/src/backend/executor/execProcnode.c
> --- b/src/backend/executor/execProcnode.c
> ***************
> *** 247,254 **** ExecInitNode(Plan *node, EState *estate, int eflags)
> break;
>
> case T_ForeignScan:
> ! result = (PlanState *) ExecInitForeignScan((ForeignScan *) node,
> ! estate, eflags);
> break;
>
> case T_CustomScan:
> --- 247,269 ----
> break;
>
> case T_ForeignScan:
> ! {
> ! Index scanrelid = ((ForeignScan *)
> node)->scan.scanrelid;
> !
> ! if (estate->es_epqTuple != NULL && scanrelid == 0)
> ! {
> ! /*
> ! * We are in foreign join inside an EvalPlanQual
> recheck.
> ! * Initialize local join execution plan, instead.
> ! */
> ! Plan *subplan = ((ForeignScan *)
> node)->fs_subplan;
> !
> ! result = ExecInitNode(subplan, estate, eflags);
> ! }
> ! else
> ! result = (PlanState *) ExecInitForeignScan((ForeignScan
> *) node,
> ! estate,
> eflags);
> ! }
> break;

I don't think that's a good idea. The Plan tree and the PlanState
tree should be mirror images of each other; breaking that equivalence
will cause confusion, at least.

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

On Thu, Sep 3, 2015 at 1:22 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> I'm wondering if there's another approach. If I understand correctly,
>> there are two reasons why the current situation is untenable. The
>> first is that ForeignRecheck always returns true, but we could instead
>> call an FDW-supplied callback routine there. The callback could be
>> optional, so that we just return true if there is none, which is nice
>> for already-existing FDWs that then don't need to do anything.
>
> My question about this is, is the callback really needed? If there are any
> FDWs that want to do the work *in their own way*, instead of just doing
> ExecProcNode for executing a local join execution plan in case of foreign
> join (or just doing ExecQual for checking remote quals in case of foreign
> table), I'd agree with introducing the callback, but if not, I don't think
> that that makes much sense.

It doesn't seem to me that it hurts much of anything to add the
callback there, and it does provide some flexibility. Actually, I'm
not really sure why we're thinking we need a subplan here at all,
rather than just having a ForeignRecheck callback that can do whatever
it needs to do with no particular help from the core infrastructure.
I think you wrote some code to show how postgres_fdw would use the API
you are proposing, but I can't find it. Can you point me in the right
direction?

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

On Wed, Sep 9, 2015 at 2:30 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> But that path might have already been discarded on the basis of cost.
>> I think Tom's idea is better: let the FDW consult some state cached
>> for this purpose in the RelOptInfo.
>
> Do you have an idea of what information would be collected into the state
> and how the FDW would derive parameterizations to consider producing
> pushed-down joins with from that information? What I'm concerned about that
> is to reduce the number of parameterizations to consider, to reduce overhead
> in costing the corresponding queries. I'm missing something, though.

I think the thing we'd want to store in the state would be enough
information to reconstruct a valid join nest. For example, the
reloptinfo for (A B) might note that A needs to be left-joined to B.
When we go to construct paths for (A B C), and there is no
SpecialJoinInfo that mentions C, we know that we can construct (A LJ
B) IJ C rather than (A IJ B) IJ C. If any paths survived, we could
find a way to pull that information out of the path, but pulling it
out of the RelOptInfo should always work.

I am not sure what to do about parameterizations. That's one of my
remaining concerns about moving the hook.

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

On 2015/09/11 6:24, Robert Haas wrote:
> On Thu, Sep 3, 2015 at 1:22 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> I'm wondering if there's another approach. If I understand correctly,
>>> there are two reasons why the current situation is untenable. The
>>> first is that ForeignRecheck always returns true, but we could instead
>>> call an FDW-supplied callback routine there. The callback could be
>>> optional, so that we just return true if there is none, which is nice
>>> for already-existing FDWs that then don't need to do anything.
>>
>> My question about this is, is the callback really needed? If there are any
>> FDWs that want to do the work *in their own way*, instead of just doing
>> ExecProcNode for executing a local join execution plan in case of foreign
>> join (or just doing ExecQual for checking remote quals in case of foreign
>> table), I'd agree with introducing the callback, but if not, I don't think
>> that that makes much sense.
>
> It doesn't seem to me that it hurts much of anything to add the
> callback there, and it does provide some flexibility. Actually, I'm
> not really sure why we're thinking we need a subplan here at all,
> rather than just having a ForeignRecheck callback that can do whatever
> it needs to do with no particular help from the core infrastructure.
> I think you wrote some code to show how postgres_fdw would use the API
> you are proposing, but I can't find it. Can you point me in the right
> direction?

I've proposed the following API changes:

* I modified create_foreignscan_path, which is called from
postgresGetForeignJoinPaths/postgresGetForeignPaths, so that a path,
subpath, is passed as the eighth argument of the function. subpath
represents a local join execution path if scanrelid==0, but NULL if
scanrelid>0.

* I modified make_foreignscan, which is called from
postgresGetForeignPlan, so that a list of quals, fdw_quals, is passed as
the last argument of the function. fdw_quals represents remote quals if
scanrelid>0, but NIL if scanrelid==0.

You can find that code in the postgres_fdw patch
(foreign_join_v16_efujita.patch) attached to [1].

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/55CB2D45.7040100@lab.ntt.co.jp

Hello,

At Thu, 10 Sep 2015 17:24:00 -0400, Robert Haas <robertmhaas(at)gmail(dot)com> wrote in <CA+TgmobxksR2=3wEdY5cEgpd1hQ6Z0WoZEBBoxgs=XKZpbfUXA(at)mail(dot)gmail(dot)com>
> On Thu, Sep 3, 2015 at 1:22 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >> I'm wondering if there's another approach. If I understand correctly,
> >> there are two reasons why the current situation is untenable. The
> >> first is that ForeignRecheck always returns true, but we could instead
> >> call an FDW-supplied callback routine there. The callback could be
> >> optional, so that we just return true if there is none, which is nice
> >> for already-existing FDWs that then don't need to do anything.
> >
> > My question about this is, is the callback really needed? If there are any
> > FDWs that want to do the work *in their own way*, instead of just doing
> > ExecProcNode for executing a local join execution plan in case of foreign
> > join (or just doing ExecQual for checking remote quals in case of foreign
> > table), I'd agree with introducing the callback, but if not, I don't think
> > that that makes much sense.
>
> It doesn't seem to me that it hurts much of anything to add the
> callback there, and it does provide some flexibility. Actually, I'm
> not really sure why we're thinking we need a subplan here at all,
> rather than just having a ForeignRecheck callback that can do whatever
> it needs to do with no particular help from the core infrastructure.
> I think you wrote some code to show how postgres_fdw would use the API
> you are proposing, but I can't find it. Can you point me in the right
> direction?

I've heard that the reason for the (fs_)subplan is that it should
be initialized using create_plan_recurse, set_plan_refs and
finalyze_plan (or others), which are static functions in the
planner, unavailable in fdw code.

Is this pointless?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Sorry, that's quite wrong.. Please let me fix it.

- Is this pointless?
+ Does it make sense?

=====
Hello,

At Thu, 10 Sep 2015 17:24:00 -0400, Robert Haas <robertmhaas(at)gmail(dot)com> wrote in <CA+TgmobxksR2=3wEdY5cEgpd1hQ6Z0WoZEBBoxgs=XKZpbfUXA(at)mail(dot)gmail(dot)com>
> On Thu, Sep 3, 2015 at 1:22 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >> I'm wondering if there's another approach. If I understand correctly,
> >> there are two reasons why the current situation is untenable. The
> >> first is that ForeignRecheck always returns true, but we could instead
> >> call an FDW-supplied callback routine there. The callback could be
> >> optional, so that we just return true if there is none, which is nice
> >> for already-existing FDWs that then don't need to do anything.
> >
> > My question about this is, is the callback really needed? If there are any
> > FDWs that want to do the work *in their own way*, instead of just doing
> > ExecProcNode for executing a local join execution plan in case of foreign
> > join (or just doing ExecQual for checking remote quals in case of foreign
> > table), I'd agree with introducing the callback, but if not, I don't think
> > that that makes much sense.
>
> It doesn't seem to me that it hurts much of anything to add the
> callback there, and it does provide some flexibility. Actually, I'm
> not really sure why we're thinking we need a subplan here at all,
> rather than just having a ForeignRecheck callback that can do whatever
> it needs to do with no particular help from the core infrastructure.
> I think you wrote some code to show how postgres_fdw would use the API
> you are proposing, but I can't find it. Can you point me in the right
> direction?

I've heard that the reason for the (fs_)subplan is that it should
be initialized using create_plan_recurse, set_plan_refs and
finalyze_plan (or others), which are static functions in the
planner, unavailable in fdw code.

Does it make sense?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

> -----Original Message-----
> From: Kyotaro HORIGUCHI [mailto:horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp]
> Sent: Friday, September 11, 2015 2:05 PM
> To: robertmhaas(at)gmail(dot)com
> Cc: fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp; Kaigai Kouhei(海外 浩平);
> pgsql-hackers(at)postgresql(dot)org; shigeru(dot)hanada(at)gmail(dot)com
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> Hello,
>
> At Thu, 10 Sep 2015 17:24:00 -0400, Robert Haas <robertmhaas(at)gmail(dot)com> wrote
> in <CA+TgmobxksR2=3wEdY5cEgpd1hQ6Z0WoZEBBoxgs=XKZpbfUXA(at)mail(dot)gmail(dot)com>
> > On Thu, Sep 3, 2015 at 1:22 AM, Etsuro Fujita
> > <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > >> I'm wondering if there's another approach. If I understand correctly,
> > >> there are two reasons why the current situation is untenable. The
> > >> first is that ForeignRecheck always returns true, but we could instead
> > >> call an FDW-supplied callback routine there. The callback could be
> > >> optional, so that we just return true if there is none, which is nice
> > >> for already-existing FDWs that then don't need to do anything.
> > >
> > > My question about this is, is the callback really needed? If there are any
> > > FDWs that want to do the work *in their own way*, instead of just doing
> > > ExecProcNode for executing a local join execution plan in case of foreign
> > > join (or just doing ExecQual for checking remote quals in case of foreign
> > > table), I'd agree with introducing the callback, but if not, I don't think
> > > that that makes much sense.
> >
> > It doesn't seem to me that it hurts much of anything to add the
> > callback there, and it does provide some flexibility. Actually, I'm
> > not really sure why we're thinking we need a subplan here at all,
> > rather than just having a ForeignRecheck callback that can do whatever
> > it needs to do with no particular help from the core infrastructure.
> > I think you wrote some code to show how postgres_fdw would use the API
> > you are proposing, but I can't find it. Can you point me in the right
> > direction?
>
> I've heard that the reason for the (fs_)subplan is that it should
> be initialized using create_plan_recurse, set_plan_refs and
> finalyze_plan (or others), which are static functions in the
> planner, unavailable in fdw code.
>
It was a discussion when custom-scan/join interface got merged, because
I primarily designed the interface to call create_plan_recurse() from
the extension, however, we concluded that we keep this function as static
and tells the core a bunch of path-nodes to be initialized.
It also reduced interface complexity because we can omit callbacks to
be placed on the setrefs.c and subselect.c.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: Etsuro Fujita [mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp]
> Sent: Friday, September 11, 2015 12:36 PM
> To: Robert Haas
> Cc: Kaigai Kouhei(海外 浩平); PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> On 2015/09/11 6:24, Robert Haas wrote:
> > On Thu, Sep 3, 2015 at 1:22 AM, Etsuro Fujita
> > <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >>> I'm wondering if there's another approach. If I understand correctly,
> >>> there are two reasons why the current situation is untenable. The
> >>> first is that ForeignRecheck always returns true, but we could instead
> >>> call an FDW-supplied callback routine there. The callback could be
> >>> optional, so that we just return true if there is none, which is nice
> >>> for already-existing FDWs that then don't need to do anything.
> >>
> >> My question about this is, is the callback really needed? If there are any
> >> FDWs that want to do the work *in their own way*, instead of just doing
> >> ExecProcNode for executing a local join execution plan in case of foreign
> >> join (or just doing ExecQual for checking remote quals in case of foreign
> >> table), I'd agree with introducing the callback, but if not, I don't think
> >> that that makes much sense.
> >
> > It doesn't seem to me that it hurts much of anything to add the
> > callback there, and it does provide some flexibility. Actually, I'm
> > not really sure why we're thinking we need a subplan here at all,
> > rather than just having a ForeignRecheck callback that can do whatever
> > it needs to do with no particular help from the core infrastructure.
> > I think you wrote some code to show how postgres_fdw would use the API
> > you are proposing, but I can't find it. Can you point me in the right
> > direction?
>
> I've proposed the following API changes:
>
> * I modified create_foreignscan_path, which is called from
> postgresGetForeignJoinPaths/postgresGetForeignPaths, so that a path,
> subpath, is passed as the eighth argument of the function. subpath
> represents a local join execution path if scanrelid==0, but NULL if
> scanrelid>0.
>
I like to suggest to have multiple path nodes, like custom-scan, because
the infrastructure will be also helpful to implement FDW driver that can
have multiple sub-plans. One expected usage is here:
http://www.postgresql.org/message-id/9A28C8860F777E439AA12E8AEA7694F8010F20AD@BPXM15GP.gisp.nec.co.jp

> * I modified make_foreignscan, which is called from
> postgresGetForeignPlan, so that a list of quals, fdw_quals, is passed as
> the last argument of the function. fdw_quals represents remote quals if
> scanrelid>0, but NIL if scanrelid==0.
>
If a callback on ForeignRecheck processes EPQ rechecks, the core PostgreSQL
don't need to know what expression was pushed down and how does it kept in
the private field (fdw_exprs). Only FDW driver knows which private field is
the expression node that was pushed down to the remote side. It shall not be
an interface contract.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/09/11 6:02, Robert Haas wrote:
> On Thu, Sep 3, 2015 at 6:25 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> I gave it another thought; the following changes to ExecInitNode would make
>> the patch much simpler, ie, we would no longer need to call the new code in
>> ExecInitForeignScan, ExecForeignScan, ExecEndForeignScan, and
>> ExecReScanForeignScan. I think that would resolve the name problem also.
>>
>> *** a/src/backend/executor/execProcnode.c
>> --- b/src/backend/executor/execProcnode.c
>> ***************
>> *** 247,254 **** ExecInitNode(Plan *node, EState *estate, int eflags)
>> break;
>>
>> case T_ForeignScan:
>> ! result = (PlanState *) ExecInitForeignScan((ForeignScan *) node,
>> ! estate, eflags);
>> break;
>>
>> case T_CustomScan:
>> --- 247,269 ----
>> break;
>>
>> case T_ForeignScan:
>> ! {
>> ! Index scanrelid = ((ForeignScan *)
>> node)->scan.scanrelid;
>> !
>> ! if (estate->es_epqTuple != NULL && scanrelid == 0)
>> ! {
>> ! /*
>> ! * We are in foreign join inside an EvalPlanQual
>> recheck.
>> ! * Initialize local join execution plan, instead.
>> ! */
>> ! Plan *subplan = ((ForeignScan *)
>> node)->fs_subplan;
>> !
>> ! result = ExecInitNode(subplan, estate, eflags);
>> ! }
>> ! else
>> ! result = (PlanState *) ExecInitForeignScan((ForeignScan
>> *) node,
>> ! estate,
>> eflags);
>> ! }
>> break;
>
> I don't think that's a good idea. The Plan tree and the PlanState
> tree should be mirror images of each other; breaking that equivalence
> will cause confusion, at least.

IIRC, Horiguchi-san also pointed that out. Honestly, I also think that
that is weird, but IIUC, I think it can't hurt. What I was concerned
about was EXPLAIN, but EXPLAIN doesn't handle an EvalPlanQual PlanState
tree at least currently.

Best regards,
Etsuro Fujita

On 2015/09/11 6:30, Robert Haas wrote:
> On Wed, Sep 9, 2015 at 2:30 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> But that path might have already been discarded on the basis of cost.
>>> I think Tom's idea is better: let the FDW consult some state cached
>>> for this purpose in the RelOptInfo.
>>
>> Do you have an idea of what information would be collected into the state
>> and how the FDW would derive parameterizations to consider producing
>> pushed-down joins with from that information? What I'm concerned about that
>> is to reduce the number of parameterizations to consider, to reduce overhead
>> in costing the corresponding queries. I'm missing something, though.
>
> I think the thing we'd want to store in the state would be enough
> information to reconstruct a valid join nest. For example, the
> reloptinfo for (A B) might note that A needs to be left-joined to B.
> When we go to construct paths for (A B C), and there is no
> SpecialJoinInfo that mentions C, we know that we can construct (A LJ
> B) IJ C rather than (A IJ B) IJ C. If any paths survived, we could
> find a way to pull that information out of the path, but pulling it
> out of the RelOptInfo should always work.

So, information to address the how-to-build-the-query-text
problem would be stored in the state, in other words. Right?

> I am not sure what to do about parameterizations. That's one of my
> remaining concerns about moving the hook.

I think we should also make it clear what to do about sort orderings.

Best regards,
Etsuro Fujita

On Thu, Sep 10, 2015 at 11:36 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> I've proposed the following API changes:
>
> * I modified create_foreignscan_path, which is called from
> postgresGetForeignJoinPaths/postgresGetForeignPaths, so that a path,
> subpath, is passed as the eighth argument of the function. subpath
> represents a local join execution path if scanrelid==0, but NULL if
> scanrelid>0.

OK, I see now. But I don't much like the way
get_unsorted_unparameterized_path() looks.

First, it's basically praying that MergePath, NodePath, and NestPath
can be flat-copied without breaking anything. In general, we have
copyfuncs.c support for nodes that we need to be able to copy, and we
use copyObject() to do it. Even if what you've got here works today,
it's not very future-proof.

Second, what guarantee do we have that we'll find a path with no
pathkeys and a NULL param_info? Why can't all of the paths for a join
relation have pathkeys? Why can't they all be parameterized? I can't
think of anything that would guarantee that.

Third, even if such a guarantee existed, why is this the right
behavior? Any join type will produce the same output; it's just a
question of performance. And if you have only one tuple on each side,
surely a nested loop would be fine.

It seems to me that what you ought to be doing is using data hung off
the fdw_private field of each RelOptInfo to cache a NestPath that can
be used for EPQ rechecks at that level. When you go to consider
pushing down another join, you can build up a new NestPath that's
suitable for the new level. That seems much cleaner than groveling
through the list of surviving paths and hoping you find the right kind
of thing.

And all that having been said, I still don't really understand why you
are resisting the idea of providing a callback so that the FDW can
execute arbitrary code in the recheck path. There doesn't seem to be
any reason not to let the FDW take control of the rechecks if it
wishes, and there's no real cost in complexity that I can see.

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

On Fri, Sep 11, 2015 at 2:01 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> If a callback on ForeignRecheck processes EPQ rechecks, the core PostgreSQL
> don't need to know what expression was pushed down and how does it kept in
> the private field (fdw_exprs). Only FDW driver knows which private field is
> the expression node that was pushed down to the remote side. It shall not be
> an interface contract.

I agree. It seems needless to involve the core code here.

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

On Fri, Sep 11, 2015 at 3:08 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> IIRC, Horiguchi-san also pointed that out. Honestly, I also think that that
> is weird, but IIUC, I think it can't hurt. What I was concerned about was
> EXPLAIN, but EXPLAIN doesn't handle an EvalPlanQual PlanState tree at least
> currently.

This has come up a few times before and some people have argued for
changing the coding rule. Nevertheless, for now, it is the rule.
IMHO, it's a pretty good rule that makes things easier to understand
and reason about. If there's an argument for changing it, it's
performance, not developer convenience. Anyway, we should try to fix
this problem without getting tangled in that argument.

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

On Fri, Sep 11, 2015 at 3:12 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> So, information to address the how-to-build-the-query-text
> problem would be stored in the state, in other words. Right?

Right.

>> I am not sure what to do about parameterizations. That's one of my
>> remaining concerns about moving the hook.
>
> I think we should also make it clear what to do about sort orderings.

How does that come into it?

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

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Robert Haas
> Sent: Saturday, September 12, 2015 1:39 AM
> To: Etsuro Fujita
> Cc: Kaigai Kouhei(海外 浩平); PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> On Thu, Sep 10, 2015 at 11:36 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > I've proposed the following API changes:
> >
> > * I modified create_foreignscan_path, which is called from
> > postgresGetForeignJoinPaths/postgresGetForeignPaths, so that a path,
> > subpath, is passed as the eighth argument of the function. subpath
> > represents a local join execution path if scanrelid==0, but NULL if
> > scanrelid>0.
>
> OK, I see now. But I don't much like the way
> get_unsorted_unparameterized_path() looks.
>
> First, it's basically praying that MergePath, NodePath, and NestPath
> can be flat-copied without breaking anything. In general, we have
> copyfuncs.c support for nodes that we need to be able to copy, and we
> use copyObject() to do it. Even if what you've got here works today,
> it's not very future-proof.
>
> Second, what guarantee do we have that we'll find a path with no
> pathkeys and a NULL param_info? Why can't all of the paths for a join
> relation have pathkeys? Why can't they all be parameterized? I can't
> think of anything that would guarantee that.
>
> Third, even if such a guarantee existed, why is this the right
> behavior? Any join type will produce the same output; it's just a
> question of performance. And if you have only one tuple on each side,
> surely a nested loop would be fine.
>
> It seems to me that what you ought to be doing is using data hung off
> the fdw_private field of each RelOptInfo to cache a NestPath that can
> be used for EPQ rechecks at that level. When you go to consider
> pushing down another join, you can build up a new NestPath that's
> suitable for the new level. That seems much cleaner than groveling
> through the list of surviving paths and hoping you find the right kind
> of thing.
>
> And all that having been said, I still don't really understand why you
> are resisting the idea of providing a callback so that the FDW can
> execute arbitrary code in the recheck path. There doesn't seem to be
> any reason not to let the FDW take control of the rechecks if it
> wishes, and there's no real cost in complexity that I can see.
>
The discussion has been pending for two weeks, even though we put this
problem on the open item towards v9.5; that means we recognize it is
a problem to be fixed by the v9.5 release.

The attached patch allows FDW driver to handle EPQ recheck by its own
preferable way, even if it is alternative local join or ExecQual to
the expression being pushed down.

Regarding to the alternative join path selection, I initially thought
it is valuable to choose the best path from performance standpoint,
however, what we need to do here is visibility check towards all the
EPQ tuples already loaded to EState. So, unparametalized NestLoop is
sufficient to execute qualifier across relations.
(What happen if HashJoin is chosen? It's probably problematic.)

So, if your modified postgres_fdw keeps an alternative path, what
we need to do is construction of dummy NestPath with no param_info,
no pathkeys, and dummy cost. Then, give this path on fdw_paths of
ForeignPath. It shall be transformed to plan-nodes, then eventually
transformed to plan-state-node by postgres_fdw itself.
I cannot find out something difficult to do any more.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Mon, Sep 28, 2015 at 3:34 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> The attached patch allows FDW driver to handle EPQ recheck by its own
> preferable way, even if it is alternative local join or ExecQual to
> the expression being pushed down.

Thanks. I was all set to commit this, or at least part of it, when I
noticed that we already have an FDW callback called RefetchForeignRow.
We seem to be intending that this new callback should refetch the row
from the foreign server and verify that any pushed-down quals apply to
it. But why can't RefetchForeignRow do that? That seems to be what
it's for.

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

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Robert Haas
> Sent: Tuesday, September 29, 2015 5:46 AM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Etsuro Fujita; PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> On Mon, Sep 28, 2015 at 3:34 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> > The attached patch allows FDW driver to handle EPQ recheck by its own
> > preferable way, even if it is alternative local join or ExecQual to
> > the expression being pushed down.
>
> Thanks. I was all set to commit this, or at least part of it, when I
> noticed that we already have an FDW callback called RefetchForeignRow.
> We seem to be intending that this new callback should refetch the row
> from the foreign server and verify that any pushed-down quals apply to
> it. But why can't RefetchForeignRow do that? That seems to be what
> it's for.
>
At least here are two matters to solve the problem with RefetchForeignRow.

1. RefetchForeignRow() does not take ForeignScanState argument, so it is
not obvious how to cooperate with the private state in ForeignScanState;
that may include expression pushed down, and so on.

2. ForeignScan with scanrelid == 0 represents the result of joined
relations. Even if the refetched tuple is visible on base-relation
level, it may not survive the join condition at the upper level.
Once relations join get pushed down, only FDW driver knows how
base relations are joined.

So, it is the only reasonable way to ask FDW driver on ExecScanFetch,
to check visibility of a particular tuple or another tuple made from
this.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/09/29 9:13, Kouhei Kaigai wrote:
>> -----Original Message-----
>> From: pgsql-hackers-owner(at)postgresql(dot)org
>> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Robert Haas
>> Sent: Tuesday, September 29, 2015 5:46 AM
>> To: Kaigai Kouhei(海外 浩平)
>> Cc: Etsuro Fujita; PostgreSQL-development; 花田茂
>> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>>
>> On Mon, Sep 28, 2015 at 3:34 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>>> The attached patch allows FDW driver to handle EPQ recheck by its own
>>> preferable way, even if it is alternative local join or ExecQual to
>>> the expression being pushed down.

Thanks for the work, KaiGai-san!

>> Thanks. I was all set to commit this, or at least part of it, when I
>> noticed that we already have an FDW callback called RefetchForeignRow.
>> We seem to be intending that this new callback should refetch the row
>> from the foreign server and verify that any pushed-down quals apply to
>> it. But why can't RefetchForeignRow do that? That seems to be what
>> it's for.

Thanks for the comments, Robert!

I thought the same thing [1]. While I thought it was relatively easy to
make changes to RefetchForeignRow that way for the foreign table case
(scanrelid>0), I was not sure how hard it would be to do so for the
foreign join case (scanrelid==0). So, I proposed to leave that changes
for 9.6. I'll have a rethink on this issue along the lines of that
approach.

Sorry for having had no response. I was on vacation.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/55DEB5A9.8010604@lab.ntt.co.jp

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> Sent: Tuesday, September 29, 2015 12:15 PM
> To: Kaigai Kouhei(海外 浩平); Robert Haas
> Cc: PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> On 2015/09/29 9:13, Kouhei Kaigai wrote:
> >> -----Original Message-----
> >> From: pgsql-hackers-owner(at)postgresql(dot)org
> >> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Robert Haas
> >> Sent: Tuesday, September 29, 2015 5:46 AM
> >> To: Kaigai Kouhei(海外 浩平)
> >> Cc: Etsuro Fujita; PostgreSQL-development; 花田茂
> >> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> >>
> >> On Mon, Sep 28, 2015 at 3:34 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> >>> The attached patch allows FDW driver to handle EPQ recheck by its own
> >>> preferable way, even if it is alternative local join or ExecQual to
> >>> the expression being pushed down.
>
> Thanks for the work, KaiGai-san!
>
> >> Thanks. I was all set to commit this, or at least part of it, when I
> >> noticed that we already have an FDW callback called RefetchForeignRow.
> >> We seem to be intending that this new callback should refetch the row
> >> from the foreign server and verify that any pushed-down quals apply to
> >> it. But why can't RefetchForeignRow do that? That seems to be what
> >> it's for.
>
> Thanks for the comments, Robert!
>
> I thought the same thing [1]. While I thought it was relatively easy to
> make changes to RefetchForeignRow that way for the foreign table case
> (scanrelid>0), I was not sure how hard it would be to do so for the
> foreign join case (scanrelid==0). So, I proposed to leave that changes
> for 9.6. I'll have a rethink on this issue along the lines of that
> approach.
>
Even if base relation case, is it really easy to do?

RefetchForeignRow() does not take ForeignScanState as its argument,
so it is not obvious to access its private field, isn't it?
ExecRowMark contains "rti" field, so it might be feasible to find out
the target PlanState using walker routine recently supported, although
it is not a simple enough.
Unless we don't have reference to the private field, it is not feasible
to access expression that was pushed down to the remote-side, therefore,
it does not allow to apply proper rechecks here.

In addition, it is problematic when scanrelid==0 because we have no
relevant ForeignScanState which represents the base relations, even
though ExecRowMark is associated with a particular base relation.
In case of scanrelid==0, EPQ recheck routine also have to ensure
the EPQ tuple is visible towards the join condition in addition to
the qualifier of base relation. These information is also stored within
private data field, so it has to have a reference to the private data
of ForeignScanState of the remote join (scanrelid==0) which contains
the target relation.

Could you introduce us (1) how to access private data field of
ForeignScanState from the RefetchForeignRow callback? (2) why it
is reasonable to implement than the callback on ForeignRecheck().

> Sorry for having had no response. I was on vacation.
>
Me too. :-)

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/09/29 13:55, Kouhei Kaigai wrote:
>> From: pgsql-hackers-owner(at)postgresql(dot)org
>> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
>> On 2015/09/29 9:13, Kouhei Kaigai wrote:

>>>> From: pgsql-hackers-owner(at)postgresql(dot)org
>>>> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Robert Haas
>>>> On Mon, Sep 28, 2015 at 3:34 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:

>>>>> The attached patch allows FDW driver to handle EPQ recheck by its own
>>>>> preferable way, even if it is alternative local join or ExecQual to
>>>>> the expression being pushed down.

>>>> Thanks. I was all set to commit this, or at least part of it, when I
>>>> noticed that we already have an FDW callback called RefetchForeignRow.
>>>> We seem to be intending that this new callback should refetch the row
>>>> from the foreign server and verify that any pushed-down quals apply to
>>>> it. But why can't RefetchForeignRow do that? That seems to be what
>>>> it's for.

>> I thought the same thing [1]. While I thought it was relatively easy to
>> make changes to RefetchForeignRow that way for the foreign table case
>> (scanrelid>0), I was not sure how hard it would be to do so for the
>> foreign join case (scanrelid==0). So, I proposed to leave that changes
>> for 9.6. I'll have a rethink on this issue along the lines of that
>> approach.

> Even if base relation case, is it really easy to do?
>
> RefetchForeignRow() does not take ForeignScanState as its argument,
> so it is not obvious to access its private field, isn't it?
> ExecRowMark contains "rti" field, so it might be feasible to find out
> the target PlanState using walker routine recently supported, although
> it is not a simple enough.
> Unless we don't have reference to the private field, it is not feasible
> to access expression that was pushed down to the remote-side, therefore,
> it does not allow to apply proper rechecks here.
>
> In addition, it is problematic when scanrelid==0 because we have no
> relevant ForeignScanState which represents the base relations, even
> though ExecRowMark is associated with a particular base relation.
> In case of scanrelid==0, EPQ recheck routine also have to ensure
> the EPQ tuple is visible towards the join condition in addition to
> the qualifier of base relation. These information is also stored within
> private data field, so it has to have a reference to the private data
> of ForeignScanState of the remote join (scanrelid==0) which contains
> the target relation.
>
> Could you introduce us (1) how to access private data field of
> ForeignScanState from the RefetchForeignRow callback? (2) why it
> is reasonable to implement than the callback on ForeignRecheck().

For the foreign table case (scanrelid>0), I imagined an approach
different than yours. In that case, I thought the issue would be
probably addressed by just modifying the remote query performed in
RefetchForeignRow, which would be of the form "SELECT ctid, * FROM
remote table WHERE ctid = $1", so that the modified query would be of
the form "SELECT ctid, * FROM remote table WHERE ctid = $1 AND *remote
quals*".

For the foreign join case (scanrelid==0), in my vision, I think we would
need some changes not only to RefetchForeignRow but to the existing
EvalPlanQual machinery in the core. I've not had a clear image yet, though.

Best regards,
Etsuro Fujita

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> Sent: Tuesday, September 29, 2015 4:36 PM
> To: Kaigai Kouhei(海外 浩平); Robert Haas
> Cc: PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> On 2015/09/29 13:55, Kouhei Kaigai wrote:
> >> From: pgsql-hackers-owner(at)postgresql(dot)org
> >> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> >> On 2015/09/29 9:13, Kouhei Kaigai wrote:
>
> >>>> From: pgsql-hackers-owner(at)postgresql(dot)org
> >>>> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Robert Haas
> >>>> On Mon, Sep 28, 2015 at 3:34 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>
> >>>>> The attached patch allows FDW driver to handle EPQ recheck by its own
> >>>>> preferable way, even if it is alternative local join or ExecQual to
> >>>>> the expression being pushed down.
>
> >>>> Thanks. I was all set to commit this, or at least part of it, when I
> >>>> noticed that we already have an FDW callback called RefetchForeignRow.
> >>>> We seem to be intending that this new callback should refetch the row
> >>>> from the foreign server and verify that any pushed-down quals apply to
> >>>> it. But why can't RefetchForeignRow do that? That seems to be what
> >>>> it's for.
>
> >> I thought the same thing [1]. While I thought it was relatively easy to
> >> make changes to RefetchForeignRow that way for the foreign table case
> >> (scanrelid>0), I was not sure how hard it would be to do so for the
> >> foreign join case (scanrelid==0). So, I proposed to leave that changes
> >> for 9.6. I'll have a rethink on this issue along the lines of that
> >> approach.
>
> > Even if base relation case, is it really easy to do?
> >
> > RefetchForeignRow() does not take ForeignScanState as its argument,
> > so it is not obvious to access its private field, isn't it?
> > ExecRowMark contains "rti" field, so it might be feasible to find out
> > the target PlanState using walker routine recently supported, although
> > it is not a simple enough.
> > Unless we don't have reference to the private field, it is not feasible
> > to access expression that was pushed down to the remote-side, therefore,
> > it does not allow to apply proper rechecks here.
> >
> > In addition, it is problematic when scanrelid==0 because we have no
> > relevant ForeignScanState which represents the base relations, even
> > though ExecRowMark is associated with a particular base relation.
> > In case of scanrelid==0, EPQ recheck routine also have to ensure
> > the EPQ tuple is visible towards the join condition in addition to
> > the qualifier of base relation. These information is also stored within
> > private data field, so it has to have a reference to the private data
> > of ForeignScanState of the remote join (scanrelid==0) which contains
> > the target relation.
> >
> > Could you introduce us (1) how to access private data field of
> > ForeignScanState from the RefetchForeignRow callback? (2) why it
> > is reasonable to implement than the callback on ForeignRecheck().
>
> For the foreign table case (scanrelid>0), I imagined an approach
> different than yours. In that case, I thought the issue would be
> probably addressed by just modifying the remote query performed in
> RefetchForeignRow, which would be of the form "SELECT ctid, * FROM
> remote table WHERE ctid = $1", so that the modified query would be of
> the form "SELECT ctid, * FROM remote table WHERE ctid = $1 AND *remote
> quals*".
>
My question is how to pull expression of the remote query.
It shall be stored at somewhere private field of ForeignScanState,
however, RefetchForeignRow does not have direct access to the
relevant ForeignScanState node.
It is what I asked at the question (1).

Also note that EvalPlanQualFetchRowMarks() will raise an error
if RefetchForeignRow callback returned NULL tuple.
Is it right or expected behavior?
It looks to me this callback is designed to pull out a particular
tuple identified by the remote-row-id, regardless of the qualifier
checks based on the latest value.

> For the foreign join case (scanrelid==0), in my vision, I think we would
> need some changes not only to RefetchForeignRow but to the existing
> EvalPlanQual machinery in the core. I've not had a clear image yet, though.
>
If people agree with FDW remote join is incomplete feature in v9.5,
the attached fix-up is the minimum requirement from the standpoint
of custom-scan/join.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On 2015/09/29 17:49, Kouhei Kaigai wrote:
>> From: pgsql-hackers-owner(at)postgresql(dot)org
>> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita

>>> RefetchForeignRow() does not take ForeignScanState as its argument,
>>> so it is not obvious to access its private field, isn't it?
>>> ExecRowMark contains "rti" field, so it might be feasible to find out
>>> the target PlanState using walker routine recently supported, although
>>> it is not a simple enough.
>>> Unless we don't have reference to the private field, it is not feasible
>>> to access expression that was pushed down to the remote-side, therefore,
>>> it does not allow to apply proper rechecks here.

>>> Could you introduce us (1) how to access private data field of
>>> ForeignScanState from the RefetchForeignRow callback?

>> For the foreign table case (scanrelid>0), I imagined an approach
>> different than yours. In that case, I thought the issue would be
>> probably addressed by just modifying the remote query performed in
>> RefetchForeignRow, which would be of the form "SELECT ctid, * FROM
>> remote table WHERE ctid = $1", so that the modified query would be of
>> the form "SELECT ctid, * FROM remote table WHERE ctid = $1 AND *remote
>> quals*".

Sorry, I forgot to add "FOR UPDATE" to the before/after queries.

> My question is how to pull expression of the remote query.
> It shall be stored at somewhere private field of ForeignScanState,
> however, RefetchForeignRow does not have direct access to the
> relevant ForeignScanState node.
> It is what I asked at the question (1).

I imagined the following steps to get the remote query string: (1)
create the remote query string and store it in fdw_private during
postgresGetForeignPlan, (2) extract the string from fdw_private and
store it in erm->ermExtra during postgresBeginForeignScan, and (3)
extract the string from erm->ermExtra in postgresRefetchForeignRow.

> Also note that EvalPlanQualFetchRowMarks() will raise an error
> if RefetchForeignRow callback returned NULL tuple.
> Is it right or expected behavior?

IIUC, I think that that behavior is reasonable.

> It looks to me this callback is designed to pull out a particular
> tuple identified by the remote-row-id, regardless of the qualifier
> checks based on the latest value.

Because erm->markType==ROW_MARK_REFERENCE, I don't think that that
behavior would cause any problem. Maybe I'm missing something, though.

Best regards,
Etsuro Fujita

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> Sent: Tuesday, September 29, 2015 8:00 PM
> To: Kaigai Kouhei(海外 浩平); Robert Haas
> Cc: PostgreSQL-development; 花田茂
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
>
> On 2015/09/29 17:49, Kouhei Kaigai wrote:
> >> From: pgsql-hackers-owner(at)postgresql(dot)org
> >> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
>
> >>> RefetchForeignRow() does not take ForeignScanState as its argument,
> >>> so it is not obvious to access its private field, isn't it?
> >>> ExecRowMark contains "rti" field, so it might be feasible to find out
> >>> the target PlanState using walker routine recently supported, although
> >>> it is not a simple enough.
> >>> Unless we don't have reference to the private field, it is not feasible
> >>> to access expression that was pushed down to the remote-side, therefore,
> >>> it does not allow to apply proper rechecks here.
>
> >>> Could you introduce us (1) how to access private data field of
> >>> ForeignScanState from the RefetchForeignRow callback?
>
> >> For the foreign table case (scanrelid>0), I imagined an approach
> >> different than yours. In that case, I thought the issue would be
> >> probably addressed by just modifying the remote query performed in
> >> RefetchForeignRow, which would be of the form "SELECT ctid, * FROM
> >> remote table WHERE ctid = $1", so that the modified query would be of
> >> the form "SELECT ctid, * FROM remote table WHERE ctid = $1 AND *remote
> >> quals*".
>
> Sorry, I forgot to add "FOR UPDATE" to the before/after queries.
>
> > My question is how to pull expression of the remote query.
> > It shall be stored at somewhere private field of ForeignScanState,
> > however, RefetchForeignRow does not have direct access to the
> > relevant ForeignScanState node.
> > It is what I asked at the question (1).
>
> I imagined the following steps to get the remote query string: (1)
> create the remote query string and store it in fdw_private during
> postgresGetForeignPlan, (2) extract the string from fdw_private and
> store it in erm->ermExtra during postgresBeginForeignScan, and (3)
> extract the string from erm->ermExtra in postgresRefetchForeignRow.
>
> > Also note that EvalPlanQualFetchRowMarks() will raise an error
> > if RefetchForeignRow callback returned NULL tuple.
> > Is it right or expected behavior?
>
> IIUC, I think that that behavior is reasonable.
>
> > It looks to me this callback is designed to pull out a particular
> > tuple identified by the remote-row-id, regardless of the qualifier
> > checks based on the latest value.
>
> Because erm->markType==ROW_MARK_REFERENCE, I don't think that that
> behavior would cause any problem. Maybe I'm missing something, though.
>
Really?

ExecLockRows() calls EvalPlanQualFetchRowMarks() to fill up EPQ tuple
slot prior to EvalPlanQualNext(), because these tuples are referenced
during EPQ rechecks.
The purpose of EvalPlanQualNext() is evaluate whether the current bunch
of rows are visible towards the qualifiers of underlying scan/join.
Then, if not visible, it *ignores* the current tuples, as follows.

/*
* Now fetch any non-locked source rows --- the EPQ logic knows how to
* do that.
*/
EvalPlanQualSetSlot(&node->lr_epqstate, slot);
EvalPlanQualFetchRowMarks(&node->lr_epqstate); <--- LOAD REMOTE ROWS

/*
* And finally we can re-evaluate the tuple.
*/
slot = EvalPlanQualNext(&node->lr_epqstate); <--- EVALUATE QUALIFIERS
if (TupIsNull(slot))
{
/* Updated tuple fails qual, so ignore it and go on */
goto lnext; <-- IGNORE THE ROW, NOT RAISE AN ERROR
}

What happen if RefetchForeignRow raise an error in case when the latest
row exists but violated towards the "remote quals" ?
This is the case to be ignored, unlike the case when remote row identified
by row-id didn't exist.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Tue, Sep 29, 2015 at 4:49 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> Also note that EvalPlanQualFetchRowMarks() will raise an error
> if RefetchForeignRow callback returned NULL tuple.
> Is it right or expected behavior?

That's not how I read the code. If RefetchForeignRow returns NULL, we
just ignore the row and continue on to the next one:

if (copyTuple == NULL)
{
/* couldn't get the lock, so skip this row */
goto lnext;
}

And that seems exactly right: RefetchForeignRow needs to test that the
tuple is still present on the remote side, and that any remote quals
are matched. If either of those is false, it can return NULL.

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