Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Dec 22, 2015 at 2:00 AM, Michael Paquier
 wrote:
> On Tue, Dec 22, 2015 at 3:52 PM, Amit Langote
>  wrote:
>> On 2015/12/22 15:24, Michael Paquier wrote:
>>> As this debate continues, I think that moving this patch to the next
>>> CF would make the most sense then.. So done this way.
>>
>> Perhaps, this ended (?) with the following commit:
>>
>> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=385f337c9f39b21dca96ca4770552a10a6d5af24
>
> Ah, thanks! What has been committed is actually more or less
> epq-recheck-v6-efujita.patch posted upthread, I'll mark the patch as
> committed then.

+1.  And thanks.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Amit Langote]

On 2015/12/22 15:24, Michael Paquier wrote:
> On Thu, Dec 10, 2015 at 1:32 AM, Robert Haas  wrote:
>> If we get the feature - join pushdown for postgres_fdw -
>> working, then we might get some feedback from users about what they
>> like about it or don't, and certainly if this is a frequent complaint
>> then that bolsters the case for doing something about it, and possibly
>> also helps us figure out what that thing should be.  On the other
>> hand, if we don't get the feature because we're busy debating
>> interface details related to this patch, then none of these details
>> matter anyway because nobody except developer is actually running the
>> code in question.
> 
> As this debate continues, I think that moving this patch to the next
> CF would make the most sense then.. So done this way.

Perhaps, this ended (?) with the following commit:

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=385f337c9f39b21dca96ca4770552a10a6d5af24

Thanks,
Amit




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Michael Paquier]

On Thu, Dec 10, 2015 at 1:32 AM, Robert Haas  wrote:
> On Wed, Dec 9, 2015 at 3:22 AM, Etsuro Fujita
>  wrote:
>> Sorry, my explanation might be not enough, but I'm not saying to hide the
>> subplan.  I think it would be better to show the subplan somewhere in the
>> EXPLAIN outout, but I'm not sure that it's a good idea to show that in the
>> current form.  We have two plan trees; one for normal query execution and
>> another for EvalPlanQual testing.  I think it'd be better to show the
>> EXPLAIN output the way that allows users to easily identify each of the plan
>> trees.
>
> It's hard to do that because we don't identify that internally
> anywhere.  Like I said before, the possibility of a ForeignScan having
> an outer subplan is formally independent of the new EPQ stuff, and I'd
> prefer to maintain that separation and just address this with
> documentation.

Fujita-san, others, could this be addressed with documentation?

> Getting this bug fixed has been one of the more exhausting experiences
> of my involvement with PostgreSQL, and to be honest, I think I'd like
> to stop spending too much time on this now and work on getting the
> feature that this is intended to support working.  Right now, the only
> people who can have an opinion on this topic are those who are
> following this thread in detail, and there really aren't that many of
> those.

I am numbering that to mainly 3 people, you included :)

> If we get the feature - join pushdown for postgres_fdw -
> working, then we might get some feedback from users about what they
> like about it or don't, and certainly if this is a frequent complaint
> then that bolsters the case for doing something about it, and possibly
> also helps us figure out what that thing should be.  On the other
> hand, if we don't get the feature because we're busy debating
> interface details related to this patch, then none of these details
> matter anyway because nobody except developer is actually running the
> code in question.

As this debate continues, I think that moving this patch to the next
CF would make the most sense then.. So done this way.
-- 
Michael


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Michael Paquier]

On Tue, Dec 22, 2015 at 3:52 PM, Amit Langote
 wrote:
> On 2015/12/22 15:24, Michael Paquier wrote:
>> As this debate continues, I think that moving this patch to the next
>> CF would make the most sense then.. So done this way.
>
> Perhaps, this ended (?) with the following commit:
>
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=385f337c9f39b21dca96ca4770552a10a6d5af24

Ah, thanks! What has been committed is actually more or less
epq-recheck-v6-efujita.patch posted upthread, I'll mark the patch as
committed then.
-- 
Michael


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/12/09 13:26, Kouhei Kaigai wrote:

On Tue, Dec 8, 2015 at 10:00 PM, Etsuro Fujita
 wrote:

I think the actual regression test outputs are fine, and that your
desire to suppress part of the plan tree from showing up in the
EXPLAIN output is misguided.  I like it just the way it is.  To
prevent user confusion, I think that when we add support to
postgres_fdw for this we might also want to add some documentation
explaining how to interpret this EXPLAIN output, but I don't think
there's any problem with the output itself.



I'm not sure that that's a good idea.  one reason for that is I think that
that would be more confusing to users when more than two foreign tables are
involved in a foreign join as shown in the following example.  Note that the
outer plans will be shown recursively.  Another reason is there is no
consistency between the costs of the outer plans and that of the main plan.



I still don't really see a problem here, but, regardless, the solution
can't be to hide nodes that are in fact present from the user.  We can
talk about making further changes here, but hiding the nodes
altogether is categorically out in my mind.



If you really want to hide the alternative sub-plan, you can move the
outer planstate onto somewhere private field on BeginForeignScan,
then kick ExecProcNode() at the ForeignRecheck callback by itself.
Explain walks down the sub-plan if outerPlanState(planstate) is
valid. So, as long as your extension keeps the planstate privately,
it is not visible from the EXPLAIN.

Of course, I don't recommend it.


Sorry, my explanation might be not enough, but I'm not saying to hide 
the subplan.  I think it would be better to show the subplan somewhere 
in the EXPLAIN outout, but I'm not sure that it's a good idea to show 
that in the current form.  We have two plan trees; one for normal query 
execution and another for EvalPlanQual testing.  I think it'd be better 
to show the EXPLAIN output the way that allows users to easily identify 
each of the plan trees.


Best regards,
Etsuro Fujita




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Wed, Dec 9, 2015 at 3:22 AM, Etsuro Fujita
 wrote:
> Sorry, my explanation might be not enough, but I'm not saying to hide the
> subplan.  I think it would be better to show the subplan somewhere in the
> EXPLAIN outout, but I'm not sure that it's a good idea to show that in the
> current form.  We have two plan trees; one for normal query execution and
> another for EvalPlanQual testing.  I think it'd be better to show the
> EXPLAIN output the way that allows users to easily identify each of the plan
> trees.

It's hard to do that because we don't identify that internally
anywhere.  Like I said before, the possibility of a ForeignScan having
an outer subplan is formally independent of the new EPQ stuff, and I'd
prefer to maintain that separation and just address this with
documentation.

Getting this bug fixed has been one of the more exhausting experiences
of my involvement with PostgreSQL, and to be honest, I think I'd like
to stop spending too much time on this now and work on getting the
feature that this is intended to support working.  Right now, the only
people who can have an opinion on this topic are those who are
following this thread in detail, and there really aren't that many of
those.  If we get the feature - join pushdown for postgres_fdw -
working, then we might get some feedback from users about what they
like about it or don't, and certainly if this is a frequent complaint
then that bolsters the case for doing something about it, and possibly
also helps us figure out what that thing should be.  On the other
hand, if we don't get the feature because we're busy debating
interface details related to this patch, then none of these details
matter anyway because nobody except developer is actually running the
code in question.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Dec 8, 2015 at 5:49 AM, Etsuro Fujita
 wrote:
> On 2015/12/08 3:06, Tom Lane wrote:
>> Robert Haas  writes:
>>> I think the core system likely needs visibility into where paths and
>>> plans are present in node trees, and putting them somewhere inside
>>> fdw_private would be going in the opposite direction.
>
>> Absolutely.  You don't really want FDWs having to take responsibility
>> for setrefs.c processing of their node trees, for example.  This is why
>> e.g. ForeignScan has both fdw_exprs and fdw_private.
>>
>> I'm not too concerned about whether we have to adjust FDW-related APIs
>> as we go along.  It's been clear from the beginning that we'd have to
>> do that, and we are nowhere near a point where we should promise that
>> we're done doing so.
>
> OK, I'd vote for Robert's idea, then.  I'd like to discuss the next
> thing about his patch.  As I mentioned in [1], the following change in
> the patch will break the EXPLAIN output.
>
> @@ -205,6 +218,11 @@ ExecInitForeignScan(ForeignScan *node, EState
> *estate, int eflags)
> scanstate->fdwroutine = fdwroutine;
> scanstate->fdw_state = NULL;
>
> +   /* Initialize any outer plan. */
> +   if (outerPlanState(scanstate))
> +   outerPlanState(scanstate) =
> +   ExecInitNode(outerPlan(node), estate, eflags);
> +
>
> As pointed out by Horiguchi-san, that's not correct, though; we should
> initialize the outer plan if outerPlan(node) != NULL, not
> outerPlanState(scanstate) != NULL.  Attached is an updated version of
> his patch.

Oops, good catch.

> I'm also attaching an updated version of the postgres_fdw
> join pushdown patch.

Is that based on Ashutosh's version of the patch, or are the two of
you developing independent of each other?  We should avoid dueling
patches if possible.

> You can find the breaking examples by doing the
> regression tests in the postgres_fdw patch.  Please apply the patches in
> the following order:
>
> epq-recheck-v6-efujita (attached)
> usermapping_matching.patch in [2]
> add_GetUserMappingById.patch in [2]
> foreign_join_v16_efujita2.patch (attached)
>
> As I proposed upthread, I think we could fix that by handling the outer
> plan as in the patch [3]; a) the core initializes the outer plan and
> stores it into somewhere in the ForeignScanState node, not the lefttree
> of the ForeignScanState node, during ExecInitForeignScan, and b) when
> the RecheckForeignScan routine gets called, the FDW extracts the plan
> from the given ForeignScanState node and executes it.  What do you think
> about that?

I think the actual regression test outputs are fine, and that your
desire to suppress part of the plan tree from showing up in the
EXPLAIN output is misguided.  I like it just the way it is.  To
prevent user confusion, I think that when we add support to
postgres_fdw for this we might also want to add some documentation
explaining how to interpret this EXPLAIN output, but I don't think
there's any problem with the output itself.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Dec 8, 2015 at 10:00 PM, Etsuro Fujita
 wrote:
>> I think the actual regression test outputs are fine, and that your
>> desire to suppress part of the plan tree from showing up in the
>> EXPLAIN output is misguided.  I like it just the way it is.  To
>> prevent user confusion, I think that when we add support to
>> postgres_fdw for this we might also want to add some documentation
>> explaining how to interpret this EXPLAIN output, but I don't think
>> there's any problem with the output itself.
>
> I'm not sure that that's a good idea.  one reason for that is I think that
> that would be more confusing to users when more than two foreign tables are
> involved in a foreign join as shown in the following example.  Note that the
> outer plans will be shown recursively.  Another reason is there is no
> consistency between the costs of the outer plans and that of the main plan.

I still don't really see a problem here, but, regardless, the solution
can't be to hide nodes that are in fact present from the user.  We can
talk about making further changes here, but hiding the nodes
altogether is categorically out in my mind.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Tue, Dec 8, 2015 at 10:00 PM, Etsuro Fujita
>  wrote:
> >> I think the actual regression test outputs are fine, and that your
> >> desire to suppress part of the plan tree from showing up in the
> >> EXPLAIN output is misguided.  I like it just the way it is.  To
> >> prevent user confusion, I think that when we add support to
> >> postgres_fdw for this we might also want to add some documentation
> >> explaining how to interpret this EXPLAIN output, but I don't think
> >> there's any problem with the output itself.
> >
> > I'm not sure that that's a good idea.  one reason for that is I think that
> > that would be more confusing to users when more than two foreign tables are
> > involved in a foreign join as shown in the following example.  Note that the
> > outer plans will be shown recursively.  Another reason is there is no
> > consistency between the costs of the outer plans and that of the main plan.
> 
> I still don't really see a problem here, but, regardless, the solution
> can't be to hide nodes that are in fact present from the user.  We can
> talk about making further changes here, but hiding the nodes
> altogether is categorically out in my mind.
>
Fujita-san,

If you really want to hide the alternative sub-plan, you can move the
outer planstate onto somewhere private field on BeginForeignScan,
then kick ExecProcNode() at the ForeignRecheck callback by itself.
Explain walks down the sub-plan if outerPlanState(planstate) is
valid. So, as long as your extension keeps the planstate privately,
it is not visible from the EXPLAIN.

Of course, I don't recommend it.
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/12/09 1:13, Robert Haas wrote:

On Tue, Dec 8, 2015 at 5:49 AM, Etsuro Fujita
 wrote:

I'd like to discuss the next
thing about his patch.  As I mentioned in [1], the following change in
the patch will break the EXPLAIN output.

@@ -205,6 +218,11 @@ ExecInitForeignScan(ForeignScan *node, EState
*estate, int eflags)
 scanstate->fdwroutine = fdwroutine;
 scanstate->fdw_state = NULL;

+   /* Initialize any outer plan. */
+   if (outerPlanState(scanstate))
+   outerPlanState(scanstate) =
+   ExecInitNode(outerPlan(node), estate, eflags);
+

As pointed out by Horiguchi-san, that's not correct, though; we should
initialize the outer plan if outerPlan(node) != NULL, not
outerPlanState(scanstate) != NULL.  Attached is an updated version of
his patch.



I'm also attaching an updated version of the postgres_fdw
join pushdown patch.



Is that based on Ashutosh's version of the patch, or are the two of
you developing independent of each other?  We should avoid dueling
patches if possible.


That's not based on his version.  I'll add to his patch changes I've 
made.  IIUC, his version is an updated version of Hanada-san's original 
patches that I've modified, so I guess that I could do that easily. 
(I've added a helper function for creating a local join execution plan 
for a given foreign join, but that is a rush work.  So, I'll rewrite that.)



You can find the breaking examples by doing the
regression tests in the postgres_fdw patch.  Please apply the patches in
the following order:

epq-recheck-v6-efujita (attached)
usermapping_matching.patch in [2]
add_GetUserMappingById.patch in [2]
foreign_join_v16_efujita2.patch (attached)

As I proposed upthread, I think we could fix that by handling the outer
plan as in the patch [3]; a) the core initializes the outer plan and
stores it into somewhere in the ForeignScanState node, not the lefttree
of the ForeignScanState node, during ExecInitForeignScan, and b) when
the RecheckForeignScan routine gets called, the FDW extracts the plan
from the given ForeignScanState node and executes it.  What do you think
about that?



I think the actual regression test outputs are fine, and that your
desire to suppress part of the plan tree from showing up in the
EXPLAIN output is misguided.  I like it just the way it is.  To
prevent user confusion, I think that when we add support to
postgres_fdw for this we might also want to add some documentation
explaining how to interpret this EXPLAIN output, but I don't think
there's any problem with the output itself.


I'm not sure that that's a good idea.  one reason for that is I think 
that that would be more confusing to users when more than two foreign 
tables are involved in a foreign join as shown in the following example. 
 Note that the outer plans will be shown recursively.  Another reason 
is there is no consistency between the costs of the outer plans and that 
of the main plan.


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


 QUERY PLAN




 LockRows  (cost=100.00..100.45 rows=15 width=96)
   Output: foo.a, bar.a, baz.a, foo.*, bar.*, baz.*
   ->  Foreign Scan  (cost=100.00..100.30 rows=15 width=96)
 Output: foo.a, bar.a, baz.a, foo.*, bar.*, baz.*
 Relations: ((public.foo) INNER JOIN (public.bar)) INNER JOIN 
(public.baz)
 Remote SQL: SELECT l.a1, l.a2, l.a3, l.a4, r.a1, r.a2 FROM 
(SELECT l.a1, l.a2, r.a1, r.a2 FROM (SELECT l.a9, ROW(l.a9) FROM (SELECT 
a a9 FROM p
ublic.foo FOR UPDATE) l) l (a1, a2) INNER JOIN (SELECT r.a9, ROW(r.a9) 
FROM (SELECT a a9 FROM public.bar FOR UPDATE) r) r (a1, a2) ON ((l.a1 = 
r.a1))) l
 (a1, a2, a3, a4) INNER JOIN (SELECT r.a9, ROW(r.a9) FROM (SELECT a a9 
FROM public.baz FOR UPDATE) r) r (a1, a2) ON ((l.a1 = r.a1))

 ->  Hash Join  (cost=272.13..272.69 rows=15 width=96)
   Output: foo.a, foo.*, bar.a, bar.*, baz.a, baz.*
   Hash Cond: (foo.a = baz.a)
   ->  Foreign Scan  (cost=100.00..100.04 rows=2 width=64)
 Output: foo.a, foo.*, bar.a, bar.*
 Relations: (public.foo) INNER JOIN (public.bar)
 Remote SQL: SELECT l.a1, l.a2, r.a1, r.a2 FROM 
(SELECT l.a9, ROW(l.a9) FROM (SELECT a a9 FROM public.foo FOR UPDATE) l) 
l (a1, a2)
INNER JOIN (SELECT r.a9, ROW(r.a9) FROM (SELECT a a9 FROM public.bar FOR 
UPDATE) r) r (a1, a2) ON ((l.a1 = 

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Mon, Dec 7, 2015 at 12:25 AM, Etsuro Fujita
 wrote:
>> Instead, I think we should go the opposite direction and pass the
>> outerplan to GetForeignPlan after all.  I was lulled into a full sense
>> of security by the realization that every FDW that uses this feature
>> MUST want to do outerPlan(scan_plan) = fdw_outerplan.  That's true,
>> but irrelevant.  The point is that the FDW might want to do something
>> additional, like frob the outer plan's tlist, and it can't do that if
>> we don't pass it fdw_outerplan.  So we should do that, after all.
>
> As I proposed upthread, another idea would be to 1) to store an
> fdw_outerpath in the fdw_private list of a ForeignPath node, and then 2) to
> create an fdw_outerplan from *the fdw_outerpath stored into
> the fdw_private* in GetForeignPlan.  One good thing for this is that we keep
> the API of create_foreignscan_path as-is.  What do you think about that?

I don't think it's a good idea, per what I said in the first paragraph
of this email:

http://www.postgresql.org/message-id/ca+tgmoz5g+zgph3stmgm6cwgtoywz3n1pjsw6lvhz31ofgl...@mail.gmail.com

I think the core system likely needs visibility into where paths and
plans are present in node trees, and putting them somewhere inside
fdw_private would be going in the opposite direction.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Tom Lane]

Robert Haas  writes:
> I think the core system likely needs visibility into where paths and
> plans are present in node trees, and putting them somewhere inside
> fdw_private would be going in the opposite direction.

Absolutely.  You don't really want FDWs having to take responsibility
for setrefs.c processing of their node trees, for example.  This is why
e.g. ForeignScan has both fdw_exprs and fdw_private.

I'm not too concerned about whether we have to adjust FDW-related APIs
as we go along.  It's been clear from the beginning that we'd have to
do that, and we are nowhere near a point where we should promise that
we're done doing so.

regards, tom lane


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/12/05 5:15, Robert Haas wrote:

On Tue, Dec 1, 2015 at 10:20 PM, Etsuro Fujita
 wrote:

One thing I can think of is that we can keep both the structure of a
ForeignPath node and the API of create_foreignscan_path as-is.  The latter
is a good thing for FDW authors.  And IIUC the patch you posted today, I
think we could make create_foreignscan_plan a bit simpler too.  Ie, in your
patch, you modified that function as follows:

@@ -2129,7 +2134,9 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath
*best_path,
  */
 scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid,

best_path,
-
tlist, scan_clauses);
+
tlist,
+
scan_clauses);
+   outerPlan(scan_plan) = fdw_outerplan;

I think that would be OK, but I think we would have to do a bit more here
about the fdw_outerplan's targetlist and qual; I think that the targetlist
needs to be changed to fdw_scan_tlist, as in the patch [1], and that it'd be
better to change the qual to remote conditions, ie, quals not in the
scan_plan's scan.plan.qual, to avoid duplicate evaluation of local
conditions.  (In the patch [1], I didn't do anything about the qual because
the current postgres_fdw join pushdown patch assumes that all the the
scan_plan's scan.plan.qual are pushed down.) Or, FDW authors might want to
do something about fdw_recheck_quals for a foreign-join while creating the
fdw_outerplan.  So if we do that during GetForeignPlan, I think we could
make create_foreignscan_plan a bit simpler, or provide flexibility to FDW
authors.



It's certainly true that we need the alternative plan's tlist to match
that of the main plan; otherwise, it's going to be difficult for the
FDW to make use of that alternative subplan to fill its slot, which is
kinda the point of all this.


OK.


However, I'm quite reluctant to
introduce code into create_foreignscan_plan() that forces the
subplan's tlist to match that of the main plan.  For one thing, that
would likely foreclose the possibility of an FDW ever using the outer
plan for any purpose other than EPQ rechecks.  It may be hard to
imagine what else you'd do with the outer plan as things are today,
but right now the two haves of the patch - letting FDWs have an outer
subplan, and providing them with a way of overriding the EPQ recheck
behavior - are technically independent.  Putting tlist-altering
behavior into create_foreignscan_plan() ties those two things together
irrevocably.


Agreed.


Instead, I think we should go the opposite direction and pass the
outerplan to GetForeignPlan after all.  I was lulled into a full sense
of security by the realization that every FDW that uses this feature
MUST want to do outerPlan(scan_plan) = fdw_outerplan.  That's true,
but irrelevant.  The point is that the FDW might want to do something
additional, like frob the outer plan's tlist, and it can't do that if
we don't pass it fdw_outerplan.  So we should do that, after all.


As I proposed upthread, another idea would be to 1) to store an 
fdw_outerpath in the fdw_private list of a ForeignPath node, and then 2) 
to create an fdw_outerplan from *the fdw_outerpath stored into
the fdw_private* in GetForeignPlan.  One good thing for this is that we 
keep the API of create_foreignscan_path as-is.  What do you think about 
that?



Updated patch attached.  This fixes a couple of whitespace issues that
were pointed out, also.


Thanks for updating the patch!

Best regards,
Etsuro Fujita




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Dec 1, 2015 at 10:20 PM, Etsuro Fujita
 wrote:
> One thing I can think of is that we can keep both the structure of a
> ForeignPath node and the API of create_foreignscan_path as-is.  The latter
> is a good thing for FDW authors.  And IIUC the patch you posted today, I
> think we could make create_foreignscan_plan a bit simpler too.  Ie, in your
> patch, you modified that function as follows:
>
> @@ -2129,7 +2134,9 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath
> *best_path,
>  */
> scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid,
>
> best_path,
> -
> tlist, scan_clauses);
> +
> tlist,
> +
> scan_clauses);
> +   outerPlan(scan_plan) = fdw_outerplan;
>
> I think that would be OK, but I think we would have to do a bit more here
> about the fdw_outerplan's targetlist and qual; I think that the targetlist
> needs to be changed to fdw_scan_tlist, as in the patch [1], and that it'd be
> better to change the qual to remote conditions, ie, quals not in the
> scan_plan's scan.plan.qual, to avoid duplicate evaluation of local
> conditions.  (In the patch [1], I didn't do anything about the qual because
> the current postgres_fdw join pushdown patch assumes that all the the
> scan_plan's scan.plan.qual are pushed down.) Or, FDW authors might want to
> do something about fdw_recheck_quals for a foreign-join while creating the
> fdw_outerplan.  So if we do that during GetForeignPlan, I think we could
> make create_foreignscan_plan a bit simpler, or provide flexibility to FDW
> authors.

It's certainly true that we need the alternative plan's tlist to match
that of the main plan; otherwise, it's going to be difficult for the
FDW to make use of that alternative subplan to fill its slot, which is
kinda the point of all this.  However, I'm quite reluctant to
introduce code into create_foreignscan_plan() that forces the
subplan's tlist to match that of the main plan.  For one thing, that
would likely foreclose the possibility of an FDW ever using the outer
plan for any purpose other than EPQ rechecks.  It may be hard to
imagine what else you'd do with the outer plan as things are today,
but right now the two haves of the patch - letting FDWs have an outer
subplan, and providing them with a way of overriding the EPQ recheck
behavior - are technically independent.  Putting tlist-altering
behavior into create_foreignscan_plan() ties those two things together
irrevocably.

Instead, I think we should go the opposite direction and pass the
outerplan to GetForeignPlan after all.  I was lulled into a full sense
of security by the realization that every FDW that uses this feature
MUST want to do outerPlan(scan_plan) = fdw_outerplan.  That's true,
but irrelevant.  The point is that the FDW might want to do something
additional, like frob the outer plan's tlist, and it can't do that if
we don't pass it fdw_outerplan.  So we should do that, after all.

Updated patch attached.  This fixes a couple of whitespace issues that
were pointed out, also.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 5ce8f90..83bbfa1 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -121,7 +121,8 @@ static ForeignScan *fileGetForeignPlan(PlannerInfo *root,
    Oid foreigntableid,
    ForeignPath *best_path,
    List *tlist,
-   List *scan_clauses);
+   List *scan_clauses,
+   Plan *outer_plan);
 static void fileExplainForeignScan(ForeignScanState *node, ExplainState *es);
 static void fileBeginForeignScan(ForeignScanState *node, int eflags);
 static TupleTableSlot *fileIterateForeignScan(ForeignScanState *node);
@@ -525,6 +526,7 @@ fileGetForeignPaths(PlannerInfo *root,
 	 total_cost,
 	 NIL,		/* no pathkeys */
 	 NULL,		/* no outer rel either */
+	 NULL,		/* no extra plan */
 	 coptions));
 
 	/*
@@ -544,7 +546,8 @@ fileGetForeignPlan(PlannerInfo *root,
    Oid foreigntableid,
    ForeignPath *best_path,
    List *tlist,
-   List *scan_clauses)
+   List *scan_clauses,
+   Plan *outer_plan)
 {
 	Index		scan_relid = baserel->relid;
 
@@ -564,7 +567,8 @@ fileGetForeignPlan(PlannerInfo *root,
 			NIL,	/* no expressions to evaluate */
 			best_path->fdw_private,
 			NIL,	/* no custom tlist */
-			NIL /* no remote quals */ );
+			NIL,	/* no remote quals */
+			outer_plan);
 }
 
 /*
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index a6ba672..9a014d4 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -214,7 +214,8 @@ static ForeignScan *postgresGetForeignPlan(PlannerInfo *root,
 	   Oid foreigntableid,
 	   ForeignPath *best_path,
 	   List *tlist,
-	   List *scan_clauses);
+	   List *scan_clauses,
+	   Plan 

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/12/02 1:54, Robert Haas wrote:

On Fri, Nov 27, 2015 at 1:25 AM, Kouhei Kaigai  wrote:

Sorry, I don't understand this.  In my understanding, fdw_recheck_quals
can be defined for a foreign join, regardless of the join type,



Yes, "can be defined", but will not be workable if either side of
joined tuple is NULL because of outer join. SQL functions returns
NULL prior to evaluation, then ExecQual() treats this result as FALSE.
However, a joined tuple that has NULL fields may be a valid tuple.

We don't need to care about unmatched tuple if INNER JOIN.



This is a really good point, and a very strong argument for the design
KaiGai has chosen here.


Maybe my explanation was not enough.  Sorry about that.  But I mean that 
we define fdw_recheck_quals for a foreign-join as quals that 1) were 
extracted by extract_actual_join_clauses as "otherclauses" 
(rinfo->is_pushed_down=true) and that 2) were pushed down to the remote 
server, not scan quals relevant to all the base tables invoved in the 
foreign-join.  So in this definition, I think fdw_recheck_quals for a 
foreign-join will be workable, regardless of the join type.


Best regards,
Etsuro Fujita




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/12/02 14:54, Kouhei Kaigai wrote:

On 2015/12/02 1:41, Robert Haas wrote:

On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita
 wrote:

The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
FDW driver can set arbitrary but one path-node here.
After that, this path-node shall be transformed to plan-node by
createplan.c, then passed to FDW driver using GetForeignPlan callback.



I understand this, as I also did the same thing in my patches, but actually,
that seems a bit complicated to me.  Instead, could we keep the
fdw_outerpath in the fdw_private of a ForeignPath node when creating the
path node during GetForeignPaths, and then create an outerplan accordingly
from the fdw_outerpath stored into the fdw_private during GetForeignPlan, by
using create_plan_recurse there?  I think that that would make the core
involvment much simpler.



I can't see how it's going to get much simpler than this.  The core
core is well under a hundred lines, and it all looks pretty
straightforward to me.  All of our existing path and plan types keep
lists of paths and plans separate from other kinds of data, and I
don't think we're going to win any awards for deviating from that
principle here.



One thing I can think of is that we can keep both the structure of a
ForeignPath node and the API of create_foreignscan_path as-is.  The
latter is a good thing for FDW authors.  And IIUC the patch you posted
today, I think we could make create_foreignscan_plan a bit simpler too.
   Ie, in your patch, you modified that function as follows:

@@ -2129,7 +2134,9 @@ create_foreignscan_plan(PlannerInfo *root,
ForeignPath *best_path,
 */
scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid,

best_path,
-
tlist, scan_clauses);
+
tlist,
+
scan_clauses);
+   outerPlan(scan_plan) = fdw_outerplan;

I think that would be OK, but I think we would have to do a bit more
here about the fdw_outerplan's targetlist and qual; I think that the
targetlist needs to be changed to fdw_scan_tlist, as in the patch [1],



Hmm... you are right. The sub-plan shall generate a tuple according to
the fdw_scan_tlist, if valid. Do you think the surgical operation is best
to apply alternative target-list than build_path_tlist()?


Sorry, I'm not sure about that.  I thought changing it to fdw_scan_tlist 
just because that's simple.



and that it'd be better to change the qual to remote conditions, ie,
quals not in the scan_plan's scan.plan.qual, to avoid duplicate
evaluation of local conditions.  (In the patch [1], I didn't do anything
about the qual because the current postgres_fdw join pushdown patch
assumes that all the the scan_plan's scan.plan.qual are pushed down.)
Or, FDW authors might want to do something about fdw_recheck_quals for a
foreign-join while creating the fdw_outerplan.  So if we do that during
GetForeignPlan, I think we could make create_foreignscan_plan a bit
simpler, or provide flexibility to FDW authors.



So, you suggest it is better to pass fdw_outerplan on the GetForeignPlan
callback, to allow FDW to adjust target-list and quals of sub-plans.


I think that is one option for us.  Another option, which I proposed 
above, is 1) store an fdw_outerpath in the fdw_private when creating the 
ForeignPath node in GetForeignPaths, and then 2) create an fdw_outerplan 
from the fdw_outerpath stored into the fdw_private when creating the 
ForeignScan node in GetForeignPlan, by using create_plan_recurse in 
GetForeignPlan.  (To do so, I was thinking to make that function 
extern.)  One good point about that is that we can keep the API of 
create_foreignscan_path as-is, which I think would be a good thing for 
FDW authors that don't care about join pushdown.



I think it is reasonable argue. Only FDW knows which qualifiers are
executable on remote side, so it is not easy to remove qualifiers to be
executed on host-side only, from the sub-plan tree.


Yeah, we could provide the flexibility to FDW authors.


@@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
*slot)

  ResetExprContext(econtext);

+   /*
+* FDW driver has to recheck visibility of EPQ tuple towards
+* the scan qualifiers once it gets pushed down.
+* In addition, if this node represents a join sub-tree, not
+* a scan, FDW driver is also responsible to reconstruct
+* a joined tuple according to the primitive EPQ tuples.
+*/
+   if (fdwroutine->RecheckForeignScan)
+   {
+   if (!fdwroutine->RecheckForeignScan(node, slot))
+   return false;
+   }

Maybe I'm missing something, but I think we should let FDW do the work if
scanrelid==0, not just if fdwroutine->RecheckForeignScan is given. (And if
scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we should
abort the transaction.)



That 

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/12/02 1:53, Robert Haas wrote:

On Fri, Nov 27, 2015 at 1:33 AM, Etsuro Fujita
 wrote: Plan   *plan =
>scan.plan;

@@ -3755,7 +3763,7 @@ make_foreignscan(List *qptlist,
 /* cost will be filled in by create_foreignscan_plan */
 plan->targetlist = qptlist;
 plan->qual = qpqual;
-   plan->lefttree = NULL;
+   plan->lefttree = fdw_outerplan;
 plan->righttree = NULL;
 node->scan.scanrelid = scanrelid;

I think that that would break the EXPLAIN output.



In what way?  EXPLAIN recurses into the left and right trees of every
plan node regardless of what type it is, so superficially I feel like
this ought to just work. What problem do you foresee?

I do think that ExecInitForeignScan ought to be changed to
ExecInitNode on it's outer plan if present rather than leaving that to
the FDW's BeginForeignScan method.


IIUC, I think the EXPLAIN output for eg,

select localtab.* from localtab, ft1, ft2 where localtab.a = ft1.a and 
ft1.a = ft2.a for update


would be something like this:

 LockRows
   ->  Nested Loop
 Join Filter: (ft1.a = localtab.a)
 ->  Seq Scan on localtab
 ->  Foreign Scan on ft1/ft2-foreign-join
   -> Nested Loop
Join Filter: (ft1.a = ft2.a)
->  Foreign Scan on ft1
->  Foreign Scan on ft2

The subplan below the Foreign Scan on the foreign-join seems odd to me. 
 One option to avoid that is to handle the subplan as in my patch [2], 
which I created to address your comment that we should not break the 
equivalence discussed below.  I'm not sure that the patch's handling of 
chgParam for the subplan is a good idea, though.



One option to avoid that
is to set the fdw_outerplan in ExecInitForeignScan as in my patch [1], or
BeginForeignScan as you proposed.  That breaks the equivalence that the Plan
tree and the PlanState tree should be mirror images of each other, but I
think that that break would be harmless.



I'm not sure how many times I have to say this, but we are not doing
that.  I will not commit any patch that does that, and I will
vigorously argue against anyone else committing such a patch either.
That *would* break EXPLAIN, because EXPLAIN relies on being able to
walk the PlanState tree and find all the Plan nodes from the
corresponding PlanState nodes.  Now you might think that it would be
OK to omit a plan node that we decided we weren't ever going to
execute, but today we don't do that, and I don't think we should.  I
think it could be very confusing if EXPLAIN and EXPLAIN ANALYZE show
different sets of plan nodes for the same query.  Quite apart from
EXPLAIN, there are numerous other places that assume that they can
walk the PlanState tree and find all the Plan nodes.  Breaking that
assumption would be bad news.


Agreed.  Thanks for the explanation!

Best regards,
Etsuro Fujita

[2] http://www.postgresql.org/message-id/5624d583.10...@lab.ntt.co.jp




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kyotaro HORIGUCHI]

Hello, thank you for taking time for this.

At Tue, 1 Dec 2015 14:56:54 -0500, Robert Haas  wrote in 

> On Thu, Nov 26, 2015 at 12:04 AM, Kouhei Kaigai  wrote:
> > This patch is not tested by actual FDW extensions, so it is helpful
> > to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.
> 
> I have done some editing and some small revisions on this patch.
> Here's what I came up with.  The revisions are mostly cosmetic, but I
> revised it a bit so that the signature of GetForeignPlan need not
> change.  Also, I made nodeForeignScan.c do some of the outer plan
> handling automatically, and I fixed the compile breaks in
> contrib/file_fdw and contrib/postgres_fdw.
> 
> Comments/review/testing are very welcome.

Applied on HEAD with no error. Regtests of core, postgres_fdw and
file_fdw finished with no error. (I haven't done any further testing)


nodeScan.c:

  The comments in nodeScan.c looks way clearer. Thank you for rewriting.

nodeForeignscan.c:

 Is this a mistake?

 > @@ -205,6 +218,11 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, 
 > int eflags)
 >  scanstate->fdwroutine = fdwroutine;
 >  scanstate->fdw_state = NULL;
 >  
 > +/* Initialize any outer plan. */
-> +if (outerPlanState(scanstate))
+> +if (outerPlanState(node))
 > +outerPlanState(scanstate) =

createplan.c, planmain.h:

 I agree with reverting the signature of GetForeignPlan.

fdwapi.h:

 The reverting of the additional parameter of ForeignScan leaves
 only change of indentation of the last parameter.

fdwhandler.sgml:

 This is easy to understand to me. Thank you.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kyotaro HORIGUCHI]

Sorry, I made a mistake.

At Wed, 02 Dec 2015 10:29:17 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20151202.102917.50152198.horiguchi.kyot...@lab.ntt.co.jp>
> Hello, thank you for editing.
> 
> At Tue, 1 Dec 2015 14:56:54 -0500, Robert Haas  wrote 
> in 
> > On Thu, Nov 26, 2015 at 12:04 AM, Kouhei Kaigai  
> > wrote:
> > > This patch is not tested by actual FDW extensions, so it is helpful
> > > to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.
> > 
> > I have done some editing and some small revisions on this patch.
> > Here's what I came up with.  The revisions are mostly cosmetic, but I
> > revised it a bit so that the signature of GetForeignPlan need not
> > change.  Also, I made nodeForeignScan.c do some of the outer plan
> > handling automatically, and I fixed the compile breaks in
> > contrib/file_fdw and contrib/postgres_fdw.
> > 
> > Comments/review/testing are very welcome.
> 
> Applied on HEAD with no error. Regtests of core, postgres_fdw and
> file_fdw finished with no error.
> 
> 
> nodeScan.c:
> 
>   The comments in nodeScan.c looks way clearer. Thank you for rewriting.
> 
> nodeForeignscan.c:
> 
>  Is this a mistake?
> 
>  > @@ -205,6 +218,11 @@ ExecInitForeignScan(ForeignScan *node, EState 
> *estate, int eflags)
>  >scanstate->fdwroutine = fdwroutine;
>  >scanstate->fdw_state = NULL;
>  >  
>  > +  /* Initialize any outer plan. */
> -> +  if (outerPlanState(scanstate))
> +> +  if (outerPlanState(node))
>  > +  outerPlanState(scanstate) =

No, the above is wrong.

-> +if (outerPlanState(scanstate))
+> +if (outerPlan(node))
 > +outerPlanState(scanstate) =

> createplan.c, planmain.h:
> 
>  I agree with reverting the signature of GetForeignPlan.
> 
> fdwapi.h:
> 
>  The reverting of the additional parameter of ForeignScan leaves
>  only change of indentation of the last parameter.
> 
> fdwhandler.sgml:
> 
>  This is easy to understand to me. Thank you.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/12/02 1:41, Robert Haas wrote:

On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita
 wrote:

The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
FDW driver can set arbitrary but one path-node here.
After that, this path-node shall be transformed to plan-node by
createplan.c, then passed to FDW driver using GetForeignPlan callback.



I understand this, as I also did the same thing in my patches, but actually,
that seems a bit complicated to me.  Instead, could we keep the
fdw_outerpath in the fdw_private of a ForeignPath node when creating the
path node during GetForeignPaths, and then create an outerplan accordingly
from the fdw_outerpath stored into the fdw_private during GetForeignPlan, by
using create_plan_recurse there?  I think that that would make the core
involvment much simpler.



I can't see how it's going to get much simpler than this.  The core
core is well under a hundred lines, and it all looks pretty
straightforward to me.  All of our existing path and plan types keep
lists of paths and plans separate from other kinds of data, and I
don't think we're going to win any awards for deviating from that
principle here.


One thing I can think of is that we can keep both the structure of a 
ForeignPath node and the API of create_foreignscan_path as-is.  The 
latter is a good thing for FDW authors.  And IIUC the patch you posted 
today, I think we could make create_foreignscan_plan a bit simpler too. 
 Ie, in your patch, you modified that function as follows:


@@ -2129,7 +2134,9 @@ create_foreignscan_plan(PlannerInfo *root, 
ForeignPath *best_path,

 */
scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid,

best_path,
-   
tlist, scan_clauses);
+   
tlist,
+   
scan_clauses);
+   outerPlan(scan_plan) = fdw_outerplan;

I think that would be OK, but I think we would have to do a bit more 
here about the fdw_outerplan's targetlist and qual; I think that the 
targetlist needs to be changed to fdw_scan_tlist, as in the patch [1], 
and that it'd be better to change the qual to remote conditions, ie, 
quals not in the scan_plan's scan.plan.qual, to avoid duplicate 
evaluation of local conditions.  (In the patch [1], I didn't do anything 
about the qual because the current postgres_fdw join pushdown patch 
assumes that all the the scan_plan's scan.plan.qual are pushed down.) 
Or, FDW authors might want to do something about fdw_recheck_quals for a 
foreign-join while creating the fdw_outerplan.  So if we do that during 
GetForeignPlan, I think we could make create_foreignscan_plan a bit 
simpler, or provide flexibility to FDW authors.



@@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
*slot)

 ResetExprContext(econtext);

+   /*
+* FDW driver has to recheck visibility of EPQ tuple towards
+* the scan qualifiers once it gets pushed down.
+* In addition, if this node represents a join sub-tree, not
+* a scan, FDW driver is also responsible to reconstruct
+* a joined tuple according to the primitive EPQ tuples.
+*/
+   if (fdwroutine->RecheckForeignScan)
+   {
+   if (!fdwroutine->RecheckForeignScan(node, slot))
+   return false;
+   }

Maybe I'm missing something, but I think we should let FDW do the work if
scanrelid==0, not just if fdwroutine->RecheckForeignScan is given. (And if
scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we should
abort the transaction.)



That would be unnecessarily restrictive.  On the one hand, even if
scanrelid != 0, the FDW can decide that it prefers to do the rechecks
using RecheckForeignScan rather than fdw_recheck_quals.  For most
FDWs, I expect using fdw_recheck_quals to be more convenient, but
there may be cases where somebody prefers to use RecheckForeignScan,
and allowing that costs nothing.


I suppose that the flexibility would probably be a good thing, but I'm a 
little bit concerned that that might be rather confusing to FDW authors. 
 Maybe I'm missing something, though.


Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/5624d583.10...@lab.ntt.co.jp




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Fri, Nov 27, 2015 at 1:33 AM, Etsuro Fujita
 wrote: Plan   *plan =
>scan.plan;
> @@ -3755,7 +3763,7 @@ make_foreignscan(List *qptlist,
> /* cost will be filled in by create_foreignscan_plan */
> plan->targetlist = qptlist;
> plan->qual = qpqual;
> -   plan->lefttree = NULL;
> +   plan->lefttree = fdw_outerplan;
> plan->righttree = NULL;
> node->scan.scanrelid = scanrelid;
>
> I think that that would break the EXPLAIN output.

In what way?  EXPLAIN recurses into the left and right trees of every
plan node regardless of what type it is, so superficially I feel like
this ought to just work. What problem do you foresee?

I do think that ExecInitForeignScan ought to be changed to
ExecInitNode on it's outer plan if present rather than leaving that to
the FDW's BeginForeignScan method.

> One option to avoid that
> is to set the fdw_outerplan in ExecInitForeignScan as in my patch [1], or
> BeginForeignScan as you proposed.  That breaks the equivalence that the Plan
> tree and the PlanState tree should be mirror images of each other, but I
> think that that break would be harmless.

I'm not sure how many times I have to say this, but we are not doing
that.  I will not commit any patch that does that, and I will
vigorously argue against anyone else committing such a patch either.
That *would* break EXPLAIN, because EXPLAIN relies on being able to
walk the PlanState tree and find all the Plan nodes from the
corresponding PlanState nodes.  Now you might think that it would be
OK to omit a plan node that we decided we weren't ever going to
execute, but today we don't do that, and I don't think we should.  I
think it could be very confusing if EXPLAIN and EXPLAIN ANALYZE show
different sets of plan nodes for the same query.  Quite apart from
EXPLAIN, there are numerous other places that assume that they can
walk the PlanState tree and find all the Plan nodes.  Breaking that
assumption would be bad news.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Fri, Nov 27, 2015 at 1:25 AM, Kouhei Kaigai  wrote:
>> Sorry, I don't understand this.  In my understanding, fdw_recheck_quals
>> can be defined for a foreign join, regardless of the join type,
>>
> Yes, "can be defined", but will not be workable if either side of
> joined tuple is NULL because of outer join. SQL functions returns
> NULL prior to evaluation, then ExecQual() treats this result as FALSE.
> However, a joined tuple that has NULL fields may be a valid tuple.
>
> We don't need to care about unmatched tuple if INNER JOIN.

This is a really good point, and a very strong argument for the design
KaiGai has chosen here.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Tom Lane]

Robert Haas  writes:
> On Fri, Nov 27, 2015 at 1:33 AM, Etsuro Fujita
>> One option to avoid that
>> is to set the fdw_outerplan in ExecInitForeignScan as in my patch [1], or
>> BeginForeignScan as you proposed.  That breaks the equivalence that the Plan
>> tree and the PlanState tree should be mirror images of each other, but I
>> think that that break would be harmless.

> I'm not sure how many times I have to say this, but we are not doing
> that.  I will not commit any patch that does that, and I will
> vigorously argue against anyone else committing such a patch either.

And I'll back him up.  That's a horrible idea.  You're proposing to break
a very fundamental structural property for the convenience of one little
corner of the system.

regards, tom lane


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Thu, Nov 26, 2015 at 12:04 AM, Kouhei Kaigai  wrote:
> > The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
> > FDW driver can set arbitrary but one path-node here.
> > After that, this path-node shall be transformed to plan-node by
> > createplan.c, then passed to FDW driver using GetForeignPlan callback.
> > We expect FDW driver set this plan-node on lefttree (a.k.a outerPlan).
> > The Plan->outerPlan is a common field, so patch size become relatively
> > small. FDW driver can initialize this plan at BeginForeignScan, then
> > execute this sub-plan-tree on demand.
> >
> > Remaining portions are as previous version. ExecScanFetch is revised
> > to call recheckMtd always when scanrelid==0, then FDW driver can get
> > control using RecheckForeignScan callback.
> > It allows FDW driver to handle (1) EPQ recheck on underlying scan nodes,
> > (2) reconstruction of joined tuple, and (3) EPQ recheck on join clauses,
> > by its preferable implementation - including execution of an alternative
> > sub-plan.
> >
> >> There seems to be no changes to make_foreignscan.  Is that OK?
> >>
> > create_foreignscan_path(), not only make_foreignscan().
> >
> > This patch is not tested by actual FDW extensions, so it is helpful
> > to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.
> 
> I have done some editing and some small revisions on this patch.
> Here's what I came up with.  The revisions are mostly cosmetic, but I
> revised it a bit so that the signature of GetForeignPlan need not
> change.
>
Thanks for the revising. (I could not be online for a few days, sorry.)

> Also, I made nodeForeignScan.c do some of the outer plan
> handling automatically,
>
It's OK for me. We may omit initialization/shutdown of sub-plan when
it is not actually needed, even if FDW driver set up. However, it is
very tiny advantage.

> and I fixed the compile breaks in
> contrib/file_fdw and contrib/postgres_fdw.
>
Sorry, I didn't fix up contrib side.

> Comments/review/testing are very welcome.
>
One small point:

@@ -3755,7 +3762,6 @@ make_foreignscan(List *qptlist,
/* cost will be filled in by create_foreignscan_plan */
plan->targetlist = qptlist;
plan->qual = qpqual;
-   plan->lefttree = NULL;
plan->righttree = NULL;
node->scan.scanrelid = scanrelid;
/* fs_server will be filled in by create_foreignscan_plan */

Although it is harmless, I prefer this line is kept because caller
of make_foreignscan() expects a ForeignScan node with empty lefttree,
even if it is filled up later.

Best regards,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On 2015/12/02 1:41, Robert Haas wrote:
> > On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita
> >  wrote:
> >>> The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
> >>> FDW driver can set arbitrary but one path-node here.
> >>> After that, this path-node shall be transformed to plan-node by
> >>> createplan.c, then passed to FDW driver using GetForeignPlan callback.
> 
> >> I understand this, as I also did the same thing in my patches, but 
> >> actually,
> >> that seems a bit complicated to me.  Instead, could we keep the
> >> fdw_outerpath in the fdw_private of a ForeignPath node when creating the
> >> path node during GetForeignPaths, and then create an outerplan accordingly
> >> from the fdw_outerpath stored into the fdw_private during GetForeignPlan, 
> >> by
> >> using create_plan_recurse there?  I think that that would make the core
> >> involvment much simpler.
> 
> > I can't see how it's going to get much simpler than this.  The core
> > core is well under a hundred lines, and it all looks pretty
> > straightforward to me.  All of our existing path and plan types keep
> > lists of paths and plans separate from other kinds of data, and I
> > don't think we're going to win any awards for deviating from that
> > principle here.
> 
> One thing I can think of is that we can keep both the structure of a
> ForeignPath node and the API of create_foreignscan_path as-is.  The
> latter is a good thing for FDW authors.  And IIUC the patch you posted
> today, I think we could make create_foreignscan_plan a bit simpler too.
>   Ie, in your patch, you modified that function as follows:
> 
> @@ -2129,7 +2134,9 @@ create_foreignscan_plan(PlannerInfo *root,
> ForeignPath *best_path,
>*/
>   scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid,
> 
>   best_path,
> -
>   tlist, scan_clauses);
> +
>   tlist,
> +
>   scan_clauses);
> + outerPlan(scan_plan) = fdw_outerplan;
> 
> I think that would be OK, but I think we would have to do a bit more
> here about the fdw_outerplan's targetlist and qual; I think that the
> targetlist needs to be changed to fdw_scan_tlist, as in the patch [1],
>
Hmm... you are right. The sub-plan shall generate a tuple according to
the fdw_scan_tlist, if valid. Do you think the surgical operation is best
to apply alternative target-list than build_path_tlist()?

> and that it'd be better to change the qual to remote conditions, ie,
> quals not in the scan_plan's scan.plan.qual, to avoid duplicate
> evaluation of local conditions.  (In the patch [1], I didn't do anything
> about the qual because the current postgres_fdw join pushdown patch
> assumes that all the the scan_plan's scan.plan.qual are pushed down.)
> Or, FDW authors might want to do something about fdw_recheck_quals for a
> foreign-join while creating the fdw_outerplan.  So if we do that during
> GetForeignPlan, I think we could make create_foreignscan_plan a bit
> simpler, or provide flexibility to FDW authors.
>
So, you suggest it is better to pass fdw_outerplan on the GetForeignPlan
callback, to allow FDW to adjust target-list and quals of sub-plans.
I think it is reasonable argue. Only FDW knows which qualifiers are
executable on remote side, so it is not easy to remove qualifiers to be
executed on host-side only, from the sub-plan tree.

> >> @@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
> >> *slot)
> >>
> >>  ResetExprContext(econtext);
> >>
> >> +   /*
> >> +* FDW driver has to recheck visibility of EPQ tuple towards
> >> +* the scan qualifiers once it gets pushed down.
> >> +* In addition, if this node represents a join sub-tree, not
> >> +* a scan, FDW driver is also responsible to reconstruct
> >> +* a joined tuple according to the primitive EPQ tuples.
> >> +*/
> >> +   if (fdwroutine->RecheckForeignScan)
> >> +   {
> >> +   if (!fdwroutine->RecheckForeignScan(node, slot))
> >> +   return false;
> >> +   }
> >>
> >> Maybe I'm missing something, but I think we should let FDW do the work if
> >> scanrelid==0, not just if fdwroutine->RecheckForeignScan is given. (And if
> >> scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we should
> >> abort the transaction.)
> 
> > That would be unnecessarily restrictive.  On the one hand, even if
> > scanrelid != 0, the FDW can decide that it prefers to do the rechecks
> > using RecheckForeignScan rather than fdw_recheck_quals.  For most
> > FDWs, I expect using fdw_recheck_quals to be more convenient, but
> > there may be cases where somebody prefers to use RecheckForeignScan,
> > and allowing that costs nothing.
> 
> I suppose that the flexibility would probably be a good thing, but I'm a
> little bit concerned that that might be rather confusing to FDW authors.
>
We expect FDW authors, like 

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Thu, Nov 26, 2015 at 12:04 AM, Kouhei Kaigai  wrote:
> The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
> FDW driver can set arbitrary but one path-node here.
> After that, this path-node shall be transformed to plan-node by
> createplan.c, then passed to FDW driver using GetForeignPlan callback.
> We expect FDW driver set this plan-node on lefttree (a.k.a outerPlan).
> The Plan->outerPlan is a common field, so patch size become relatively
> small. FDW driver can initialize this plan at BeginForeignScan, then
> execute this sub-plan-tree on demand.
>
> Remaining portions are as previous version. ExecScanFetch is revised
> to call recheckMtd always when scanrelid==0, then FDW driver can get
> control using RecheckForeignScan callback.
> It allows FDW driver to handle (1) EPQ recheck on underlying scan nodes,
> (2) reconstruction of joined tuple, and (3) EPQ recheck on join clauses,
> by its preferable implementation - including execution of an alternative
> sub-plan.
>
>> There seems to be no changes to make_foreignscan.  Is that OK?
>>
> create_foreignscan_path(), not only make_foreignscan().
>
> This patch is not tested by actual FDW extensions, so it is helpful
> to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.

I have done some editing and some small revisions on this patch.
Here's what I came up with.  The revisions are mostly cosmetic, but I
revised it a bit so that the signature of GetForeignPlan need not
change.  Also, I made nodeForeignScan.c do some of the outer plan
handling automatically, and I fixed the compile breaks in
contrib/file_fdw and contrib/postgres_fdw.

Comments/review/testing are very welcome.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 5ce8f90..1966b51 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -525,6 +525,7 @@ fileGetForeignPaths(PlannerInfo *root,
 	 total_cost,
 	 NIL,		/* no pathkeys */
 	 NULL,		/* no outer rel either */
+	 NULL,		/* no extra plan */
 	 coptions));
 
 	/*
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index a6ba672..dd63159 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -535,6 +535,7 @@ postgresGetForeignPaths(PlannerInfo *root,
    fpinfo->total_cost,
    NIL, /* no pathkeys */
    NULL,		/* no outer rel either */
+   NULL,		/* no extra plan */
    NIL);		/* no fdw_private list */
 	add_path(baserel, (Path *) path);
 
@@ -589,6 +590,7 @@ postgresGetForeignPaths(PlannerInfo *root,
 		 total_cost,
 		 usable_pathkeys,
 		 NULL,
+		 NULL,
 		 NIL));
 	}
 
@@ -756,6 +758,7 @@ postgresGetForeignPaths(PlannerInfo *root,
 	   total_cost,
 	   NIL,		/* no pathkeys */
 	   param_info->ppi_req_outer,
+	   NULL,
 	   NIL);	/* no fdw_private list */
 		add_path(baserel, (Path *) path);
 	}
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 1533a6b..a646b2a 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -765,6 +765,35 @@ RefetchForeignRow (EState *estate,
  See  for more information.
 
 
+
+
+bool
+RecheckForeignScan (ForeignScanState *node, TupleTableSlot *slot);
+
+ Recheck that a previously-returned tuple still matches the relevant
+ scan and join qualifiers, and possibly provide a modified version of
+ the tuple.  For foreign data wrappers which do not perform join pushdown,
+ it will typically be more convenient to set this to NULL and
+ instead set fdw_recheck_quals appropriately.
+ When outer joins are pushed down, however, it isn't sufficient to
+ reapply the checks relevant to all the base tables to the result tuple,
+ even if all needed attributes are present, because failure to match some
+ qualifier might result in some attributes going to NULL, rather than in
+ no tuple being returned.  RecheckForeignScan can recheck
+ qualifiers and return true if they are still satisfied and false
+ otherwise, but it can also store a replacement tuple into the supplied
+ slot.
+
+
+
+ To implement join pushdown, a foreign data wrapper will typically
+ construct an alternative local join plan which is used only for
+ rechecks; this will become the outer subplan of the
+ ForeignScan.  When a recheck is required, this subplan
+ can be executed and the resulting tuple can be stored in the slot.
+ This plan need not be efficient since no base table will return more
+ that one row; for example, it may implement all joins as nested loops.
+

 

@@ -1137,11 +1166,17 @@ GetForeignServerByName(const char *name, bool missing_ok);
 
 
  

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita
 wrote:
>> The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
>> FDW driver can set arbitrary but one path-node here.
>> After that, this path-node shall be transformed to plan-node by
>> createplan.c, then passed to FDW driver using GetForeignPlan callback.
>
> I understand this, as I also did the same thing in my patches, but actually,
> that seems a bit complicated to me.  Instead, could we keep the
> fdw_outerpath in the fdw_private of a ForeignPath node when creating the
> path node during GetForeignPaths, and then create an outerplan accordingly
> from the fdw_outerpath stored into the fdw_private during GetForeignPlan, by
> using create_plan_recurse there?  I think that that would make the core
> involvment much simpler.

I can't see how it's going to get much simpler than this.  The core
core is well under a hundred lines, and it all looks pretty
straightforward to me.  All of our existing path and plan types keep
lists of paths and plans separate from other kinds of data, and I
don't think we're going to win any awards for deviating from that
principle here.

> @@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
> *slot)
>
> ResetExprContext(econtext);
>
> +   /*
> +* FDW driver has to recheck visibility of EPQ tuple towards
> +* the scan qualifiers once it gets pushed down.
> +* In addition, if this node represents a join sub-tree, not
> +* a scan, FDW driver is also responsible to reconstruct
> +* a joined tuple according to the primitive EPQ tuples.
> +*/
> +   if (fdwroutine->RecheckForeignScan)
> +   {
> +   if (!fdwroutine->RecheckForeignScan(node, slot))
> +   return false;
> +   }
>
> Maybe I'm missing something, but I think we should let FDW do the work if
> scanrelid==0, not just if fdwroutine->RecheckForeignScan is given. (And if
> scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we should
> abort the transaction.)

That would be unnecessarily restrictive.  On the one hand, even if
scanrelid != 0, the FDW can decide that it prefers to do the rechecks
using RecheckForeignScan rather than fdw_recheck_quals.  For most
FDWs, I expect using fdw_recheck_quals to be more convenient, but
there may be cases where somebody prefers to use RecheckForeignScan,
and allowing that costs nothing.  On the flip side, an FDW could
choose to support join pushdown but not worry about EPQ rechecks: it
can just refuse to push down joins when any rowmarks are present.
Requiring the FDW author to supply a dummy RecheckForeignScan method
in that case is pointless.  So I think KaiGai's check is exactly
right.

> Another thing I'm concerned about is
>
> @@ -347,8 +355,26 @@ ExecScanReScan(ScanState *node)
> {
> Index   scanrelid = ((Scan *)
> node->ps.plan)->scanrelid;
>
> -   Assert(scanrelid > 0);
> +   if (scanrelid > 0)
> +   estate->es_epqScanDone[scanrelid - 1] = false;
> +   else
> +   {
> +   Bitmapset  *relids;
> +   int rtindex = -1;
> +
> +   if (IsA(node->ps.plan, ForeignScan))
> +   relids = ((ForeignScan *)
> node->ps.plan)->fs_relids;
> +   else if (IsA(node->ps.plan, CustomScan))
> +   relids = ((CustomScan *)
> node->ps.plan)->custom_relids;
> +   else
> +   elog(ERROR, "unexpected scan node: %d",
> +(int)nodeTag(node->ps.plan));
>
> -   estate->es_epqScanDone[scanrelid - 1] = false;
> +   while ((rtindex = bms_next_member(relids, rtindex))
>>= 0)
> +   {
> +   Assert(rtindex > 0);
> +   estate->es_epqScanDone[rtindex - 1] = false;
> +   }
> +   }
> }
>
> That seems the outerplan's business to me, so I think it'd be better to just
> return, right before the assertion, as I said before.  Seen from another
> angle, ISTM that FDWs that don't use a local join execution plan wouldn't
> need to be aware of handling the es_epqScanDone flags.  (Do you think that
> such FDWs should do something like what ExecScanFtch is doing about the
> flags, in their RecheckForeignScans?  If so, I think we need docs for that.)

I noticed this too when reviewing KaiGai's patch, but ultimately I
think the way KaiGai has it is fine.  It may not be useful in some
cases, but AFAICS it should be harmless.

>> This patch is not tested by actual FDW extensions, so it is helpful
>> to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.
>
> Will do.

That would be great.

-- 
Robert Haas

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/27 0:14, Kouhei Kaigai wrote:

>> The documentation says as following so I think the former has.
>>
>> # I don't understhad what 'can or must' means, though... 'can and
>> # must'?
>>
>> + Also, this callback can or must recheck scan qualifiers and join
>> + conditions which are pushed down. Especially, it needs special

> If fdw_recheck_quals is set up correctly and join type is inner join,
> FDW driver does not recheck by itself. Elsewhere, it has to recheck
> the joined tuple, not only reconstruction.

Sorry, I don't understand this.  In my understanding, fdw_recheck_quals
can be defined for a foreign join, regardless of the join type, and when
the fdw_recheck_quals are defined, the RecheckForeignScan callback
routine doesn't need to evaluate the fdw_recheck_quals by itself.  No?

Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/27 0:14, Kouhei Kaigai wrote:

On 2015/11/26 14:04, Kouhei Kaigai wrote:

The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
FDW driver can set arbitrary but one path-node here.
After that, this path-node shall be transformed to plan-node by
createplan.c, then passed to FDW driver using GetForeignPlan callback.



I understand this, as I also did the same thing in my patches, but
actually, that seems a bit complicated to me.  Instead, could we keep
the fdw_outerpath in the fdw_private of a ForeignPath node when creating
the path node during GetForeignPaths, and then create an outerplan
accordingly from the fdw_outerpath stored into the fdw_private during
GetForeignPlan, by using create_plan_recurse there?  I think that that
would make the core involvment much simpler.



How to use create_plan_recurse by extension? It is a static function.


I was just thinking a change to make that function extern.


We expect FDW driver set this plan-node on lefttree (a.k.a outerPlan).
The Plan->outerPlan is a common field, so patch size become relatively
small. FDW driver can initialize this plan at BeginForeignScan, then
execute this sub-plan-tree on demand.



Another idea would be to add the core support for
initializing/closing/rescanning the outerplan tree when the tree is given.



No. Please don't repeat same discussion again.


IIUC, I think your point is to allow FDWs to do something else, instead 
of performing a local join execution plan, during RecheckForeignScan. 
So, what's wrong with the core doing that support in that case?



@@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
*slot)

ResetExprContext(econtext);

+   /*
+* FDW driver has to recheck visibility of EPQ tuple towards
+* the scan qualifiers once it gets pushed down.
+* In addition, if this node represents a join sub-tree, not
+* a scan, FDW driver is also responsible to reconstruct
+* a joined tuple according to the primitive EPQ tuples.
+*/
+   if (fdwroutine->RecheckForeignScan)
+   {
+   if (!fdwroutine->RecheckForeignScan(node, slot))
+   return false;
+   }

Maybe I'm missing something, but I think we should let FDW do the work
if scanrelid==0, not just if fdwroutine->RecheckForeignScan is given.
(And if scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we
should abort the transaction.)



It should be Assert(). The node with scanrelid==0 never happen
unless FDW driver does not add such a path explicitly.


That's an idea.  But the abort seems to me more consistent with other 
places (see eg, RefetchForeignRow in EvalPlanQualFetchRowMarks).



Another thing I'm concerned about is

@@ -347,8 +355,26 @@ ExecScanReScan(ScanState *node)
{
Index   scanrelid = ((Scan *)
node->ps.plan)->scanrelid;

-   Assert(scanrelid > 0);
+   if (scanrelid > 0)
+   estate->es_epqScanDone[scanrelid - 1] = false;
+   else
+   {
+   Bitmapset  *relids;
+   int rtindex = -1;
+
+   if (IsA(node->ps.plan, ForeignScan))
+   relids = ((ForeignScan *)
node->ps.plan)->fs_relids;
+   else if (IsA(node->ps.plan, CustomScan))
+   relids = ((CustomScan *)
node->ps.plan)->custom_relids;
+   else
+   elog(ERROR, "unexpected scan node: %d",
+(int)nodeTag(node->ps.plan));

-   estate->es_epqScanDone[scanrelid - 1] = false;
+   while ((rtindex = bms_next_member(relids, rtindex)) >=
0)
+   {
+   Assert(rtindex > 0);
+   estate->es_epqScanDone[rtindex - 1] = false;
+   }
+   }
}

That seems the outerplan's business to me, so I think it'd be better to
just return, right before the assertion, as I said before.  Seen from
another angle, ISTM that FDWs that don't use a local join execution plan
wouldn't need to be aware of handling the es_epqScanDone flags.  (Do you
think that such FDWs should do something like what ExecScanFtch is doing
about the flags, in their RecheckForeignScans?  If so, I think we need
docs for that.)



Execution of alternative local subplan (outerplan) is discretional.
We have to pay attention FDW drivers which handles EPQ recheck by
itself. Even though you argue callback can violate state of
es_epqScanDone flags, it is safe to follow the existing behavior.


So, I think the documentation needs more work.

Yet another thing that I'm concerned about is

@@ -3747,7 +3754,8 @@ make_foreignscan(List *qptlist,
 List *fdw_exprs,
 List *fdw_private,
   

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Friday, November 27, 2015 2:40 PM
> To: Kaigai Kouhei(海外 浩平); Kyotaro HORIGUCHI
> Cc: robertmh...@gmail.com; t...@sss.pgh.pa.us; pgsql-hackers@postgresql.org;
> shigeru.han...@gmail.com
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On 2015/11/27 0:14, Kouhei Kaigai wrote:
> 
> >> The documentation says as following so I think the former has.
> >>
> >> # I don't understhad what 'can or must' means, though... 'can and
> >> # must'?
> >>
> >> + Also, this callback can or must recheck scan qualifiers and join
> >> + conditions which are pushed down. Especially, it needs special
> 
> > If fdw_recheck_quals is set up correctly and join type is inner join,
> > FDW driver does not recheck by itself. Elsewhere, it has to recheck
> > the joined tuple, not only reconstruction.
> 
> Sorry, I don't understand this.  In my understanding, fdw_recheck_quals
> can be defined for a foreign join, regardless of the join type,
>
Yes, "can be defined", but will not be workable if either side of
joined tuple is NULL because of outer join. SQL functions returns
NULL prior to evaluation, then ExecQual() treats this result as FALSE.
However, a joined tuple that has NULL fields may be a valid tuple.

We don't need to care about unmatched tuple if INNER JOIN.

> and when
> the fdw_recheck_quals are defined, the RecheckForeignScan callback
> routine doesn't need to evaluate the fdw_recheck_quals by itself.  No?
>
Yes, it does not need to run fdw_recheck_quals by itself (if they
can guarantee correct results for any corner cases).
Of course, if FDW driver keep expression for scan-qualifiers and
join-clauses on another place (like fdw_exprs), it is FDW driver's
responsibility to execute it, regardless of fdw_recheck_quals.

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



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/26 14:04, Kouhei Kaigai wrote:

On 2015/11/24 2:41, Robert Haas wrote:

On Fri, Nov 20, 2015 at 12:11 AM, Kouhei Kaigai  wrote:

One subplan means FDW driver run an entire join sub-tree with local
alternative sub-plan; that is my expectation for the majority case.



What I'm imagining is that we'd add handling that allows the
ForeignScan to have inner and outer children.  If the FDW wants to
delegate the EvalPlanQual handling to a local plan, it can use the
outer child for that.  Or the inner one, if it likes.  The other one
is available for some other purposes which we can't imagine yet.  If
this is too weird, we can only add handling for an outer subplan and
forget about having an inner subplan for now.  I just thought to make
it symmetric, since outer and inner subplans are pretty deeply baked
into the structure of the system.



I'd vote for only allowing an outer subplan.



The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
FDW driver can set arbitrary but one path-node here.
After that, this path-node shall be transformed to plan-node by
createplan.c, then passed to FDW driver using GetForeignPlan callback.


I understand this, as I also did the same thing in my patches, but 
actually, that seems a bit complicated to me.  Instead, could we keep 
the fdw_outerpath in the fdw_private of a ForeignPath node when creating 
the path node during GetForeignPaths, and then create an outerplan 
accordingly from the fdw_outerpath stored into the fdw_private during 
GetForeignPlan, by using create_plan_recurse there?  I think that that 
would make the core involvment much simpler.



We expect FDW driver set this plan-node on lefttree (a.k.a outerPlan).
The Plan->outerPlan is a common field, so patch size become relatively
small. FDW driver can initialize this plan at BeginForeignScan, then
execute this sub-plan-tree on demand.


Another idea would be to add the core support for 
initializing/closing/rescanning the outerplan tree when the tree is given.



Remaining portions are as previous version. ExecScanFetch is revised
to call recheckMtd always when scanrelid==0, then FDW driver can get
control using RecheckForeignScan callback.
It allows FDW driver to handle (1) EPQ recheck on underlying scan nodes,
(2) reconstruction of joined tuple, and (3) EPQ recheck on join clauses,
by its preferable implementation - including execution of an alternative
sub-plan.


@@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot 
*slot)


ResetExprContext(econtext);

+   /*
+* FDW driver has to recheck visibility of EPQ tuple towards
+* the scan qualifiers once it gets pushed down.
+* In addition, if this node represents a join sub-tree, not
+* a scan, FDW driver is also responsible to reconstruct
+* a joined tuple according to the primitive EPQ tuples.
+*/
+   if (fdwroutine->RecheckForeignScan)
+   {
+   if (!fdwroutine->RecheckForeignScan(node, slot))
+   return false;
+   }

Maybe I'm missing something, but I think we should let FDW do the work 
if scanrelid==0, not just if fdwroutine->RecheckForeignScan is given. 
(And if scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we 
should abort the transaction.)


Another thing I'm concerned about is

@@ -347,8 +355,26 @@ ExecScanReScan(ScanState *node)
{
Index   scanrelid = ((Scan *) node->ps.plan)->scanrelid;

-   Assert(scanrelid > 0);
+   if (scanrelid > 0)
+   estate->es_epqScanDone[scanrelid - 1] = false;
+   else
+   {
+   Bitmapset  *relids;
+   int rtindex = -1;
+
+   if (IsA(node->ps.plan, ForeignScan))
+   relids = ((ForeignScan *) 
node->ps.plan)->fs_relids;
+   else if (IsA(node->ps.plan, CustomScan))
+   relids = ((CustomScan *) 
node->ps.plan)->custom_relids;
+   else
+   elog(ERROR, "unexpected scan node: %d",
+(int)nodeTag(node->ps.plan));

-   estate->es_epqScanDone[scanrelid - 1] = false;
+   while ((rtindex = bms_next_member(relids, rtindex)) >= 
0)
+   {
+   Assert(rtindex > 0);
+   estate->es_epqScanDone[rtindex - 1] = false;
+   }
+   }
}

That seems the outerplan's business to me, so I think it'd be better to 
just return, right before the assertion, as I said before.  Seen from 
another angle, ISTM that FDWs that don't use a local join execution plan 
wouldn't need to be aware of handling the es_epqScanDone flags.  (Do you 
think that such FDWs should do something like what 

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On 2015/11/26 14:04, Kouhei Kaigai wrote:
> >> On 2015/11/24 2:41, Robert Haas wrote:
> >>> On Fri, Nov 20, 2015 at 12:11 AM, Kouhei Kaigai  
> >>> wrote:
>  One subplan means FDW driver run an entire join sub-tree with local
>  alternative sub-plan; that is my expectation for the majority case.
> 
> >>> What I'm imagining is that we'd add handling that allows the
> >>> ForeignScan to have inner and outer children.  If the FDW wants to
> >>> delegate the EvalPlanQual handling to a local plan, it can use the
> >>> outer child for that.  Or the inner one, if it likes.  The other one
> >>> is available for some other purposes which we can't imagine yet.  If
> >>> this is too weird, we can only add handling for an outer subplan and
> >>> forget about having an inner subplan for now.  I just thought to make
> >>> it symmetric, since outer and inner subplans are pretty deeply baked
> >>> into the structure of the system.
> 
> >> I'd vote for only allowing an outer subplan.
> 
> > The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
> > FDW driver can set arbitrary but one path-node here.
> > After that, this path-node shall be transformed to plan-node by
> > createplan.c, then passed to FDW driver using GetForeignPlan callback.
> 
> I understand this, as I also did the same thing in my patches, but
> actually, that seems a bit complicated to me.  Instead, could we keep
> the fdw_outerpath in the fdw_private of a ForeignPath node when creating
> the path node during GetForeignPaths, and then create an outerplan
> accordingly from the fdw_outerpath stored into the fdw_private during
> GetForeignPlan, by using create_plan_recurse there?  I think that that
> would make the core involvment much simpler.
>
How to use create_plan_recurse by extension? It is a static function.

> > We expect FDW driver set this plan-node on lefttree (a.k.a outerPlan).
> > The Plan->outerPlan is a common field, so patch size become relatively
> > small. FDW driver can initialize this plan at BeginForeignScan, then
> > execute this sub-plan-tree on demand.
> 
> Another idea would be to add the core support for
> initializing/closing/rescanning the outerplan tree when the tree is given.
>
No. Please don't repeat same discussion again.

> > Remaining portions are as previous version. ExecScanFetch is revised
> > to call recheckMtd always when scanrelid==0, then FDW driver can get
> > control using RecheckForeignScan callback.
> > It allows FDW driver to handle (1) EPQ recheck on underlying scan nodes,
> > (2) reconstruction of joined tuple, and (3) EPQ recheck on join clauses,
> > by its preferable implementation - including execution of an alternative
> > sub-plan.
> 
> @@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
> *slot)
> 
>   ResetExprContext(econtext);
> 
> + /*
> +  * FDW driver has to recheck visibility of EPQ tuple towards
> +  * the scan qualifiers once it gets pushed down.
> +  * In addition, if this node represents a join sub-tree, not
> +  * a scan, FDW driver is also responsible to reconstruct
> +  * a joined tuple according to the primitive EPQ tuples.
> +  */
> + if (fdwroutine->RecheckForeignScan)
> + {
> + if (!fdwroutine->RecheckForeignScan(node, slot))
> + return false;
> + }
> 
> Maybe I'm missing something, but I think we should let FDW do the work
> if scanrelid==0, not just if fdwroutine->RecheckForeignScan is given.
> (And if scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we
> should abort the transaction.)
>
It should be Assert(). The node with scanrelid==0 never happen
unless FDW driver does not add such a path explicitly.

> Another thing I'm concerned about is
> 
> @@ -347,8 +355,26 @@ ExecScanReScan(ScanState *node)
>   {
>   Index   scanrelid = ((Scan *)
> node->ps.plan)->scanrelid;
> 
> - Assert(scanrelid > 0);
> + if (scanrelid > 0)
> + estate->es_epqScanDone[scanrelid - 1] = false;
> + else
> + {
> + Bitmapset  *relids;
> + int rtindex = -1;
> +
> + if (IsA(node->ps.plan, ForeignScan))
> + relids = ((ForeignScan *)
> node->ps.plan)->fs_relids;
> + else if (IsA(node->ps.plan, CustomScan))
> + relids = ((CustomScan *)
> node->ps.plan)->custom_relids;
> + else
> + elog(ERROR, "unexpected scan node: %d",
> +  (int)nodeTag(node->ps.plan));
> 
> - estate->es_epqScanDone[scanrelid - 1] = false;
> + while ((rtindex = bms_next_member(relids, rtindex)) >=
> 0)
> + {
> + Assert(rtindex > 0);
> + 

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> At Thu, 26 Nov 2015 05:04:32 +, Kouhei Kaigai  wrote
> in <9a28c8860f777e439aa12e8aea7694f801176...@bpxm15gp.gisp.nec.co.jp>
> > > On 2015/11/24 2:41, Robert Haas wrote:
> > > > On Fri, Nov 20, 2015 at 12:11 AM, Kouhei Kaigai  
> > > > wrote:
> > > >> One subplan means FDW driver run an entire join sub-tree with local
> > > >> alternative sub-plan; that is my expectation for the majority case.
> > >
> > > > What I'm imagining is that we'd add handling that allows the
> > > > ForeignScan to have inner and outer children.  If the FDW wants to
> > > > delegate the EvalPlanQual handling to a local plan, it can use the
> > > > outer child for that.  Or the inner one, if it likes.  The other one
> > > > is available for some other purposes which we can't imagine yet.  If
> > > > this is too weird, we can only add handling for an outer subplan and
> > > > forget about having an inner subplan for now.  I just thought to make
> > > > it symmetric, since outer and inner subplans are pretty deeply baked
> > > > into the structure of the system.
> > >
> > > I'd vote for only allowing an outer subplan.
> > >
> > The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
> > FDW driver can set arbitrary but one path-node here.
> 
> It is named "outerpath/plan". Surely we used the term 'outer' in
> association with other nodes for disign decision but is it valid
> to call it outer?  Addition to that, there's no innerpath in this
> patch and have "path" instead.
>
Just "path" is too simple, not to inform people the expected usage
of the node.
If we would assign another name, my preference is "fdw_subpath" or
"fdw_altpath".

> > After that, this path-node shall be transformed to plan-node by
> > createplan.c, then passed to FDW driver using GetForeignPlan callback.
> > We expect FDW driver set this plan-node on lefttree (a.k.a outerPlan).
> > The Plan->outerPlan is a common field, so patch size become relatively
> 
>  Plan->outerPlan => Plan->lefttree?
>
Yes, s/outerPlan/lefttree/g

> > small. FDW driver can initialize this plan at BeginForeignScan, then
> > execute this sub-plan-tree on demand.
> >
> > Remaining portions are as previous version. ExecScanFetch is revised
> > to call recheckMtd always when scanrelid==0, then FDW driver can get
> > control using RecheckForeignScan callback.
> 
> Perhaps we need a comment about foreignscan as a fake join for
> the case with scanrelid == 0 in ExecScanReScan.
>
Indeed,

> > It allows FDW driver to handle (1) EPQ recheck on underlying scan nodes,
> > (2) reconstruction of joined tuple, and (3) EPQ recheck on join clauses,
> > by its preferable implementation - including execution of an alternative
> > sub-plan.
> 
> In ForeignRecheck, ExecQual on fdw_recheck_quals is executed if
> RecheckForeignScan returns true, which I think the signal that
> the returned tuple matches the recheck qual. Whether do you think
> have the responsibility to check the reconstructed tuple when
> RecheckCoreignScan is registered, RecheckForeignScan or ExecQual?
>
Only RecheckForeignScan can reconstruct a joined tuple. On the other
hands, both of facility can recheck scan-qualifiers and join-clauses.
FDW author can choose its design according to his preference.
If fdw_recheck_quals==NIL, FDW can apply all the rechecks within
RecheckForeignScan callback.


> The documentation says as following so I think the former has.
> 
> # I don't understhad what 'can or must' means, though... 'can and
> # must'?
> 
> + Also, this callback can or must recheck scan qualifiers and join
> + conditions which are pushed down. Especially, it needs special
>
If fdw_recheck_quals is set up correctly and join type is inner join,
FDW driver does not recheck by itself. Elsewhere, it has to recheck
the joined tuple, not only reconstruction.
I try to revise the SGML stuff.

> 
> > > There seems to be no changes to make_foreignscan.  Is that OK?
> > >
> > create_foreignscan_path(), not only make_foreignscan().
> >
> > This patch is not tested by actual FDW extensions, so it is helpful
> > to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.
> 
> regardes,
>
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Tuesday, November 24, 2015 12:45 PM
> To: Robert Haas; Kaigai Kouhei(海外 浩平)
> Cc: Tom Lane; Kyotaro HORIGUCHI; pgsql-hackers@postgresql.org; Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On 2015/11/24 2:41, Robert Haas wrote:
> > On Fri, Nov 20, 2015 at 12:11 AM, Kouhei Kaigai <kai...@ak.jp.nec.com> 
> > wrote:
> >> One subplan means FDW driver run an entire join sub-tree with local
> >> alternative sub-plan; that is my expectation for the majority case.
> 
> > What I'm imagining is that we'd add handling that allows the
> > ForeignScan to have inner and outer children.  If the FDW wants to
> > delegate the EvalPlanQual handling to a local plan, it can use the
> > outer child for that.  Or the inner one, if it likes.  The other one
> > is available for some other purposes which we can't imagine yet.  If
> > this is too weird, we can only add handling for an outer subplan and
> > forget about having an inner subplan for now.  I just thought to make
> > it symmetric, since outer and inner subplans are pretty deeply baked
> > into the structure of the system.
> 
> I'd vote for only allowing an outer subplan.
>
The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
FDW driver can set arbitrary but one path-node here.
After that, this path-node shall be transformed to plan-node by
createplan.c, then passed to FDW driver using GetForeignPlan callback.
We expect FDW driver set this plan-node on lefttree (a.k.a outerPlan).
The Plan->outerPlan is a common field, so patch size become relatively
small. FDW driver can initialize this plan at BeginForeignScan, then
execute this sub-plan-tree on demand.

Remaining portions are as previous version. ExecScanFetch is revised
to call recheckMtd always when scanrelid==0, then FDW driver can get
control using RecheckForeignScan callback.
It allows FDW driver to handle (1) EPQ recheck on underlying scan nodes,
(2) reconstruction of joined tuple, and (3) EPQ recheck on join clauses,
by its preferable implementation - including execution of an alternative
sub-plan.

> There seems to be no changes to make_foreignscan.  Is that OK?
>
create_foreignscan_path(), not only make_foreignscan().

This patch is not tested by actual FDW extensions, so it is helpful
to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.

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




pgsql-fdw-epq-recheck.v4.patch
Description: pgsql-fdw-epq-recheck.v4.patch

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kyotaro HORIGUCHI]

Hello,

At Thu, 26 Nov 2015 05:04:32 +, Kouhei Kaigai  wrote 
in <9a28c8860f777e439aa12e8aea7694f801176...@bpxm15gp.gisp.nec.co.jp>
> > On 2015/11/24 2:41, Robert Haas wrote:
> > > On Fri, Nov 20, 2015 at 12:11 AM, Kouhei Kaigai  
> > > wrote:
> > >> One subplan means FDW driver run an entire join sub-tree with local
> > >> alternative sub-plan; that is my expectation for the majority case.
> > 
> > > What I'm imagining is that we'd add handling that allows the
> > > ForeignScan to have inner and outer children.  If the FDW wants to
> > > delegate the EvalPlanQual handling to a local plan, it can use the
> > > outer child for that.  Or the inner one, if it likes.  The other one
> > > is available for some other purposes which we can't imagine yet.  If
> > > this is too weird, we can only add handling for an outer subplan and
> > > forget about having an inner subplan for now.  I just thought to make
> > > it symmetric, since outer and inner subplans are pretty deeply baked
> > > into the structure of the system.
> > 
> > I'd vote for only allowing an outer subplan.
> >
> The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
> FDW driver can set arbitrary but one path-node here.

It is named "outerpath/plan". Surely we used the term 'outer' in
association with other nodes for disign decision but is it valid
to call it outer?  Addition to that, there's no innerpath in this
patch and have "path" instead.

> After that, this path-node shall be transformed to plan-node by
> createplan.c, then passed to FDW driver using GetForeignPlan callback.
> We expect FDW driver set this plan-node on lefttree (a.k.a outerPlan).
> The Plan->outerPlan is a common field, so patch size become relatively

 Plan->outerPlan => Plan->lefttree?

> small. FDW driver can initialize this plan at BeginForeignScan, then
> execute this sub-plan-tree on demand.
> 
> Remaining portions are as previous version. ExecScanFetch is revised
> to call recheckMtd always when scanrelid==0, then FDW driver can get
> control using RecheckForeignScan callback.

Perhaps we need a comment about foreignscan as a fake join for
the case with scanrelid == 0 in ExecScanReScan.

> It allows FDW driver to handle (1) EPQ recheck on underlying scan nodes,
> (2) reconstruction of joined tuple, and (3) EPQ recheck on join clauses,
> by its preferable implementation - including execution of an alternative
> sub-plan.

In ForeignRecheck, ExecQual on fdw_recheck_quals is executed if
RecheckForeignScan returns true, which I think the signal that
the returned tuple matches the recheck qual. Whether do you think
have the responsibility to check the reconstructed tuple when
RecheckCoreignScan is registered, RecheckForeignScan or ExecQual?

The documentation says as following so I think the former has.

# I don't understhad what 'can or must' means, though... 'can and
# must'?

+ Also, this callback can or must recheck scan qualifiers and join
+ conditions which are pushed down. Especially, it needs special


> > There seems to be no changes to make_foreignscan.  Is that OK?
> >
> create_foreignscan_path(), not only make_foreignscan().
> 
> This patch is not tested by actual FDW extensions, so it is helpful
> to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.

regardes,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Fri, Nov 20, 2015 at 12:11 AM, Kouhei Kaigai  wrote:
> >> On Thu, Nov 19, 2015 at 6:39 AM, Kouhei Kaigai  
> >> wrote:
> >> > So, are you suggesting to make a patch that allows ForeignScan to have
> >> > multiple sub-plans right now? Or, one sub-plan?
> >>
> >> Two:
> >>
> http://www.postgresql.org/message-id/CA+TgmoYZeje+ot1kX4wdoB7R7DPS0CWXAzfqZ-
> >> 14ykfkgkr...@mail.gmail.com
> >>
> > Hmm. Two is a bit mysterious for me because two sub-plans (likely)
> > means this ForeignScan node checks join clauses and reconstruct
> > a joined tuple by itself but does not check scan clauses pushed-
> > down (it is job of inner/outer scan plan, isn't it?).
> > In this case, how do we treat N-way remote join cases (N>2) if we
> > assume such a capability in FDW driver?
> >
> > One subplan means FDW driver run an entire join sub-tree with local
> > alternative sub-plan; that is my expectation for the majority case.
> > However, I cannot explain two subplans, but not multiple, well.
> 
> What I'm imagining is that we'd add handling that allows the
> ForeignScan to have inner and outer children.  If the FDW wants to
> delegate the EvalPlanQual handling to a local plan, it can use the
> outer child for that.  Or the inner one, if it likes.  The other one
> is available for some other purposes which we can't imagine yet.  If
> this is too weird, we can only add handling for an outer subplan and
> forget about having an inner subplan for now.
>
I'd like to agree the last sentence. Having one sub-plan is better
(but the second best from my standpoint) than fixed two subplans,
because ...

> I just thought to make
> it symmetric, since outer and inner subplans are pretty deeply baked
> into the structure of the system.
>
Yep, if we would have a special ForeignJoinPath to handle two foreign-
tables join, it will be natural. However, our choice allows N-way join
at once if sub-plan is consists of three or more foreign-tables.
In this case, ForeignScan (scanrelid==0) can represents a sub-plan that
shall be equivalent to a stack of joins; that looks like a ForeignScan
has inner, outer and variable number of "middler" input streams.

If and when we assume ForeignScan has own join mechanism but processes
scan-qualifiers by local sub-plans, fixed-number sub-plans are not
sufficient. (Probably, it is minority case although.)

I'm inclined to put just one outer path at this moment, because the
purpose of the FDW sub-plans is EPQ recheck right now. So, we will
be able to enhance the feature when we implement other stuffs - more
aggressive join push-down for example.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/24 2:41, Robert Haas wrote:

On Fri, Nov 20, 2015 at 12:11 AM, Kouhei Kaigai  wrote:

One subplan means FDW driver run an entire join sub-tree with local
alternative sub-plan; that is my expectation for the majority case.



What I'm imagining is that we'd add handling that allows the
ForeignScan to have inner and outer children.  If the FDW wants to
delegate the EvalPlanQual handling to a local plan, it can use the
outer child for that.  Or the inner one, if it likes.  The other one
is available for some other purposes which we can't imagine yet.  If
this is too weird, we can only add handling for an outer subplan and
forget about having an inner subplan for now.  I just thought to make
it symmetric, since outer and inner subplans are pretty deeply baked
into the structure of the system.


I'd vote for only allowing an outer subplan.

Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/20 22:45, Kouhei Kaigai wrote:
I wrote:

* This patch means we can define fdw_recheck_quals even for the case of
foreign tables with non-NIL fdw_scan_tlist.  However, we discussed in
another thread [1] that such foreign tables might break EvalPlanQual
tests.  Where are we on that issue?



In case of later locking, RefetchForeignRow() will set a base tuple
that have compatible layout of the base relation, not fdw_scan_tlist,
because RefetchForeignRow() does not have information about scan node.


IIUC, I think the base tuple would be stored into EPQ state not only in 
case of late row locking but in case of early row locking.



* For the case of foreign joins, I think fdw_recheck_quals can be
defined for example, the same way as for the case of foreign tables, ie,
quals not in scan.plan.qual, or ones defined as "otherclauses"
(rinfo->is_pushed_down=true) pushed down to the remote.  But since it's
required that the FDW has to add to the fdw_scan_tlist the set of
columns needed to check quals in fdw_recheck_quals in preparation for
EvalPlanQual tests, it's likely that fdw_scan_tlist will end up being
long, leading to an increase in a total data transfer amount from the
remote.  So, that seems not practical to me.  Maybe I'm missing
something, but what use cases are you thinking?



It is trade-off. What solution do you think we can have?
To avoid data transfer used for EPQ recheck only, we can implement
FDW driver to issue remote join again on EPQ recheck, however, it
is not a wise design, isn't it?

If we would be able to have no extra data transfer and no remote
join execution during EPQ recheck, it is a perfect.


I was thinking that in an approach using a local join execution plan, I 
would just set fdw_recheck_quals set to NIL and evaluate the 
otherclauses as part of the local join execution plan, so that 
fdw_scan_tlist won't end up being longer, as in the patch [1].  (Note 
that in that patch, remote_exprs==NIL when calling make_foreignscan 
during postgresGetForeignPlan in case of foreign joins.)


Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/5624d583.10...@lab.ntt.co.jp



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/09 9:26, Kouhei Kaigai wrote:

The attached patch is an adjusted version of the previous one.


There seems to be no changes to make_foreignscan.  Is that OK?

Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Fri, Nov 20, 2015 at 12:11 AM, Kouhei Kaigai  wrote:
>> On Thu, Nov 19, 2015 at 6:39 AM, Kouhei Kaigai  wrote:
>> > So, are you suggesting to make a patch that allows ForeignScan to have
>> > multiple sub-plans right now? Or, one sub-plan?
>>
>> Two:
>>
>> http://www.postgresql.org/message-id/CA+TgmoYZeje+ot1kX4wdoB7R7DPS0CWXAzfqZ-
>> 14ykfkgkr...@mail.gmail.com
>>
> Hmm. Two is a bit mysterious for me because two sub-plans (likely)
> means this ForeignScan node checks join clauses and reconstruct
> a joined tuple by itself but does not check scan clauses pushed-
> down (it is job of inner/outer scan plan, isn't it?).
> In this case, how do we treat N-way remote join cases (N>2) if we
> assume such a capability in FDW driver?
>
> One subplan means FDW driver run an entire join sub-tree with local
> alternative sub-plan; that is my expectation for the majority case.
> However, I cannot explain two subplans, but not multiple, well.

What I'm imagining is that we'd add handling that allows the
ForeignScan to have inner and outer children.  If the FDW wants to
delegate the EvalPlanQual handling to a local plan, it can use the
outer child for that.  Or the inner one, if it likes.  The other one
is available for some other purposes which we can't imagine yet.  If
this is too weird, we can only add handling for an outer subplan and
forget about having an inner subplan for now.  I just thought to make
it symmetric, since outer and inner subplans are pretty deeply baked
into the structure of the system.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On 2015/11/19 12:32, Robert Haas wrote:
> > On Tue, Nov 17, 2015 at 8:47 PM, Kouhei Kaigai  wrote:
> >> The attached patch is the portion cut from the previous EPQ recheck
> >> patch.
> 
> > Thanks, committed.
> 
> Thanks, Robert and KaiGai-san.
> 
> Sorry, I'm a bit late to the party.  Here are my questions:
> 
> * This patch means we can define fdw_recheck_quals even for the case of
> foreign tables with non-NIL fdw_scan_tlist.  However, we discussed in
> another thread [1] that such foreign tables might break EvalPlanQual
> tests.  Where are we on that issue?
>
In case of later locking, RefetchForeignRow() will set a base tuple
that have compatible layout of the base relation, not fdw_scan_tlist,
because RefetchForeignRow() does not have information about scan node.
Here is two solutions. 1) You should not use fdw_scan_tlist for the
FDW that uses late locking mechanism. 2) recheck callback applies
projection to fit fdw_scan_tlist (that is not difficult to provide
as a utility function by the core).

Even though we allow to set up fdw_scan_tlist on simple scan cases,
it does not mean it works for any cases.

> * For the case of foreign joins, I think fdw_recheck_quals can be
> defined for example, the same way as for the case of foreign tables, ie,
> quals not in scan.plan.qual, or ones defined as "otherclauses"
> (rinfo->is_pushed_down=true) pushed down to the remote.  But since it's
> required that the FDW has to add to the fdw_scan_tlist the set of
> columns needed to check quals in fdw_recheck_quals in preparation for
> EvalPlanQual tests, it's likely that fdw_scan_tlist will end up being
> long, leading to an increase in a total data transfer amount from the
> remote.  So, that seems not practical to me.  Maybe I'm missing
> something, but what use cases are you thinking?
>
It is trade-off. What solution do you think we can have?
To avoid data transfer used for EPQ recheck only, we can implement
FDW driver to issue remote join again on EPQ recheck, however, it
is not a wise design, isn't it?

If we would be able to have no extra data transfer and no remote
join execution during EPQ recheck, it is a perfect.
However, we have to take both advantage and disadvantage when
we determine an implementation. We usually choose a way that
has more advantage than disadvantage, but it does not mean no
disadvantage.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/19 12:32, Robert Haas wrote:

On Tue, Nov 17, 2015 at 8:47 PM, Kouhei Kaigai  wrote:

The attached patch is the portion cut from the previous EPQ recheck
patch.



Thanks, committed.


Thanks, Robert and KaiGai-san.

Sorry, I'm a bit late to the party.  Here are my questions:

* This patch means we can define fdw_recheck_quals even for the case of 
foreign tables with non-NIL fdw_scan_tlist.  However, we discussed in 
another thread [1] that such foreign tables might break EvalPlanQual 
tests.  Where are we on that issue?


* For the case of foreign joins, I think fdw_recheck_quals can be 
defined for example, the same way as for the case of foreign tables, ie, 
quals not in scan.plan.qual, or ones defined as "otherclauses" 
(rinfo->is_pushed_down=true) pushed down to the remote.  But since it's 
required that the FDW has to add to the fdw_scan_tlist the set of 
columns needed to check quals in fdw_recheck_quals in preparation for 
EvalPlanQual tests, it's likely that fdw_scan_tlist will end up being 
long, leading to an increase in a total data transfer amount from the 
remote.  So, that seems not practical to me.  Maybe I'm missing 
something, but what use cases are you thinking?


Best regards,
Etsuro Fujita

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



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/20 6:57, Robert Haas wrote:

On Wed, Nov 18, 2015 at 10:54 PM, Etsuro Fujita
 wrote:

Noted, but let's do it that way and move on.  It would be a shame if
we didn't end up with a working FDW join pushdown system in 9.6
because of a disagreement on this point.



Another idea would be to consider join pushdown as unsupported for now when
select-for-update is involved in 9.5, as described in [1], and revisit this
issue when adding join pushdown to postgres_fdw in 9.6.



Well, I think it's probably too late to squeeze this into 9.5 at this
point, but I'm eager to get it fixed for 9.6.


OK, I'll update the postgres_fdw-join-pushdown patch so as to work with 
that callback routine, if needed.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Wed, Nov 18, 2015 at 10:54 PM, Etsuro Fujita
 wrote:
>> Noted, but let's do it that way and move on.  It would be a shame if
>> we didn't end up with a working FDW join pushdown system in 9.6
>> because of a disagreement on this point.
>
> Another idea would be to consider join pushdown as unsupported for now when
> select-for-update is involved in 9.5, as described in [1], and revisit this
> issue when adding join pushdown to postgres_fdw in 9.6.

Well, I think it's probably too late to squeeze this into 9.5 at this
point, but I'm eager to get it fixed for 9.6.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Thu, Nov 19, 2015 at 6:39 AM, Kouhei Kaigai  wrote:
> So, are you suggesting to make a patch that allows ForeignScan to have
> multiple sub-plans right now? Or, one sub-plan?

Two:

http://www.postgresql.org/message-id/ca+tgmoyzeje+ot1kx4wdob7r7dps0cwxazfqz-14ykfkgkr...@mail.gmail.com

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Tue, Nov 17, 2015 at 10:22 PM, Kouhei Kaigai  wrote:
> > Apart from EPQ rechecks, the above aggressive push-down idea allows to send
> > contents of multiple relations to the remote side. In this case, ForeignScan
> > needs to have multiple sub-plans.
> >
> > For example, please assume here is three relations; tbl_A and tbl_B are
> > local and small, tbl_F is remote and large.
> > In case when both of (tbl_A JOIN tbl_F) and (tbl_B JOIN tbl_F) produces
> > large number of rows thus consumes deserved amount of network traffic but
> > (tbl_A JOIN tbl_B JOIN tbl_F) produce small number of rows, the optimal
> > strategy is to send local contents to the remote side once then run
> > a remote query here to produce relatively smaller rows.
> > In the implementation level, ForeignScan shall represent (tbl_A JOIN tbl_B
> > JOIN tbl_F), then it returns a bunch of joined tuples. Its remote query
> > contains VALUES(...) clauses to pack contents of the tbl_A and tbl_B, thus,
> > it needs to be capable to execute underlying multiple scan plans and fetch
> > tuples prior to remote query execution.
> 
> Hmm, maybe.  I'm not entirely sure multiple subplans is the best way
> to implement that, but let's argue about that another day.
>
So, are you suggesting to make a patch that allows ForeignScan to have
multiple sub-plans right now? Or, one sub-plan?

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Thu, Nov 19, 2015 at 6:39 AM, Kouhei Kaigai  wrote:
> > So, are you suggesting to make a patch that allows ForeignScan to have
> > multiple sub-plans right now? Or, one sub-plan?
> 
> Two:
> 
> http://www.postgresql.org/message-id/CA+TgmoYZeje+ot1kX4wdoB7R7DPS0CWXAzfqZ-
> 14ykfkgkr...@mail.gmail.com
>
Hmm. Two is a bit mysterious for me because two sub-plans (likely)
means this ForeignScan node checks join clauses and reconstruct
a joined tuple by itself but does not check scan clauses pushed-
down (it is job of inner/outer scan plan, isn't it?).
In this case, how do we treat N-way remote join cases (N>2) if we
assume such a capability in FDW driver?

One subplan means FDW driver run an entire join sub-tree with local
alternative sub-plan; that is my expectation for the majority case.
However, I cannot explain two subplans, but not multiple, well.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/19 12:34, Robert Haas wrote:

On Tue, Nov 17, 2015 at 9:30 PM, Etsuro Fujita
 wrote:

I suppose you (and KaiGai-san) are probably right, but I really fail to see
it actually doing that.



Noted, but let's do it that way and move on.  It would be a shame if
we didn't end up with a working FDW join pushdown system in 9.6
because of a disagreement on this point.


Another idea would be to consider join pushdown as unsupported for now 
when select-for-update is involved in 9.5, as described in [1], and 
revisit this issue when adding join pushdown to postgres_fdw in 9.6.


Best regards,
Etsuro Fujita

[1] https://wiki.postgresql.org/wiki/Open_Items



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Nov 17, 2015 at 9:30 PM, Etsuro Fujita
 wrote:
> I suppose you (and KaiGai-san) are probably right, but I really fail to see
> it actually doing that.

Noted, but let's do it that way and move on.  It would be a shame if
we didn't end up with a working FDW join pushdown system in 9.6
because of a disagreement on this point.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Nov 17, 2015 at 8:47 PM, Kouhei Kaigai  wrote:
> The attached patch is the portion cut from the previous EPQ recheck
> patch.

Thanks, committed.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Nov 17, 2015 at 10:22 PM, Kouhei Kaigai  wrote:
> Apart from EPQ rechecks, the above aggressive push-down idea allows to send
> contents of multiple relations to the remote side. In this case, ForeignScan
> needs to have multiple sub-plans.
>
> For example, please assume here is three relations; tbl_A and tbl_B are
> local and small, tbl_F is remote and large.
> In case when both of (tbl_A JOIN tbl_F) and (tbl_B JOIN tbl_F) produces
> large number of rows thus consumes deserved amount of network traffic but
> (tbl_A JOIN tbl_B JOIN tbl_F) produce small number of rows, the optimal
> strategy is to send local contents to the remote side once then run
> a remote query here to produce relatively smaller rows.
> In the implementation level, ForeignScan shall represent (tbl_A JOIN tbl_B
> JOIN tbl_F), then it returns a bunch of joined tuples. Its remote query
> contains VALUES(...) clauses to pack contents of the tbl_A and tbl_B, thus,
> it needs to be capable to execute underlying multiple scan plans and fetch
> tuples prior to remote query execution.

Hmm, maybe.  I'm not entirely sure multiple subplans is the best way
to implement that, but let's argue about that another day.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Thu, Nov 12, 2015 at 12:54 AM, Etsuro Fujita
 wrote:
> Really?  I think there would be not a little burden on an FDW author; when
> postgres_fdw delegates to the subplan to the remote server, for example, it
> would need to create a remote join query by looking at tuples possibly
> fetched and stored in estate->es_epqTuple[], send the query and receive the
> result during the callback routine.  Furthermore, what I'm most concerned
> about is that wouldn't be efficient.  So, my question about that approach is
> whether FDWs really do some thing like that during the callback routine,
> instead of performing a secondary join plan locally.  As I said before, I
> know that KaiGai-san considers that that approach would be useful for custom
> joins.  But I see zero evidence that there is a good use-case for an FDW.

It could do that.  But it could also just invoke a subplan as you are
proposing.  Or at least, I think we should set it up so that such a
thing is possible.  In which case I don't see the problem.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Sun, Nov 8, 2015 at 7:26 PM, Kouhei Kaigai  wrote:
> The attached patch is an adjusted version of the previous one.
> Even though it co-exists a new callback and fdw_recheck_quals,
> the callback is kicked first as follows.

This seems excessive to me: why would we need an arbitrary-length list
of plans for an FDW?  I think we should just allow an outer child and
an inner child, which is probably one more than we'll ever need in
practice.

This looks like an independent bug fix:

+   fscan->fdw_recheck_quals = (List *)
+   fix_upper_expr(root,
+  (Node *)
fscan->fdw_recheck_quals,
+  itlist,
+  INDEX_VAR,
+  rtoffset);
pfree(itlist);

If so, it should be committed separately and back-patched to 9.5.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Sun, Nov 8, 2015 at 7:26 PM, Kouhei Kaigai  wrote:
> > The attached patch is an adjusted version of the previous one.
> > Even though it co-exists a new callback and fdw_recheck_quals,
> > the callback is kicked first as follows.
> 
> This seems excessive to me: why would we need an arbitrary-length list
> of plans for an FDW?  I think we should just allow an outer child and
> an inner child, which is probably one more than we'll ever need in
> practice.
>
It just intends to keep code symmetry with custom-scan case, so not
a significant reason.
And, I expected ForeignScan will also need multiple sub-plans soon
to support more intelligent push-down like:
http://www.postgresql.org/message-id/9a28c8860f777e439aa12e8aea7694f8010f4...@bpxm15gp.gisp.nec.co.jp

It is a separate discussion, of course, so I don't have strong preference
here.

> This looks like an independent bug fix:
> 
> +   fscan->fdw_recheck_quals = (List *)
> +   fix_upper_expr(root,
> +  (Node *)
> fscan->fdw_recheck_quals,
> +  itlist,
> +  INDEX_VAR,
> +  rtoffset);
> pfree(itlist);
> 
> If so, it should be committed separately and back-patched to 9.5.
>
OK, I'll split the patch into two.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> > On Sun, Nov 8, 2015 at 7:26 PM, Kouhei Kaigai  wrote:
> > > The attached patch is an adjusted version of the previous one.
> > > Even though it co-exists a new callback and fdw_recheck_quals,
> > > the callback is kicked first as follows.
> >
> > This seems excessive to me: why would we need an arbitrary-length list
> > of plans for an FDW?  I think we should just allow an outer child and
> > an inner child, which is probably one more than we'll ever need in
> > practice.
> >
> It just intends to keep code symmetry with custom-scan case, so not
> a significant reason.
> And, I expected ForeignScan will also need multiple sub-plans soon
> to support more intelligent push-down like:
> http://www.postgresql.org/message-id/9A28C8860F777E439AA12E8AEA7694F8010F47D
> a...@bpxm15gp.gisp.nec.co.jp
> 
> It is a separate discussion, of course, so I don't have strong preference
> here.
> 
> > This looks like an independent bug fix:
> >
> > +   fscan->fdw_recheck_quals = (List *)
> > +   fix_upper_expr(root,
> > +  (Node *)
> > fscan->fdw_recheck_quals,
> > +  itlist,
> > +  INDEX_VAR,
> > +  rtoffset);
> > pfree(itlist);
> >
> > If so, it should be committed separately and back-patched to 9.5.
> >
> OK, I'll split the patch into two.
>
The attached patch is the portion cut from the previous EPQ recheck
patch.

Regarding of the fdw_plans or fdw_plan, I'll follow your suggestion.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 



pgsql-bugfix-fdw_recheck_quals-on-setrefs.patch
Description: pgsql-bugfix-fdw_recheck_quals-on-setrefs.patch

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/18 3:19, Robert Haas wrote:

On Thu, Nov 12, 2015 at 12:54 AM, Etsuro Fujita
 wrote:

Really?  I think there would be not a little burden on an FDW author; when
postgres_fdw delegates to the subplan to the remote server, for example, it
would need to create a remote join query by looking at tuples possibly
fetched and stored in estate->es_epqTuple[], send the query and receive the
result during the callback routine.  Furthermore, what I'm most concerned
about is that wouldn't be efficient.  So, my question about that approach is
whether FDWs really do some thing like that during the callback routine,
instead of performing a secondary join plan locally.  As I said before, I
know that KaiGai-san considers that that approach would be useful for custom
joins.  But I see zero evidence that there is a good use-case for an FDW.



It could do that.  But it could also just invoke a subplan as you are
proposing.  Or at least, I think we should set it up so that such a
thing is possible.  In which case I don't see the problem.


I suppose you (and KaiGai-san) are probably right, but I really fail to 
see it actually doing that.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Nov 17, 2015 at 6:51 PM, Kouhei Kaigai  wrote:
> It just intends to keep code symmetry with custom-scan case, so not
> a significant reason.
> And, I expected ForeignScan will also need multiple sub-plans soon
> to support more intelligent push-down like:
> http://www.postgresql.org/message-id/9a28c8860f777e439aa12e8aea7694f8010f4...@bpxm15gp.gisp.nec.co.jp

I might be missing something, but why would that require multiple child plans?

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Tue, Nov 17, 2015 at 6:51 PM, Kouhei Kaigai  wrote:
> > It just intends to keep code symmetry with custom-scan case, so not
> > a significant reason.
> > And, I expected ForeignScan will also need multiple sub-plans soon
> > to support more intelligent push-down like:
> >
> http://www.postgresql.org/message-id/9A28C8860F777E439AA12E8AEA7694F8010F47D
> a...@bpxm15gp.gisp.nec.co.jp
> 
> I might be missing something, but why would that require multiple child plans?
>
Apart from EPQ rechecks, the above aggressive push-down idea allows to send
contents of multiple relations to the remote side. In this case, ForeignScan
needs to have multiple sub-plans.

For example, please assume here is three relations; tbl_A and tbl_B are
local and small, tbl_F is remote and large.
In case when both of (tbl_A JOIN tbl_F) and (tbl_B JOIN tbl_F) produces
large number of rows thus consumes deserved amount of network traffic but
(tbl_A JOIN tbl_B JOIN tbl_F) produce small number of rows, the optimal
strategy is to send local contents to the remote side once then run
a remote query here to produce relatively smaller rows.
In the implementation level, ForeignScan shall represent (tbl_A JOIN tbl_B
JOIN tbl_F), then it returns a bunch of joined tuples. Its remote query
contains VALUES(...) clauses to pack contents of the tbl_A and tbl_B, thus,
it needs to be capable to execute underlying multiple scan plans and fetch
tuples prior to remote query execution.
So, ForeignScan may also have multiple sub-plans.

Of course, it is an independent feature from the EPQ rechecks.
It is not a matter even if we will extend this field later.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/13 11:31, Kouhei Kaigai wrote:

On 2015/11/12 2:53, Robert Haas wrote:

  From my point of view I'm now
thinking this solution has two parts:

(1) Let foreign scans have inner and outer subplans.  For this
purpose, we only need one, but it's no more work to enable both, so we
may as well.  If we had some reason, we could add a list of subplans
of arbitrary length, but there doesn't seem to be an urgent need for
that.


I wrote:

But one thing I'm concerned about is enable both inner and
outer plans, because I think that that would make the planner
postprocessing complicated, depending on what the foreign scans do by
the inner/outer subplans.  Is it worth doing so?  Maybe I'm missing
something, though.



If you persuade other person who has different opinion, you need to
explain why was it complicated, how much complicated and what was
the solution you tried at that time.
The "complicated" is a subjectively-based term. At least, we don't
share your experience, so it is hard to understand the how complexity.


I don't mean to object that idea.  I'm unfamiliar with that idea, so I 
just wanted to know the reason, or use cases.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/13 13:44, Kyotaro HORIGUCHI wrote:

I wrote:

What I think is, I
see zero evidence that there is a good use-case for an FDW to do
something other than doing an ExecProcNode in the callback routine, as I
said below, so I don't see the need to add such a routine while that
would cause maybe not a large, but not a little burden for writing such
a routine on FDW authors.


KaiGai-san wrote:

It is quite natural because we cannot predicate what kind of extension
is implemented on FDW interface. You might know the initial version of
PG-Strom is implemented on FDW (about 4 years before...). If I would
continue to stick FDW, it became a FDW driver with own join engine.



 From the standpoint of interface design, if we would not admit flexibility
of implementation unless community don't see a working example, a reasonable
tactics *for extension author* is to follow the interface restriction even
if it is not best approach from his standpoint.
It does not mean the approach by majority is also best for the minority.
It just requires the minority a compromise.



Or try to open the way to introduce the feature he/she wants.


I think the biggest difference between KaiGai-san's patch and mine is 
that KaiGai-san's patch introduces a callback routine to allow an FDW 
author not only to execute a secondary plan but to do something else, 
instead of executing the plan, if he/she wants to do so.  His approach 
would provide the flexibility, but IMHO I think major FDWs that would be 
implementing join pushdown, such as postgres_fdw, wouldn't be utilizing 
the flexibility; probably, they would be just executing the secondary 
plan in the routine.  Furthermore, since that for executing the plan, 
his approach would require that an FDW author has to add code not only 
for creating the plan but for initializing/executing/ending it to 
his/her FDW by itself while in my approach, he/she only has to add code 
for the plan creation, his approach would impose a more development 
burden on such major FDWs' authors than mine.  I think the flexibility 
would be a good thing, but I also think it's important not to burden FDW 
authors.  Maybe I'm missing something, though.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On 2015/11/13 13:44, Kyotaro HORIGUCHI wrote:
> 
> I wrote:
> >>> What I think is, I
> >>> see zero evidence that there is a good use-case for an FDW to do
> >>> something other than doing an ExecProcNode in the callback routine, as I
> >>> said below, so I don't see the need to add such a routine while that
> >>> would cause maybe not a large, but not a little burden for writing such
> >>> a routine on FDW authors.
> 
> KaiGai-san wrote:
> >> It is quite natural because we cannot predicate what kind of extension
> >> is implemented on FDW interface. You might know the initial version of
> >> PG-Strom is implemented on FDW (about 4 years before...). If I would
> >> continue to stick FDW, it became a FDW driver with own join engine.
> 
> >>  From the standpoint of interface design, if we would not admit flexibility
> >> of implementation unless community don't see a working example, a 
> >> reasonable
> >> tactics *for extension author* is to follow the interface restriction even
> >> if it is not best approach from his standpoint.
> >> It does not mean the approach by majority is also best for the minority.
> >> It just requires the minority a compromise.
> 
> > Or try to open the way to introduce the feature he/she wants.
> 
> I think the biggest difference between KaiGai-san's patch and mine is
> that KaiGai-san's patch introduces a callback routine to allow an FDW
> author not only to execute a secondary plan but to do something else,
> instead of executing the plan, if he/she wants to do so.  His approach
> would provide the flexibility, but IMHO I think major FDWs that would be
> implementing join pushdown, such as postgres_fdw, wouldn't be utilizing
> the flexibility; probably, they would be just executing the secondary
> plan in the routine.
>
Yes, my approach never deny. 

> Furthermore, since that for executing the plan,
> his approach would require that an FDW author has to add code not only
> for creating the plan but for initializing
>
Pick up a plan from fdw_plans, then call ExecInitNode()

> executing
>
Pick up a plan-state from fdw_ps then call ExecProcNode()

> ending it to
>
Also, call ExecEndNode() towards the plan-state.

> his/her FDW by itself while in my approach, he/she only has to add code
> for the plan creation, his approach would impose a more development
> burden on such major FDWs' authors than mine.
>
It looks to me the more development burden is additional three lines.

Both of our approaches commonly needs to construct alternative local
plan, likely unparametalized nest-loop, on planner phase, it shall be
supported by a utility function in the core background.
So, one more additional line will be eventually needed.

> I think the flexibility
> would be a good thing, but I also think it's important not to burden FDW
> authors.  Maybe I'm missing something, though.
>
The actual pain is people cannot design/implement their module as
they want. I've repeatedly pointed out FDW driver can have own join
implementation and people potentially want to use own logic than
local plan. At least, if PG-Strom would still run on FDW, I *want*
to reuse its CPU-fallback routine instead of the alternative sub-plan.

Could you introduce us why above sequence (a few additional lines) are
unacceptable burden and can justify to eliminate flexibility for
minorities?
If you can implement the "common part" for majority, we can implement
same stuff as utility functions can be called from the callbacks.

My questions are:
* How much lines do you expect for the additional burden?
* Why does it justify to eliminate flexibility of the interface?
* Why cannot we implement the common part as utility routines that
  can be called from the callback?

Please don't hesitate to point out flaw of my proposition, if you
noticed something significant we have never noticed.
However, at this moment, it does not seems to me your concern is
something significant.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

Robert and Kaigai-san,

Sorry, I sent in an unfinished email.

On 2015/11/12 15:30, Kouhei Kaigai wrote:

On 2015/11/12 2:53, Robert Haas wrote:

On Sun, Nov 8, 2015 at 11:13 PM, Etsuro Fujita
 wrote:

To test this change, I think we should update the postgres_fdw patch so as
to add the RecheckForeignScan.

Having said that, as I said previously, I don't see much value in adding the
callback routine, to be honest.  I know KaiGai-san considers that that would
be useful for custom joins, but I don't think that that would be useful even
for foreign joins, because I think that in case of foreign joins, the
practical implementation of that routine in FDWs would be to create a
secondary plan and execute that plan by performing ExecProcNode, as my patch
does [1].  Maybe I'm missing something, though.



I really don't see why you're fighting on this point.  Making this a
generic feature will require only a few extra lines of code for FDW
authors.  If this were going to cause some great inconvenience for FDW
authors, then I'd agree it isn't worth it.  But I see zero evidence
that this is actually the case.



Really?  I think there would be not a little burden on an FDW author;
when postgres_fdw delegates to the subplan to the remote server, for
example, it would need to create a remote join query by looking at
tuples possibly fetched and stored in estate->es_epqTuple[], send the
query and receive the result during the callback routine.



I cannot understand why it is the only solution.


I didn't say that.


Furthermore,
what I'm most concerned about is that wouldn't be efficient. So, my



You have to add "because ..." sentence here because I and Robert
think a little inefficiency is not a problem.


Sorry, my explanation was not enough.  The reason for that is that in 
the above postgres_fdw case for example, the overhead in sending the 
query to the remote end and transferring the result to the local end 
would not be negligible.  Yeah, we might be able to apply a special 
handling for the improved efficiency when using early row locking, but 
otherwise can we do the same thing?



Please don't start the sentence from "I think ...". We all knows
your opinion, but what I've wanted to see is "the reason why my
approach is valuable is ...".


I didn't say that my approach is *valuable* either.  What I think is, I 
see zero evidence that there is a good use-case for an FDW to do 
something other than doing an ExecProcNode in the callback routine, as I 
said below, so I don't see the need to add such a routine while that 
would cause maybe not a large, but not a little burden for writing such 
a routine on FDW authors.



Nobody prohibits postgres_fdw performs a secondary join here.
All you need to do is, picking up a sub-plan tree from FDW's private
field then call ExecProcNode() inside the callback.



As I said before, I know that KaiGai-san considers that
that approach would be useful for custom joins.  But I see zero evidence
that there is a good use-case for an FDW.



 From my point of view I'm now
thinking this solution has two parts:

(1) Let foreign scans have inner and outer subplans.  For this
purpose, we only need one, but it's no more work to enable both, so we
may as well.  If we had some reason, we could add a list of subplans
of arbitrary length, but there doesn't seem to be an urgent need for
that.


I did the same thing in an earlier version of the patch I posted. 
Although I agreed on Robert's comment "The Plan tree and the PlanState 
tree should be mirror images of each other; breaking that equivalence 
will cause confusion, at least.", I think that that would make code much 
simpler, especially the code for setting chgParam for inner/outer 
subplans.  But one thing I'm concerned about is enable both inner and 
outer plans, because I think that that would make the planner 
postprocessing complicated, depending on what the foreign scans do by 
the inner/outer subplans.  Is it worth doing so?  Maybe I'm missing 
something, though.



(2) Add a recheck callback.

If the foreign data wrapper wants to adopt the solution you're
proposing, the recheck callback can call
ExecProcNode(outerPlanState(node)).  I don't think this should end up
being more than a few lines of code, although of course we should
verify that.


Yeah, I think FDWs would probably need to create a subplan accordingly 
at planning time, and then initializing/closing the plan at execution 
time.  I think we could facilitate subplan creation by providing helper 
functions for that, though.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

Horiguchi-san,

On 2015/11/12 16:10, Kyotaro HORIGUCHI wrote:

I really don't see why you're fighting on this point.  Making this a
generic feature will require only a few extra lines of code for FDW
authors.  If this were going to cause some great inconvenience for FDW
authors, then I'd agree it isn't worth it.  But I see zero evidence
that this is actually the case.



Really?  I think there would be not a little burden on an FDW author;
when postgres_fdw delegates to the subplan to the remote server, for
example, it would need to create a remote join query by looking at
tuples possibly fetched and stored in estate->es_epqTuple[], send the
query and receive the result during the callback routine.



Do you mind that FDW cannot generate a plan so that make a tuple
from eqpTules then apply fdw_quals from predefined executor
nodes?


No.  Please see my previous email.  Sorry for my unfinished email.

Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kyotaro HORIGUCHI]

Hello, I also uncertain about what exactly is the blocker..

At Fri, 13 Nov 2015 02:31:53 +, Kouhei Kaigai  wrote 
in <9a28c8860f777e439aa12e8aea7694f80116f...@bpxm15gp.gisp.nec.co.jp>
> > Sorry, my explanation was not enough.  The reason for that is that in
> > the above postgres_fdw case for example, the overhead in sending the
> > query to the remote end and transferring the result to the local end
> > would not be negligible.  Yeah, we might be able to apply a special
> > handling for the improved efficiency when using early row locking, but
> > otherwise can we do the same thing?
> >
> It is trade-off. Late locking semantics allows to lock relatively smaller
> number of remote rows, it will take extra latency.
> Also, it became clear we have a challenge to pull a joined tuple at once.

Late row locking anyway needs to send query to the remote side
and needs to generate the joined row in either side of the
connection. Early row locking on FDW don't need that since the
necessary tuples are already in out hand. Is there any
performance issue in this? Unfortunately I've not comprehend what
is the problem:(

Or, Are you Fujita-san thinking about bulk late row locking or
such?  If so, it is a matter of future, as update/insert
pushdown, I suppose.

> > I didn't say that my approach is *valuable* either.  What I think is, I
> > see zero evidence that there is a good use-case for an FDW to do
> > something other than doing an ExecProcNode in the callback routine, as I
> > said below, so I don't see the need to add such a routine while that
> > would cause maybe not a large, but not a little burden for writing such
> > a routine on FDW authors.
> >
> It is quite natural because we cannot predicate what kind of extension
> is implemented on FDW interface. You might know the initial version of
> PG-Strom is implemented on FDW (about 4 years before...). If I would
> continue to stick FDW, it became a FDW driver with own join engine.
> (cstore_fdw may potentially support own join logic on top of their
> columnar storage for instance?)
> 
> From the standpoint of interface design, if we would not admit flexibility
> of implementation unless community don't see a working example, a reasonable
> tactics *for extension author* is to follow the interface restriction even
> if it is not best approach from his standpoint.
> It does not mean the approach by majority is also best for the minority.
> It just requires the minority a compromise.

Or try to open the way to introduce the feature he/she wants. If
workable postgres_fdw with join pushdown based on this API to any
extent be shown here, we can envestigate on the problem
there. But perhaps the deadline is just before us..

> > I did the same thing in an earlier version of the patch I posted.
> > Although I agreed on Robert's comment "The Plan tree and the PlanState
> > tree should be mirror images of each other; breaking that equivalence
> > will cause confusion, at least.", I think that that would make code much
> > simpler, especially the code for setting chgParam for inner/outer
> > subplans.

I see that the Kaigai-san's patch doesn't put different nodes
from paths during plan creation, in other words, it doesn't break
coherence between paths and plans as long as core's point of
view. The Fujita-san's patch mentioned above altered a node in
core's sight. I understand that it is the most significant
difference between them..

> >  But one thing I'm concerned about is enable both inner and
> > outer plans, because I think that that would make the planner
> > postprocessing complicated, depending on what the foreign scans do by
> > the inner/outer subplans.  Is it worth doing so?  Maybe I'm missing
> > something, though.

This is discussion about late row locking? Join pushdown itself
is a kind of complicated process. And since it fools planner in
one aspect, the additional feature would be inevitable to be
complex to some extent. We could discuss on that after some
specific problem comes in out sight.

> If you persuade other person who has different opinion, you need to
> explain why was it complicated, how much complicated and what was
> the solution you tried at that time.
> The "complicated" is a subjectively-based term. At least, we don't
> share your experience, so it is hard to understand the how complexity.

Mee too.  It surely might be complicated (though the extent is
mainly in indivisual's mind..) but also I don't see how the
Fujita-san's patch resolves that "problem".

> I guess it is similar to what built-in logic is usually doing, thus,
> it should not be a problem we cannot solve. A utility routine FDW
> driver can call will solve the issue (even if it is not supported
> on v9.5 yet).
>
> > >>> (2) Add a recheck callback.
> > >>>
> > >>> If the foreign data wrapper wants to adopt the solution you're
> > >>> proposing, the recheck callback can call
> > >>> ExecProcNode(outerPlanState(node)).  I don't think this should end up

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Thursday, November 12, 2015 6:54 PM
> To: Kaigai Kouhei(海外 浩平); Robert Haas
> Cc: Tom Lane; Kyotaro HORIGUCHI; pgsql-hackers@postgresql.org; Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> Robert and Kaigai-san,
> 
> Sorry, I sent in an unfinished email.
> 
> On 2015/11/12 15:30, Kouhei Kaigai wrote:
> >> On 2015/11/12 2:53, Robert Haas wrote:
> >>> On Sun, Nov 8, 2015 at 11:13 PM, Etsuro Fujita
> >>> <fujita.ets...@lab.ntt.co.jp> wrote:
> >>>> To test this change, I think we should update the postgres_fdw patch so 
> >>>> as
> >>>> to add the RecheckForeignScan.
> >>>>
> >>>> Having said that, as I said previously, I don't see much value in adding
> the
> >>>> callback routine, to be honest.  I know KaiGai-san considers that that 
> >>>> would
> >>>> be useful for custom joins, but I don't think that that would be useful 
> >>>> even
> >>>> for foreign joins, because I think that in case of foreign joins, the
> >>>> practical implementation of that routine in FDWs would be to create a
> >>>> secondary plan and execute that plan by performing ExecProcNode, as my 
> >>>> patch
> >>>> does [1].  Maybe I'm missing something, though.
> 
> >>> I really don't see why you're fighting on this point.  Making this a
> >>> generic feature will require only a few extra lines of code for FDW
> >>> authors.  If this were going to cause some great inconvenience for FDW
> >>> authors, then I'd agree it isn't worth it.  But I see zero evidence
> >>> that this is actually the case.
> 
> >> Really?  I think there would be not a little burden on an FDW author;
> >> when postgres_fdw delegates to the subplan to the remote server, for
> >> example, it would need to create a remote join query by looking at
> >> tuples possibly fetched and stored in estate->es_epqTuple[], send the
> >> query and receive the result during the callback routine.
> 
> > I cannot understand why it is the only solution.
> 
> I didn't say that.
> 
> >> Furthermore,
> >> what I'm most concerned about is that wouldn't be efficient. So, my
> 
> > You have to add "because ..." sentence here because I and Robert
> > think a little inefficiency is not a problem.
> 
> Sorry, my explanation was not enough.  The reason for that is that in
> the above postgres_fdw case for example, the overhead in sending the
> query to the remote end and transferring the result to the local end
> would not be negligible.  Yeah, we might be able to apply a special
> handling for the improved efficiency when using early row locking, but
> otherwise can we do the same thing?
>
It is trade-off. Late locking semantics allows to lock relatively smaller
number of remote rows, it will take extra latency.
Also, it became clear we have a challenge to pull a joined tuple at once.

> > Please don't start the sentence from "I think ...". We all knows
> > your opinion, but what I've wanted to see is "the reason why my
> > approach is valuable is ...".
> 
> I didn't say that my approach is *valuable* either.  What I think is, I
> see zero evidence that there is a good use-case for an FDW to do
> something other than doing an ExecProcNode in the callback routine, as I
> said below, so I don't see the need to add such a routine while that
> would cause maybe not a large, but not a little burden for writing such
> a routine on FDW authors.
>
It is quite natural because we cannot predicate what kind of extension
is implemented on FDW interface. You might know the initial version of
PG-Strom is implemented on FDW (about 4 years before...). If I would
continue to stick FDW, it became a FDW driver with own join engine.
(cstore_fdw may potentially support own join logic on top of their
columnar storage for instance?)

From the standpoint of interface design, if we would not admit flexibility
of implementation unless community don't see a working example, a reasonable
tactics *for extension author* is to follow the interface restriction even
if it is not best approach from his standpoint.
It does not mean the approach by majority is also best for the minority.
It just requires the minority a compromise.

> > Nobody prohibits postgres_fdw performs a secondary join here.
> > All you need to do is, picking up a sub-plan tree from FDW's private
> > field then call ExecProcNode() inside the callback.
>

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Sun, Nov 8, 2015 at 11:13 PM, Etsuro Fujita
 wrote:
> To test this change, I think we should update the postgres_fdw patch so as
> to add the RecheckForeignScan.
>
> Having said that, as I said previously, I don't see much value in adding the
> callback routine, to be honest.  I know KaiGai-san considers that that would
> be useful for custom joins, but I don't think that that would be useful even
> for foreign joins, because I think that in case of foreign joins, the
> practical implementation of that routine in FDWs would be to create a
> secondary plan and execute that plan by performing ExecProcNode, as my patch
> does [1].  Maybe I'm missing something, though.

I really don't see why you're fighting on this point.  Making this a
generic feature will require only a few extra lines of code for FDW
authors.  If this were going to cause some great inconvenience for FDW
authors, then I'd agree it isn't worth it.  But I see zero evidence
that this is actually the case.  From my point of view I'm now
thinking this solution has two parts:

(1) Let foreign scans have inner and outer subplans.  For this
purpose, we only need one, but it's no more work to enable both, so we
may as well.  If we had some reason, we could add a list of subplans
of arbitrary length, but there doesn't seem to be an urgent need for
that.

(2) Add a recheck callback.

If the foreign data wrapper wants to adopt the solution you're
proposing, the recheck callback can call
ExecProcNode(outerPlanState(node)).  I don't think this should end up
being more than a few lines of code, although of course we should
verify that.  So no problem: postgres_fdw and any other FDWs where the
remote side is a database can easily delegate to a subplan, and
anybody who wants to do something else still can.

What is not to like about that?

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/12 2:53, Robert Haas wrote:

On Sun, Nov 8, 2015 at 11:13 PM, Etsuro Fujita
 wrote:

To test this change, I think we should update the postgres_fdw patch so as
to add the RecheckForeignScan.

Having said that, as I said previously, I don't see much value in adding the
callback routine, to be honest.  I know KaiGai-san considers that that would
be useful for custom joins, but I don't think that that would be useful even
for foreign joins, because I think that in case of foreign joins, the
practical implementation of that routine in FDWs would be to create a
secondary plan and execute that plan by performing ExecProcNode, as my patch
does [1].  Maybe I'm missing something, though.



I really don't see why you're fighting on this point.  Making this a
generic feature will require only a few extra lines of code for FDW
authors.  If this were going to cause some great inconvenience for FDW
authors, then I'd agree it isn't worth it.  But I see zero evidence
that this is actually the case.


Really?  I think there would be not a little burden on an FDW author; 
when postgres_fdw delegates to the subplan to the remote server, for 
example, it would need to create a remote join query by looking at 
tuples possibly fetched and stored in estate->es_epqTuple[], send the 
query and receive the result during the callback routine.  Furthermore, 
what I'm most concerned about is that wouldn't be efficient.  So, my 
question about that approach is whether FDWs really do some thing like 
that during the callback routine, instead of performing a secondary join 
plan locally.  As I said before, I know that KaiGai-san considers that 
that approach would be useful for custom joins.  But I see zero evidence 
that there is a good use-case for an FDW.



From my point of view I'm now
thinking this solution has two parts:

(1) Let foreign scans have inner and outer subplans.  For this
purpose, we only need one, but it's no more work to enable both, so we
may as well.  If we had some reason, we could add a list of subplans
of arbitrary length, but there doesn't seem to be an urgent need for
that.

(2) Add a recheck callback.

If the foreign data wrapper wants to adopt the solution you're
proposing, the recheck callback can call
ExecProcNode(outerPlanState(node)).  I don't think this should end up
being more than a few lines of code, although of course we should
verify that.  So no problem: postgres_fdw and any other FDWs where the
remote side is a database can easily delegate to a subplan, and
anybody who wants to do something else still can.

What is not to like about that?





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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kyotaro HORIGUCHI]

Hello,


> > I really don't see why you're fighting on this point.  Making this a
> > generic feature will require only a few extra lines of code for FDW
> > authors.  If this were going to cause some great inconvenience for FDW
> > authors, then I'd agree it isn't worth it.  But I see zero evidence
> > that this is actually the case.
> 
> Really?  I think there would be not a little burden on an FDW author;
> when postgres_fdw delegates to the subplan to the remote server, for
> example, it would need to create a remote join query by looking at
> tuples possibly fetched and stored in estate->es_epqTuple[], send the
> query and receive the result during the callback routine.

Do you mind that FDW cannot generate a plan so that make a tuple
from eqpTules then apply fdw_quals from predefined executor
nodes?

The returned tuple itself can be stored in fdw_private as I think
Kiagai-san said before. So it is enough if we can fabricate a
Result node outerPlan of which is ForeignScan, which somehow
returns the tuple to examine.

I should be missing something, though.

regards,

> Furthermore, what I'm most concerned about is that wouldn't be
> efficient.  So, my question about that approach is whether FDWs really
> do some thing like that during the callback routine, instead of
> performing a secondary join plan locally.  As I said before, I know
> that KaiGai-san considers that that approach would be useful for
> custom joins.  But I see zero evidence that there is a good use-case
> for an FDW.
> 
> > From my point of view I'm now
> > thinking this solution has two parts:
> >
> > (1) Let foreign scans have inner and outer subplans.  For this
> > purpose, we only need one, but it's no more work to enable both, so we
> > may as well.  If we had some reason, we could add a list of subplans
> > of arbitrary length, but there doesn't seem to be an urgent need for
> > that.
> >
> > (2) Add a recheck callback.
> >
> > If the foreign data wrapper wants to adopt the solution you're
> > proposing, the recheck callback can call
> > ExecProcNode(outerPlanState(node)).  I don't think this should end up
> > being more than a few lines of code, although of course we should
> > verify that.  So no problem: postgres_fdw and any other FDWs where the
> > remote side is a database can easily delegate to a subplan, and
> > anybody who wants to do something else still can.
> >
> > What is not to like about that?

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Thursday, November 12, 2015 2:54 PM
> To: Robert Haas
> Cc: Kaigai Kouhei(海外 浩平); Tom Lane; Kyotaro HORIGUCHI;
> pgsql-hackers@postgresql.org; Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On 2015/11/12 2:53, Robert Haas wrote:
> > On Sun, Nov 8, 2015 at 11:13 PM, Etsuro Fujita
> > <fujita.ets...@lab.ntt.co.jp> wrote:
> >> To test this change, I think we should update the postgres_fdw patch so as
> >> to add the RecheckForeignScan.
> >>
> >> Having said that, as I said previously, I don't see much value in adding 
> >> the
> >> callback routine, to be honest.  I know KaiGai-san considers that that 
> >> would
> >> be useful for custom joins, but I don't think that that would be useful 
> >> even
> >> for foreign joins, because I think that in case of foreign joins, the
> >> practical implementation of that routine in FDWs would be to create a
> >> secondary plan and execute that plan by performing ExecProcNode, as my 
> >> patch
> >> does [1].  Maybe I'm missing something, though.
> 
> > I really don't see why you're fighting on this point.  Making this a
> > generic feature will require only a few extra lines of code for FDW
> > authors.  If this were going to cause some great inconvenience for FDW
> > authors, then I'd agree it isn't worth it.  But I see zero evidence
> > that this is actually the case.
> 
> Really?  I think there would be not a little burden on an FDW author;
> when postgres_fdw delegates to the subplan to the remote server, for
> example, it would need to create a remote join query by looking at
> tuples possibly fetched and stored in estate->es_epqTuple[], send the
> query and receive the result during the callback routine.
>
I cannot understand why it is the only solution.
Our assumption is, FDW driver knows the best way to do. So, you can
take the best way for your FDW driver - including what you want to
implement in the built-in feature.

> Furthermore,
> what I'm most concerned about is that wouldn't be efficient. So, my
>
You have to add "because ..." sentence here because I and Robert
think a little inefficiency is not a problem. If you try to
persuade other parsons who have different opinion, you need to
introduce WHY you have different conclusion. (Of course, we might
oversight something)
Please don't start the sentence from "I think ...". We all knows
your opinion, but what I've wanted to see is "the reason why my
approach is valuable is ...".

I never suggest something technically difficult, but it is
a problem on communication.

> question about that approach is whether FDWs really do some thing like
> that during the callback routine, instead of performing a secondary join
> plan locally.
>
Nobody prohibits postgres_fdw performs a secondary join here.
All you need to do is, picking up a sub-plan tree from FDW's private
field then call ExecProcNode() inside the callback.

> As I said before, I know that KaiGai-san considers that
> that approach would be useful for custom joins.  But I see zero evidence
> that there is a good use-case for an FDW.
> 
> > From my point of view I'm now
> > thinking this solution has two parts:
> >
> > (1) Let foreign scans have inner and outer subplans.  For this
> > purpose, we only need one, but it's no more work to enable both, so we
> > may as well.  If we had some reason, we could add a list of subplans
> > of arbitrary length, but there doesn't seem to be an urgent need for
> > that.
> >
> > (2) Add a recheck callback.
> >
> > If the foreign data wrapper wants to adopt the solution you're
> > proposing, the recheck callback can call
> > ExecProcNode(outerPlanState(node)).  I don't think this should end up
> > being more than a few lines of code, although of course we should
> > verify that.  So no problem: postgres_fdw and any other FDWs where the
> > remote side is a database can easily delegate to a subplan, and
> > anybody who wants to do something else still can.
> >
> > What is not to like about that?
> >

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

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/09 13:40, Kouhei Kaigai wrote:

Having said that, as I said previously, I don't see much value in adding
the callback routine, to be honest.  I know KaiGai-san considers that
that would be useful for custom joins, but I don't think that that would
be useful even for foreign joins, because I think that in case of
foreign joins, the practical implementation of that routine in FDWs
would be to create a secondary plan and execute that plan by performing
ExecProcNode, as my patch does [1].  Maybe I'm missing something, though.



I've never denied that alternative local sub-plan is one of the best
approach for postgres_fdw, however, I've also never heard why you can
say the best approach for postgres_fdw is definitely also best for
others.
If we would justify less flexible interface specification because of
comfort for a particular extension, it should not be an extension,
but a built-in feature.
My standpoint has been consistent through the discussion; we can never
predicate which feature shall be implemented on FDW interface, therefore,
we also cannot predicate which implementation is best for EPQ rechecks
also. Only FDW driver knows which is the "best" for them, not us.


What the RecheckForeignScan routine does for the foreign-join case would 
be the following for tuples stored in estate->es_epqTuple[]:


1. Apply relevant restriction clauses, including fdw_recheck_quals, to 
the tuples for the baserels involved in a foreign-join, and see if the 
tuples still pass the clauses.


2. If so, form a join tuple, while applying relevant join clauses to the 
tuples, and set the join tuple in the given slot.  Else set empty.


I think these would be more efficiently processed internally in core 
than externally in FDWs.  That's why I don't see much value in adding 
the routine.  I have to admit that that means no flexibility, though.


However, the routine as-is doesn't seem good enough, either.  For 
example, since the routine is called after each of the tuples was 
re-fetched from the remote end or re-computed from the whole-row var and 
stored in the corresponding estate->es_epqTuple[], the routine wouldn't 
allow for what Robert proposed in [2].  To do such a thing, I think we 
would probably need to change the existing EPQ machinery more 
drastically and rethink the right place for calling the routine.


Best regards,
Etsuro Fujita

[2] 
http://www.postgresql.org/message-id/ca+tgmozdpu_fcspozxxpd1xvyq3czcawd7-x3avwbkgsfoh...@mail.gmail.com




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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On 2015/11/09 13:40, Kouhei Kaigai wrote:
> >> Having said that, as I said previously, I don't see much value in adding
> >> the callback routine, to be honest.  I know KaiGai-san considers that
> >> that would be useful for custom joins, but I don't think that that would
> >> be useful even for foreign joins, because I think that in case of
> >> foreign joins, the practical implementation of that routine in FDWs
> >> would be to create a secondary plan and execute that plan by performing
> >> ExecProcNode, as my patch does [1].  Maybe I'm missing something, though.
> 
> > I've never denied that alternative local sub-plan is one of the best
> > approach for postgres_fdw, however, I've also never heard why you can
> > say the best approach for postgres_fdw is definitely also best for
> > others.
> > If we would justify less flexible interface specification because of
> > comfort for a particular extension, it should not be an extension,
> > but a built-in feature.
> > My standpoint has been consistent through the discussion; we can never
> > predicate which feature shall be implemented on FDW interface, therefore,
> > we also cannot predicate which implementation is best for EPQ rechecks
> > also. Only FDW driver knows which is the "best" for them, not us.
> 
> What the RecheckForeignScan routine does for the foreign-join case would
> be the following for tuples stored in estate->es_epqTuple[]:
> 
> 1. Apply relevant restriction clauses, including fdw_recheck_quals, to
> the tuples for the baserels involved in a foreign-join, and see if the
> tuples still pass the clauses.
>
It depends on how FDW driver has restriction clauses, but you should not
use fdw_recheck_quals to recheck individual base relations, because it is
initialized to run on the joined tuple according to fdw_scan_tlist, so
restriction clauses has to be kept in other private field.

> 2. If so, form a join tuple, while applying relevant join clauses to the
> tuples, and set the join tuple in the given slot.  Else set empty.
>
No need to form a joined tuple after the rechecks of base relations's
clauses. If FDW support only inner-join, it can reconstruct a joined
tuple first, then run fdw_recheck_quals (by caller) that contains both
relation's clauses and join clause.
FDW driver can choose its comfortable way according to its implementation
and capability.

> I think these would be more efficiently processed internally in core
> than externally in FDWs.  That's why I don't see much value in adding
> the routine.  I have to admit that that means no flexibility, though.
>
Something like "efficiently", "better", "reasonable" and etc... are
your opinions from your standpoint. Things important is why you
thought X is better and Y is worse. It is what I've wanted to see for
three months, but never seen.

Discussion will become unproductive without understanding of the
reason of different conclusion. Please don't omit why you think it
is "efficient" that can justify to enforce all FDW drivers
a particular implementation manner, as a part of interface contract.

> However, the routine as-is doesn't seem good enough, either.  For
> example, since the routine is called after each of the tuples was
> re-fetched from the remote end or re-computed from the whole-row var and
> stored in the corresponding estate->es_epqTuple[], the routine wouldn't
> allow for what Robert proposed in [2].  To do such a thing, I think we
> would probably need to change the existing EPQ machinery more
> drastically and rethink the right place for calling the routine.
>
Please also see my message:
http://www.postgresql.org/message-id/9a28c8860f777e439aa12e8aea7694f801161...@bpxm15gp.gisp.nec.co.jp

And, why Robert thought here is a tough challenge:
http://www.postgresql.org/message-id/CA+TgmoY5Lf+vYy1Bha=u7__s3qtmqp7d+gssfd+ln4xz6fy...@mail.gmail.com

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Kaigai Kouhei(海外 浩平)
> Sent: Sunday, November 08, 2015 12:38 AM
> To: 'Robert Haas'
> Cc: Etsuro Fujita; Tom Lane; Kyotaro HORIGUCHI; pgsql-hackers@postgresql.org;
> Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> > On Fri, Nov 6, 2015 at 9:42 AM, Kouhei Kaigai <kai...@ak.jp.nec.com> wrote:
> > > This patch needs to be rebased.
> > > One thing different from the latest version is fdw_recheck_quals of
> > > ForeignScan was added. So, ...
> > >
> > > (1) Principle is that FDW driver knows what qualifiers were pushed down
> > > and how does it kept in the private field. So, fdw_recheck_quals is
> > > redundant and to be reverted.
> > >
> > > (2) Even though the principle is as described in (1), however,
> > > wired logic in ForeignRecheck() and fdw_recheck_quals are useful
> > > default for most of FDW drivers. So, it shall be kept and valid
> > > only if RecheckForeignScan callback is not defined.
> > >
> > > Which is better approach for the v3 patch?
> > > My preference is (1), because fdw_recheck_quals is a new feature,
> > > thus, FDW driver has to be adjusted in v9.5 more or less, even if
> > > it already supports qualifier push-down.
> > > In general, interface becomes more graceful to stick its principle.
> >
> > fdw_recheck_quals seems likely to be very convenient for FDW authors,
> > and I think ripping it out would be a terrible decision.
> >
> OK, I try to co-exist fdw_recheck_quals and RecheckForeignScan callback.
> 
> > I think ForeignRecheck should first call ExecQual to test
> > fdw_recheck_quals.  If it returns false, return false.  If it returns
> > true, then give the FDW callback a chance, if one is defined.  If that
> > returns false, return false.   If we haven't yet returned false,
> > return true.
> >
> I think ExecQual on fdw_recheck_quals shall be called next to the
> RecheckForeignScan callback, because econtext->ecxt_scantuple shall
> not be reconstructed unless RecheckForeignScan callback is not called
> if scanrelid==0.
> 
> If RecheckForeignScan is called prior to ExecQual, FDW driver can
> take either of two options according to its preference.
> 
> (1) RecheckForeignScan callback reconstruct a joined tuple based on
> the primitive EPQ slots, but nothing are rechecked by itself.
> ForeignRecheck runs ExecQual on fdw_recheck_quals that represents
> qualifiers of base relations and join condition.
> 
> (2) RecheckForeignScan callback reconstruct a joined tuple based on
> the primitive EPQ slots, then rechecks qualifiers of base relations
> and join condition by itself. It put NIL on fdw_recheck_quals, so
> ExecQual in ForeignRecheck() always true.
> 
> In either case, we cannot use ExecQual prior to reconstruction of
> a joined tuple because only FDW driver knows how to reconstruct it.
> So, it means ForeignScan with scanrelid==0 always has to set NIL on
> fdw_recheck_quals, if we would put ExecQual prior to the callback.
>
The attached patch is an adjusted version of the previous one.
Even though it co-exists a new callback and fdw_recheck_quals,
the callback is kicked first as follows.


@@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
 
ResetExprContext(econtext);
 
+   /*
+* FDW driver has to recheck visibility of EPQ tuple towards
+* the scan qualifiers once it gets pushed down.
+* In addition, if this node represents a join sub-tree, not
+* a scan, FDW driver is also responsible to reconstruct
+* a joined tuple according to the primitive EPQ tuples.
+*/
+   if (fdwroutine->RecheckForeignScan)
+   {
+   if (!fdwroutine->RecheckForeignScan(node, slot))
+   return false;
+   }
return ExecQual(node->fdw_recheck_quals, econtext, false);
 }


If callback is invoked first, FDW driver can reconstruct a joined tuple
with its comfortable way, then remaining checks can be done by ExecQual
and fds_recheck_quals on the caller side.
If callback would be located on the tail, FDW driver has no choice.

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



pgsql-fdw-epq-recheck.v3.patch
Description: pgsql-fdw-epq-recheck.v3.patch

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> > 
> > @@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
> *slot)
> >
> > ResetExprContext(econtext);
> >
> > +   /*
> > +* FDW driver has to recheck visibility of EPQ tuple towards
> > +* the scan qualifiers once it gets pushed down.
> > +* In addition, if this node represents a join sub-tree, not
> > +* a scan, FDW driver is also responsible to reconstruct
> > +* a joined tuple according to the primitive EPQ tuples.
> > +*/
> > +   if (fdwroutine->RecheckForeignScan)
> > +   {
> > +   if (!fdwroutine->RecheckForeignScan(node, slot))
> > +   return false;
> > +   }
> > return ExecQual(node->fdw_recheck_quals, econtext, false);
> >   }
> > 
> >
> > If callback is invoked first, FDW driver can reconstruct a joined tuple
> > with its comfortable way, then remaining checks can be done by ExecQual
> > and fds_recheck_quals on the caller side.
> > If callback would be located on the tail, FDW driver has no choice.
> 
> To test this change, I think we should update the postgres_fdw patch so
> as to add the RecheckForeignScan.
> 
> Having said that, as I said previously, I don't see much value in adding
> the callback routine, to be honest.  I know KaiGai-san considers that
> that would be useful for custom joins, but I don't think that that would
> be useful even for foreign joins, because I think that in case of
> foreign joins, the practical implementation of that routine in FDWs
> would be to create a secondary plan and execute that plan by performing
> ExecProcNode, as my patch does [1].  Maybe I'm missing something, though.
>
I've never denied that alternative local sub-plan is one of the best
approach for postgres_fdw, however, I've also never heard why you can
say the best approach for postgres_fdw is definitely also best for
others.
If we would justify less flexible interface specification because of
comfort for a particular extension, it should not be an extension,
but a built-in feature.
My standpoint has been consistent through the discussion; we can never
predicate which feature shall be implemented on FDW interface, therefore,
we also cannot predicate which implementation is best for EPQ rechecks
also. Only FDW driver knows which is the "best" for them, not us.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 

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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/09 9:26, Kouhei Kaigai wrote:

I think ForeignRecheck should first call ExecQual to test
fdw_recheck_quals.  If it returns false, return false.  If it returns
true, then give the FDW callback a chance, if one is defined.  If that
returns false, return false.   If we haven't yet returned false,
return true.



I think ExecQual on fdw_recheck_quals shall be called next to the
RecheckForeignScan callback, because econtext->ecxt_scantuple shall
not be reconstructed unless RecheckForeignScan callback is not called
if scanrelid==0.


I agree with KaiGai-san.  I think we can define fdw_recheck_quals for 
the foreign-join case as quals not in scan.plan.qual, the same way as 
the simple foreign scan case.  (In other words, the quals would be 
defind as "otherclauses", ie, rinfo->is_pushed_down=true, that have been 
pushed down to the remote server.  For checking the fdw_recheck_quals, 
however, I think we should reconstruct the join tuple first, which I 
think is essential for cases where an outer join is performed remotely, 
to avoid changing the semantics.  BTW, in my patch [1], a secondary plan 
will be created to evaluate such otherclauses after reconstructing the 
join tuple.



The attached patch is an adjusted version of the previous one.
Even though it co-exists a new callback and fdw_recheck_quals,
the callback is kicked first as follows.


Thanks for the patch!



@@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)

ResetExprContext(econtext);

+   /*
+* FDW driver has to recheck visibility of EPQ tuple towards
+* the scan qualifiers once it gets pushed down.
+* In addition, if this node represents a join sub-tree, not
+* a scan, FDW driver is also responsible to reconstruct
+* a joined tuple according to the primitive EPQ tuples.
+*/
+   if (fdwroutine->RecheckForeignScan)
+   {
+   if (!fdwroutine->RecheckForeignScan(node, slot))
+   return false;
+   }
return ExecQual(node->fdw_recheck_quals, econtext, false);
  }


If callback is invoked first, FDW driver can reconstruct a joined tuple
with its comfortable way, then remaining checks can be done by ExecQual
and fds_recheck_quals on the caller side.
If callback would be located on the tail, FDW driver has no choice.


To test this change, I think we should update the postgres_fdw patch so 
as to add the RecheckForeignScan.


Having said that, as I said previously, I don't see much value in adding 
the callback routine, to be honest.  I know KaiGai-san considers that 
that would be useful for custom joins, but I don't think that that would 
be useful even for foreign joins, because I think that in case of 
foreign joins, the practical implementation of that routine in FDWs 
would be to create a secondary plan and execute that plan by performing 
ExecProcNode, as my patch does [1].  Maybe I'm missing something, though.


Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/5624d583.10...@lab.ntt.co.jp



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Fri, Nov 6, 2015 at 9:42 AM, Kouhei Kaigai  wrote:
> > This patch needs to be rebased.
> > One thing different from the latest version is fdw_recheck_quals of
> > ForeignScan was added. So, ...
> >
> > (1) Principle is that FDW driver knows what qualifiers were pushed down
> > and how does it kept in the private field. So, fdw_recheck_quals is
> > redundant and to be reverted.
> >
> > (2) Even though the principle is as described in (1), however,
> > wired logic in ForeignRecheck() and fdw_recheck_quals are useful
> > default for most of FDW drivers. So, it shall be kept and valid
> > only if RecheckForeignScan callback is not defined.
> >
> > Which is better approach for the v3 patch?
> > My preference is (1), because fdw_recheck_quals is a new feature,
> > thus, FDW driver has to be adjusted in v9.5 more or less, even if
> > it already supports qualifier push-down.
> > In general, interface becomes more graceful to stick its principle.
> 
> fdw_recheck_quals seems likely to be very convenient for FDW authors,
> and I think ripping it out would be a terrible decision.
>
OK, I try to co-exist fdw_recheck_quals and RecheckForeignScan callback.

> I think ForeignRecheck should first call ExecQual to test
> fdw_recheck_quals.  If it returns false, return false.  If it returns
> true, then give the FDW callback a chance, if one is defined.  If that
> returns false, return false.   If we haven't yet returned false,
> return true.
>
I think ExecQual on fdw_recheck_quals shall be called next to the
RecheckForeignScan callback, because econtext->ecxt_scantuple shall
not be reconstructed unless RecheckForeignScan callback is not called
if scanrelid==0.

If RecheckForeignScan is called prior to ExecQual, FDW driver can
take either of two options according to its preference.

(1) RecheckForeignScan callback reconstruct a joined tuple based on
the primitive EPQ slots, but nothing are rechecked by itself.
ForeignRecheck runs ExecQual on fdw_recheck_quals that represents
qualifiers of base relations and join condition.

(2) RecheckForeignScan callback reconstruct a joined tuple based on
the primitive EPQ slots, then rechecks qualifiers of base relations
and join condition by itself. It put NIL on fdw_recheck_quals, so
ExecQual in ForeignRecheck() always true.

In either case, we cannot use ExecQual prior to reconstruction of
a joined tuple because only FDW driver knows how to reconstruct it.
So, it means ForeignScan with scanrelid==0 always has to set NIL on
fdw_recheck_quals, if we would put ExecQual prior to the callback.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Fri, Nov 6, 2015 at 9:42 AM, Kouhei Kaigai  wrote:
> This patch needs to be rebased.
> One thing different from the latest version is fdw_recheck_quals of
> ForeignScan was added. So, ...
>
> (1) Principle is that FDW driver knows what qualifiers were pushed down
> and how does it kept in the private field. So, fdw_recheck_quals is
> redundant and to be reverted.
>
> (2) Even though the principle is as described in (1), however,
> wired logic in ForeignRecheck() and fdw_recheck_quals are useful
> default for most of FDW drivers. So, it shall be kept and valid
> only if RecheckForeignScan callback is not defined.
>
> Which is better approach for the v3 patch?
> My preference is (1), because fdw_recheck_quals is a new feature,
> thus, FDW driver has to be adjusted in v9.5 more or less, even if
> it already supports qualifier push-down.
> In general, interface becomes more graceful to stick its principle.

fdw_recheck_quals seems likely to be very convenient for FDW authors,
and I think ripping it out would be a terrible decision.

I think ForeignRecheck should first call ExecQual to test
fdw_recheck_quals.  If it returns false, return false.  If it returns
true, then give the FDW callback a chance, if one is defined.  If that
returns false, return false.   If we haven't yet returned false,
return true.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Sent: Friday, November 06, 2015 9:40 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Etsuro Fujita; Tom Lane; Kyotaro HORIGUCHI; pgsql-hackers@postgresql.org;
> Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On Tue, Nov 3, 2015 at 8:15 AM, Kouhei Kaigai <kai...@ak.jp.nec.com> wrote:
> > A challenge is that junk wholerow references on behalf of ROW_MARK_COPY
> > are injected by preprocess_targetlist(). It is earlier than the main path
> > consideration by query_planner(), thus, it is not predictable how remote
> > query shall be executed at this point.
> 
> Oh, dear.  That seems like a rather serious problem for my approach.
> 
> > If ROW_MARK_COPY, base tuple image is fetched using this junk attribute.
> > So, here is two options if we allow to put joined tuple on either of
> > es_epqTuple[].
> 
> Neither of these sounds viable to me.
> 
> I'm inclined to go back to something like what you proposed here:
>
Good :-)

> http://www.postgresql.org/message-id/9A28C8860F777E439AA12E8AEA7694F80114B89
> d...@bpxm15gp.gisp.nec.co.jp
>
This patch needs to be rebased.
One thing different from the latest version is fdw_recheck_quals of
ForeignScan was added. So, ...

(1) Principle is that FDW driver knows what qualifiers were pushed down
and how does it kept in the private field. So, fdw_recheck_quals is
redundant and to be reverted.

(2) Even though the principle is as described in (1), however,
wired logic in ForeignRecheck() and fdw_recheck_quals are useful
default for most of FDW drivers. So, it shall be kept and valid
only if RecheckForeignScan callback is not defined.

Which is better approach for the v3 patch?
My preference is (1), because fdw_recheck_quals is a new feature,
thus, FDW driver has to be adjusted in v9.5 more or less, even if
it already supports qualifier push-down.
In general, interface becomes more graceful to stick its principle.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Nov 3, 2015 at 8:15 AM, Kouhei Kaigai  wrote:
> A challenge is that junk wholerow references on behalf of ROW_MARK_COPY
> are injected by preprocess_targetlist(). It is earlier than the main path
> consideration by query_planner(), thus, it is not predictable how remote
> query shall be executed at this point.

Oh, dear.  That seems like a rather serious problem for my approach.

> If ROW_MARK_COPY, base tuple image is fetched using this junk attribute.
> So, here is two options if we allow to put joined tuple on either of
> es_epqTuple[].

Neither of these sounds viable to me.

I'm inclined to go back to something like what you proposed here:

http://www.postgresql.org/message-id/9a28c8860f777e439aa12e8aea7694f80114b...@bpxm15gp.gisp.nec.co.jp

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/04 18:50, Etsuro Fujita wrote:

On 2015/11/04 17:10, Kouhei Kaigai wrote:

On 2015/10/28 6:04, Robert Haas wrote:

On Tue, Oct 20, 2015 at 12:39 PM, Etsuro Fujita
 wrote:

Sorry, my explanation was not correct.  (Needed to take in
caffeine.) What
I'm concerned about is the following:

SELECT * FROM localtab JOIN (ft1 LEFT JOIN ft2 ON ft1.x = ft2.x) ON
localtab.id = ft1.id FOR UPDATE OF ft1



If an EPQ recheck was invoked
due to a concurrent transaction on the remote server that changed
only the
value x of the ft1 tuple previously retrieved, then we would have to
generate a fake ft1/ft2-join tuple with nulls for ft2. (Assume that
the ft2
tuple previously retrieved was not a null tuple.) However, I'm not
sure how
we can do that in ForeignRecheck; we can't know for example, which
one is
outer and which one is inner, without an alternative local join
execution
plan.  Maybe I'm missing something, though.



I would expect it to issue a new query like: SELECT * FROM ft1 LEFT
JOIN ft2 WHERE ft1.x = ft2.x AND ft1.tid = $0 AND ft2.tid = $1.



We assume here that ft1 uses late row locking, so I thought the above
SQL should include "FOR UPDATE of ft1".  But I still don't think that
that is right; the SQL with "FOR UPDATE of ft1" wouldn't generate the
fake ft1/ft2-join tuple with nulls for ft2, as expected.  The reason for
that is that the updated version of the ft1 tuple wouldn't satisfy the
ft1.tid = $0 condition in an EPQ recheck, because the ctid for the
updated version of the ft1 tuple has changed.  (IIUC, I think that if we
use a TID scan for ft1, the SQL would generate the expected result,
because I think that the TID condition would be ignored in the EPQ
recheck, but I don't think it's guaranteed to use a TID scan for ft1.)
Maybe I'm missing something, though.



It looks to me, we should not use ctid system column to identify remote
row when postgres_fdw tries to support late row locking.



The "rowid" should not be changed once it is fetched from the remote side
until it is actually updated, deleted or locked, for correct
identification.
If ctid is used for this purpose, it is safe only when remote row is
locked
when it is fetched - it is exactly early row locking behavior, isn't it?



In case of SELECT FOR UPDATE, I think we are allowed to use ctid to
identify target rows for late row locking, but I think the above SQL
should be changed to something like this:

SELECT * FROM (SELECT * FROM ft1 WHERE ft1.tid = $0 FOR UPDATE) ss1 LEFT
JOIN (SELECT * FROM ft2 WHERE ft2.tid = $1) ss2 ON ss1.x = ss2.x


I noticed that the modofied SQL was still wrong; ss1 would produce no 
tuple, if using eg, a sequential scan for ss1, as discussed above. 
Sheesh, where is my brain?


I still think we are allowed to do that, but what is the right SQL for 
that?  In the current implementation of postgres_fdw, we need not take 
into consideration that what was fetched was an updated version of the 
tuple rather than the same version previously obtained, since that 
always uses at least REPEATABLE READ in the remote session.  But 
otherwise it would be possible that what was fetched was an updated 
version of the tuple, having a different ctid value, which wouldn't 
satisfy the condition like "ft1.tid = $0" in ss1 any more.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kyotaro HORIGUCHI]

Hello,

At Thu, 5 Nov 2015 01:58:00 +, Kouhei Kaigai  wrote 
in <9a28c8860f777e439aa12e8aea7694f801162...@bpxm15gp.gisp.nec.co.jp>
> > So, as the third way, I propose to resurrect the abandoned
> > ForeinJoinState seems to be for the unearthed requirements. FDW
> > returns ForeignJoinPath, not ForeignScanPath then finally it
> > becomes ForeignJoinState, which is handeled as a join node with
> > no doubt.
> > 
> > What do you think about this?
> >
> Apart from EPQ issues, it is fundamentally impossible to reflect
> the remote join tree on local side, because remote server runs
> the partial join in their best or arbitrary way.
> If this ForeignJoinState has just a compatible join sub-tree, what
> is the difference from the alternative local join sub-plan?

I think the ForeignJoinState don't have subnodes and might has no
difference in its structure from ForeignScanState. Its
significant difference from ForeignScanState would be that the
core can properly handle the return from the node as a joined
tuple in ordinary way. Executor no more calls ExecScan for joined
tuples again.

> Even if we have another node, the roles of FDW driver is unchanged.
> It eventually needs to do them:
>  1. Recheck scan-qualifier of base foreign table
>  2. Recheck join-clause of remote joins
>  3. Reconstruct a joined tuple

Yes, the most significant point of this proposal is in not FDW
side but core side. 

> I try to estimate your intention...
> You say that ForeignScan with scanrelid==0 is not a scan actually,
> so it is problematic to call ExecScan on ExecForeignScan always.
> Thus, individual ForeignJoin shall be defined.
> Right?

Definitely.

> In case of scanrelid==0, it performs like a scan on pseudo relation
> that has record type defined by fdw_scan_tlist. The rows generated
> with this node are consists of rows in underlying base relations.
> A significant point is, FDW driver is responsible to generate the
> rows according to the fdw_scan_tlist. Once FDW driver generates rows,
> ExecScan() runs remaining tasks - execution of host clauses (although
> it is not easy to image remote join includes host clause has cheaper
> cost than others) and projection.

Agreed. The role of FDW won't be changed by introducing
ForeignJoin.

> One thing I can agree is, ForeignScan is enforced to use ExecScan,
> thus some FDW driver may concern about this hard-wired logic.
> If we try to make ForeignScan unbound from the ExecScan, I like to
> suggest to revise ExecForeignScan, just invoke a callback; then
> FDW driver can choose whether ExecScan is best or not.

Agreed. Calling ExecScan unconditionally from ForeignScan is the
cause of the root(?) cause I mentioned. Since there'd be no
difference in data structure between Foreign(Join), calling
fdwroutine->ExecForeignScan() or something instaed of ExecScan()
from ExecForeignScan could be the alternative and most promising
solution for all problems in focus now.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kyotaro HORIGUCHI]

Hello,

The attached small patch is what I have in mind now.

fdwroutine->ExecForeignScan may be unset if the FDW does nothing
special. And all the FDW routine needs is the node.

> Subject: [PATCH] Allow substitute ExecScan body for ExecForignScan
> 
> ForeignScan node may return joined tuple. This joined tuple cannot be
> handled properly by ExecScan during EQP recheck. This patch allows
> FDWs to give a special treat to such tuples.

regards,


> > One thing I can agree is, ForeignScan is enforced to use ExecScan,
> > thus some FDW driver may concern about this hard-wired logic.
> > If we try to make ForeignScan unbound from the ExecScan, I like to
> > suggest to revise ExecForeignScan, just invoke a callback; then
> > FDW driver can choose whether ExecScan is best or not.
> 
> Agreed. Calling ExecScan unconditionally from ForeignScan is the
> cause of the root(?) cause I mentioned. Since there'd be no
> difference in data structure between Foreign(Join), calling
> fdwroutine->ExecForeignScan() or something instaed of ExecScan()
> from ExecForeignScan could be the alternative and most promising
> solution for all problems in focus now.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From cddbb29bf09e33af38bc7690d1b78f4e20f363b3 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Fri, 6 Nov 2015 13:23:55 +0900
Subject: [PATCH] Allow substitute ExecScan body for ExecForignScan

ForeignScan node may return joined tuple. This joined tuple cannot be
handled properly by ExecScan during EQP recheck. This patch allows
FDWs to give a special treat to such tuples.
---
 src/backend/executor/nodeForeignscan.c | 3 +++
 src/include/foreign/fdwapi.h   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 6165e4a..f43a50b 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -100,6 +100,9 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
 TupleTableSlot *
 ExecForeignScan(ForeignScanState *node)
 {
+	if (node->fdwroutine->ExecForeignScan)
+		return node->fdwroutine->ExecForeignScan(node);
+
 	return ExecScan((ScanState *) node,
 	(ExecScanAccessMtd) ForeignNext,
 	(ExecScanRecheckMtd) ForeignRecheck);
diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h
index 69b48b4..564898d 100644
--- a/src/include/foreign/fdwapi.h
+++ b/src/include/foreign/fdwapi.h
@@ -41,6 +41,8 @@ typedef ForeignScan *(*GetForeignPlan_function) (PlannerInfo *root,
 typedef void (*BeginForeignScan_function) (ForeignScanState *node,
 	   int eflags);
 
+typedef TupleTableSlot *(*ExecForeignScan_function) (ForeignScanState *node);
+
 typedef TupleTableSlot *(*IterateForeignScan_function) (ForeignScanState *node);
 
 typedef void (*ReScanForeignScan_function) (ForeignScanState *node);
@@ -137,6 +139,7 @@ typedef struct FdwRoutine
 	GetForeignPaths_function GetForeignPaths;
 	GetForeignPlan_function GetForeignPlan;
 	BeginForeignScan_function BeginForeignScan;
+	ExecForeignScan_function ExecForeignScan;
 	IterateForeignScan_function IterateForeignScan;
 	ReScanForeignScan_function ReScanForeignScan;
 	EndForeignScan_function EndForeignScan;
-- 
1.8.3.1


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kyotaro HORIGUCHI]

Hi, I've caught up again.

> OK, so if we all agree that the joined-tuple optimization is just an
> option for the case where all the component tables use ROW_MARK_COPY,
> I'd propose to leave that for 9.6.

I still think that ExecScan is called under EPQ recheck without
EQP tuple for the *scan*.

The ForeignScan can be generated for a join and underlying
foreign scans and such execution node returns what the core
deesn't expect for any scan node. This is what I think is the
root cause of this problem.

So, as the third way, I propose to resurrect the abandoned
ForeinJoinState seems to be for the unearthed requirements. FDW
returns ForeignJoinPath, not ForeignScanPath then finally it
becomes ForeignJoinState, which is handeled as a join node with
no doubt.

What do you think about this?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Kyotaro HORIGUCHI [mailto:horiguchi.kyot...@lab.ntt.co.jp]
> Sent: Thursday, November 05, 2015 10:02 AM
> To: fujita.ets...@lab.ntt.co.jp
> Cc: Kaigai Kouhei(海外 浩平); robertmh...@gmail.com; t...@sss.pgh.pa.us;
> pgsql-hackers@postgresql.org; shigeru.han...@gmail.com
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> Hi, I've caught up again.
> 
> > OK, so if we all agree that the joined-tuple optimization is just an
> > option for the case where all the component tables use ROW_MARK_COPY,
> > I'd propose to leave that for 9.6.
> 
> I still think that ExecScan is called under EPQ recheck without
> EQP tuple for the *scan*.
> 
> The ForeignScan can be generated for a join and underlying
> foreign scans and such execution node returns what the core
> deesn't expect for any scan node. This is what I think is the
> root cause of this problem.
> 
> So, as the third way, I propose to resurrect the abandoned
> ForeinJoinState seems to be for the unearthed requirements. FDW
> returns ForeignJoinPath, not ForeignScanPath then finally it
> becomes ForeignJoinState, which is handeled as a join node with
> no doubt.
> 
> What do you think about this?
>
Apart from EPQ issues, it is fundamentally impossible to reflect
the remote join tree on local side, because remote server runs
the partial join in their best or arbitrary way.
If this ForeignJoinState has just a compatible join sub-tree, what
is the difference from the alternative local join sub-plan?

Even if we have another node, the roles of FDW driver is unchanged.
It eventually needs to do them:
 1. Recheck scan-qualifier of base foreign table
 2. Recheck join-clause of remote joins
 3. Reconstruct a joined tuple


I try to estimate your intention...
You say that ForeignScan with scanrelid==0 is not a scan actually,
so it is problematic to call ExecScan on ExecForeignScan always.
Thus, individual ForeignJoin shall be defined.
Right?


In case of scanrelid==0, it performs like a scan on pseudo relation
that has record type defined by fdw_scan_tlist. The rows generated
with this node are consists of rows in underlying base relations.
A significant point is, FDW driver is responsible to generate the
rows according to the fdw_scan_tlist. Once FDW driver generates rows,
ExecScan() runs remaining tasks - execution of host clauses (although
it is not easy to image remote join includes host clause has cheaper
cost than others) and projection.

One thing I can agree is, ForeignScan is enforced to use ExecScan,
thus some FDW driver may concern about this hard-wired logic.
If we try to make ForeignScan unbound from the ExecScan, I like to
suggest to revise ExecForeignScan, just invoke a callback; then
FDW driver can choose whether ExecScan is best or not.

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



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/03 22:15, Kouhei Kaigai wrote:

A challenge is that junk wholerow references on behalf of ROW_MARK_COPY
are injected by preprocess_targetlist(). It is earlier than the main path
consideration by query_planner(), thus, it is not predictable how remote
query shall be executed at this point.
If ROW_MARK_COPY, base tuple image is fetched using this junk attribute.
So, here is two options if we allow to put joined tuple on either of
es_epqTuple[].

options-1) We ignore record type definition. FDW returns a joined tuple
towards the whole-row reference of either of the base relations in this
join. The junk attribute shall be filtered out eventually and only FDW
driver shall see, so it is harmless to do (probably).
This option takes no big changes, however, we need a little brave to adopt.

options-2) We allow FDW/CSP to adjust target-list of the relevant nodes
after these paths get chosen by planner. It enables to remove whole-row
reference of base relations and add alternative whole-row reference instead
if FDW/CSP can support it.
This feature can be relevant to target-list push-down to the remote side,
not only EPQ rechecks, because adjustment of target-list means we allows
FDW/CSP to determine which expression shall be executed locally, or shall
not be.
I think, this option is more straightforward, however, needs a little bit
deeper consideration, because we have to design the best hook point and
need to ensure how path-ification will perform.

Therefore, I think we need two steps towards the entire solution.
Step-1) FDW/CSP will recheck base EPQ tuples and support local
reconstruction on the fly. It does not need something special
enhancement on the planner - so we can fix up by v9.5 release.
Step-2) FDW/CSP will support adjustment of target-list to add whole-row
reference of joined tuple instead of multiple base relations, then FDW/CSP
will be able to put a joined tuple on either of EPQ slot if it wants - it
takes a new feature enhancement, so v9.6 is a suitable timeline.

How about your opinion towards the direction?
I don't want to drop extra optimization opportunity, however, we are now in
November. I don't have enough brave to add none-obvious new feature here.


I think we need to consider a general solution that can be applied not 
only to the case where the component tables in a foreign join all use 
ROW_MARK_COPY but to the case where those tables use different rowmark 
types such as ROW_MARK_COPY and ROW_MARK_EXCLUSIVE, as I pointed out 
upthread.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On 2015/10/28 6:04, Robert Haas wrote:
> > On Tue, Oct 20, 2015 at 12:39 PM, Etsuro Fujita
> >  wrote:
> >> Sorry, my explanation was not correct.  (Needed to take in caffeine.) What
> >> I'm concerned about is the following:
> >>
> >> SELECT * FROM localtab JOIN (ft1 LEFT JOIN ft2 ON ft1.x = ft2.x) ON
> >> localtab.id = ft1.id FOR UPDATE OF ft1
> >>
> >> LockRows
> >> -> Nested Loop
> >>   Join Filter: (localtab.id = ft1.id)
> >>   -> Seq Scan on localtab
> >>   -> Foreign Scan on 
> >>Remote SQL: SELECT * FROM ft1 LEFT JOIN ft2 WHERE ft1.x = ft2.x
> >> FOR UPDATE OF ft1
> >>
> >> Assume that ft1 performs late row locking.
> 
> > If the SQL includes "FOR UPDATE of ft1", then it clearly performs
> > early row locking.  I assume you meant to omit that.
> 
> Right.  Sorry for my mistake.
> 
> >> If an EPQ recheck was invoked
> >> due to a concurrent transaction on the remote server that changed only the
> >> value x of the ft1 tuple previously retrieved, then we would have to
> >> generate a fake ft1/ft2-join tuple with nulls for ft2. (Assume that the ft2
> >> tuple previously retrieved was not a null tuple.) However, I'm not sure how
> >> we can do that in ForeignRecheck; we can't know for example, which one is
> >> outer and which one is inner, without an alternative local join execution
> >> plan.  Maybe I'm missing something, though.
> 
> > I would expect it to issue a new query like: SELECT * FROM ft1 LEFT
> > JOIN ft2 WHERE ft1.x = ft2.x AND ft1.tid = $0 AND ft2.tid = $1.
> 
> We assume here that ft1 uses late row locking, so I thought the above
> SQL should include "FOR UPDATE of ft1".  But I still don't think that
> that is right; the SQL with "FOR UPDATE of ft1" wouldn't generate the
> fake ft1/ft2-join tuple with nulls for ft2, as expected.  The reason for
> that is that the updated version of the ft1 tuple wouldn't satisfy the
> ft1.tid = $0 condition in an EPQ recheck, because the ctid for the
> updated version of the ft1 tuple has changed.  (IIUC, I think that if we
> use a TID scan for ft1, the SQL would generate the expected result,
> because I think that the TID condition would be ignored in the EPQ
> recheck, but I don't think it's guaranteed to use a TID scan for ft1.)
> Maybe I'm missing something, though.
>
It looks to me, we should not use ctid system column to identify remote
row when postgres_fdw tries to support late row locking.

The documentation says:
  
http://www.postgresql.org/docs/devel/static/fdw-callbacks.html#FDW-CALLBACKS-UPDATE

  UPDATE and DELETE operations are performed against rows previously
  fetched by the table-scanning functions. The FDW may need extra information,
  such as a row ID or the values of primary-key columns, to ensure that it can
  identify the exact row to update or delete

The "rowid" should not be changed once it is fetched from the remote side
until it is actually updated, deleted or locked, for correct identification.
If ctid is used for this purpose, it is safe only when remote row is locked
when it is fetched - it is exactly early row locking behavior, isn't it?

> > This should be significantly more efficient than fetching the base
> > rows from each of two tables with two separate queries.
> 
> Maybe I think we could fix the SQL, so I have to admit that, but I'm
> just wondering (1) what would happen for the case when ft1 uses late row
> rocking and ft2 uses early row rocking and (2) that would be still more
> efficient than re-fetching only the base row from ft1.
>
It should be decision by FDW driver. It is not easy to estimate a certain
FDW driver mixes up early and late locking policy within a same remote join
query. Do you really want to support such a mysterious implementation?

Or, do you expect all the FDW driver is enforced to return a joined tuple
if remote join case? It is different from my idea; it shall be an extra
optimization option if FDW can fetch a joined tuple at once, but not always.
So, if FDW driver does not support this optimal behavior, your driver can
fetch two base tables then run local alternative join (or something other).

> What I thought to improve the efficiency in the secondary-plan approach
> that I proposed was that if we could parallelize re-fetching foreign
> rows in ExecLockRows and EvalPlanQualFetchRowMarks, we would be able to
> improve the efficiency not only for the case when performing a join of
> foreign tables remotely but for the case when performing the join locally.
>
Parallelism is not a magic bullet...

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Wednesday, November 04, 2015 5:11 PM
> To: Kaigai Kouhei(海外 浩平); Robert Haas
> Cc: Tom Lane; Kyotaro HORIGUCHI; pgsql-hackers@postgresql.org; Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On 2015/11/03 22:15, Kouhei Kaigai wrote:
> > A challenge is that junk wholerow references on behalf of ROW_MARK_COPY
> > are injected by preprocess_targetlist(). It is earlier than the main path
> > consideration by query_planner(), thus, it is not predictable how remote
> > query shall be executed at this point.
> > If ROW_MARK_COPY, base tuple image is fetched using this junk attribute.
> > So, here is two options if we allow to put joined tuple on either of
> > es_epqTuple[].
> >
> > options-1) We ignore record type definition. FDW returns a joined tuple
> > towards the whole-row reference of either of the base relations in this
> > join. The junk attribute shall be filtered out eventually and only FDW
> > driver shall see, so it is harmless to do (probably).
> > This option takes no big changes, however, we need a little brave to adopt.
> >
> > options-2) We allow FDW/CSP to adjust target-list of the relevant nodes
> > after these paths get chosen by planner. It enables to remove whole-row
> > reference of base relations and add alternative whole-row reference instead
> > if FDW/CSP can support it.
> > This feature can be relevant to target-list push-down to the remote side,
> > not only EPQ rechecks, because adjustment of target-list means we allows
> > FDW/CSP to determine which expression shall be executed locally, or shall
> > not be.
> > I think, this option is more straightforward, however, needs a little bit
> > deeper consideration, because we have to design the best hook point and
> > need to ensure how path-ification will perform.
> >
> > Therefore, I think we need two steps towards the entire solution.
> > Step-1) FDW/CSP will recheck base EPQ tuples and support local
> > reconstruction on the fly. It does not need something special
> > enhancement on the planner - so we can fix up by v9.5 release.
> > Step-2) FDW/CSP will support adjustment of target-list to add whole-row
> > reference of joined tuple instead of multiple base relations, then FDW/CSP
> > will be able to put a joined tuple on either of EPQ slot if it wants - it
> > takes a new feature enhancement, so v9.6 is a suitable timeline.
> >
> > How about your opinion towards the direction?
> > I don't want to drop extra optimization opportunity, however, we are now in
> > November. I don't have enough brave to add none-obvious new feature here.
> 
> I think we need to consider a general solution that can be applied not
> only to the case where the component tables in a foreign join all use
> ROW_MARK_COPY but to the case where those tables use different rowmark
> types such as ROW_MARK_COPY and ROW_MARK_EXCLUSIVE, as I pointed out
> upthread.
>
In mixture case, FDW/CSP can choose local recheck & reconstruction based
on the EPQ tuples of base relation. Nobody enforce FDW/CSP to return
a joined tuple always even if author don't want to support the feature.
Why do you think it is not a generic solution? FDW/CSP driver "can choose"
the best solution according to its implementation and capability.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/04 17:10, Kouhei Kaigai wrote:

On 2015/10/28 6:04, Robert Haas wrote:

On Tue, Oct 20, 2015 at 12:39 PM, Etsuro Fujita
 wrote:

Sorry, my explanation was not correct.  (Needed to take in caffeine.) What
I'm concerned about is the following:

SELECT * FROM localtab JOIN (ft1 LEFT JOIN ft2 ON ft1.x = ft2.x) ON
localtab.id = ft1.id FOR UPDATE OF ft1

LockRows
-> Nested Loop
   Join Filter: (localtab.id = ft1.id)
   -> Seq Scan on localtab
   -> Foreign Scan on 
Remote SQL: SELECT * FROM ft1 LEFT JOIN ft2 WHERE ft1.x = ft2.x
FOR UPDATE OF ft1

Assume that ft1 performs late row locking.



If the SQL includes "FOR UPDATE of ft1", then it clearly performs
early row locking.  I assume you meant to omit that.



If an EPQ recheck was invoked
due to a concurrent transaction on the remote server that changed only the
value x of the ft1 tuple previously retrieved, then we would have to
generate a fake ft1/ft2-join tuple with nulls for ft2. (Assume that the ft2
tuple previously retrieved was not a null tuple.) However, I'm not sure how
we can do that in ForeignRecheck; we can't know for example, which one is
outer and which one is inner, without an alternative local join execution
plan.  Maybe I'm missing something, though.



I would expect it to issue a new query like: SELECT * FROM ft1 LEFT
JOIN ft2 WHERE ft1.x = ft2.x AND ft1.tid = $0 AND ft2.tid = $1.



We assume here that ft1 uses late row locking, so I thought the above
SQL should include "FOR UPDATE of ft1".  But I still don't think that
that is right; the SQL with "FOR UPDATE of ft1" wouldn't generate the
fake ft1/ft2-join tuple with nulls for ft2, as expected.  The reason for
that is that the updated version of the ft1 tuple wouldn't satisfy the
ft1.tid = $0 condition in an EPQ recheck, because the ctid for the
updated version of the ft1 tuple has changed.  (IIUC, I think that if we
use a TID scan for ft1, the SQL would generate the expected result,
because I think that the TID condition would be ignored in the EPQ
recheck, but I don't think it's guaranteed to use a TID scan for ft1.)
Maybe I'm missing something, though.



It looks to me, we should not use ctid system column to identify remote
row when postgres_fdw tries to support late row locking.

The documentation says:
   
http://www.postgresql.org/docs/devel/static/fdw-callbacks.html#FDW-CALLBACKS-UPDATE

   UPDATE and DELETE operations are performed against rows previously
   fetched by the table-scanning functions. The FDW may need extra information,
   such as a row ID or the values of primary-key columns, to ensure that it can
   identify the exact row to update or delete

The "rowid" should not be changed once it is fetched from the remote side
until it is actually updated, deleted or locked, for correct identification.
If ctid is used for this purpose, it is safe only when remote row is locked
when it is fetched - it is exactly early row locking behavior, isn't it?


Yeah, we should use early row locking for a target foreign table in 
UPDATE/DELETE.


In case of SELECT FOR UPDATE, I think we are allowed to use ctid to 
identify target rows for late row locking, but I think the above SQL 
should be changed to something like this:


SELECT * FROM (SELECT * FROM ft1 WHERE ft1.tid = $0 FOR UPDATE) ss1 LEFT 
JOIN (SELECT * FROM ft2 WHERE ft2.tid = $1) ss2 ON ss1.x = ss2.x



This should be significantly more efficient than fetching the base
rows from each of two tables with two separate queries.



Maybe I think we could fix the SQL, so I have to admit that, but I'm
just wondering (1) what would happen for the case when ft1 uses late row
rocking and ft2 uses early row rocking and (2) that would be still more
efficient than re-fetching only the base row from ft1.



It should be decision by FDW driver. It is not easy to estimate a certain
FDW driver mixes up early and late locking policy within a same remote join
query. Do you really want to support such a mysterious implementation?


Yeah, the reason for that is because GetForeignRowMarkType allows that.


Or, do you expect all the FDW driver is enforced to return a joined tuple
if remote join case?


No.  That wouldn't make sense if at least one component table involved 
in a foreign join uses the rowmark type other than ROW_MARK_COPY.



It is different from my idea; it shall be an extra
optimization option if FDW can fetch a joined tuple at once, but not always.
So, if FDW driver does not support this optimal behavior, your driver can
fetch two base tables then run local alternative join (or something other).


OK, so if we all agree that the joined-tuple optimization is just an 
option for the case where all the component tables use ROW_MARK_COPY, 
I'd propose to leave that for 9.6.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/11/04 17:28, Kouhei Kaigai wrote:

I think we need to consider a general solution that can be applied not
only to the case where the component tables in a foreign join all use
ROW_MARK_COPY but to the case where those tables use different rowmark
types such as ROW_MARK_COPY and ROW_MARK_EXCLUSIVE, as I pointed out
upthread.



In mixture case, FDW/CSP can choose local recheck & reconstruction based
on the EPQ tuples of base relation. Nobody enforce FDW/CSP to return
a joined tuple always even if author don't want to support the feature.
Why do you think it is not a generic solution? FDW/CSP driver "can choose"
the best solution according to its implementation and capability.


It looked to me that you were discussing only the case where component 
foreign tables in a foreign join all use ROW_MARK_COPY, so I commented 
that.  Sorry for my misunderstanding.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Thu, Oct 29, 2015 at 6:05 AM, Kouhei Kaigai  wrote:
> > In this case, the EPQ slot to store the joined tuple is still
> > a challenge to be solved.
> >
> > Is it possible to use one or any of EPQ slots that are setup for
> > base relations but represented by ForeignScan/CustomScan?
> 
> Yes, I proposed that exact thing upthread.
> 
> > In case when ForeignScan run a remote join that involves three
> > base foreign tables (relid=2, 3, 5 for example), for example,
> > no other code touches this slot. So, it is safe even if we put
> > a joined tuple on EPQ slots of underlying base relations.
> >
> > In this case, EPQ slots are initialized as below:
> >
> >   es_epqTuple[0] ... EPQ tuple of base relation (relid=1)
> >   es_epqTuple[1] ... EPQ of the joined tuple (for relis=2, 3 5)
> >   es_epqTuple[2] ... EPQ of the joined tuple (for relis=2, 3 5), copy of 
> > above
> >   es_epqTuple[3] ... EPQ tuple of base relation (relid=4)
> >   es_epqTuple[4] ... EPQ of the joined tuple (for relis=2, 3 5), copy of 
> > above
> >   es_epqTuple[5] ... EPQ tuple of base relation (relid=6)
> 
> You don't really need to initialize them all.  You can just initialize
> es_epqTuple[1] and leave 2 and 4 unused.
> 
> > Then, if FDW/CSP is designed to utilize the preliminary joined
> > tuples rather than local join, it can just raise the tuple kept
> > in one of the EPQ slots for underlying base relations.
> > If FDW/CSP prefers local join, it can perform as like local join
> > doing; check join condition and construct a joined tuple by itself
> > or by alternative plan.
> 
> Right.
>
A challenge is that junk wholerow references on behalf of ROW_MARK_COPY
are injected by preprocess_targetlist(). It is earlier than the main path
consideration by query_planner(), thus, it is not predictable how remote
query shall be executed at this point.
If ROW_MARK_COPY, base tuple image is fetched using this junk attribute.
So, here is two options if we allow to put joined tuple on either of
es_epqTuple[].

options-1) We ignore record type definition. FDW returns a joined tuple
towards the whole-row reference of either of the base relations in this
join. The junk attribute shall be filtered out eventually and only FDW
driver shall see, so it is harmless to do (probably).
This option takes no big changes, however, we need a little brave to adopt.

options-2) We allow FDW/CSP to adjust target-list of the relevant nodes
after these paths get chosen by planner. It enables to remove whole-row
reference of base relations and add alternative whole-row reference instead
if FDW/CSP can support it.
This feature can be relevant to target-list push-down to the remote side,
not only EPQ rechecks, because adjustment of target-list means we allows
FDW/CSP to determine which expression shall be executed locally, or shall
not be.
I think, this option is more straightforward, however, needs a little bit
deeper consideration, because we have to design the best hook point and
need to ensure how path-ification will perform.

Therefore, I think we need two steps towards the entire solution.
Step-1) FDW/CSP will recheck base EPQ tuples and support local
reconstruction on the fly. It does not need something special
enhancement on the planner - so we can fix up by v9.5 release.
Step-2) FDW/CSP will support adjustment of target-list to add whole-row
reference of joined tuple instead of multiple base relations, then FDW/CSP
will be able to put a joined tuple on either of EPQ slot if it wants - it
takes a new feature enhancement, so v9.6 is a suitable timeline.

How about your opinion towards the direction?
I don't want to drop extra optimization opportunity, however, we are now in
November. I don't have enough brave to add none-obvious new feature here.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/10/28 6:04, Robert Haas wrote:

On Tue, Oct 20, 2015 at 12:39 PM, Etsuro Fujita
 wrote:

Sorry, my explanation was not correct.  (Needed to take in caffeine.) What
I'm concerned about is the following:

SELECT * FROM localtab JOIN (ft1 LEFT JOIN ft2 ON ft1.x = ft2.x) ON
localtab.id = ft1.id FOR UPDATE OF ft1

LockRows
-> Nested Loop
  Join Filter: (localtab.id = ft1.id)
  -> Seq Scan on localtab
  -> Foreign Scan on 
   Remote SQL: SELECT * FROM ft1 LEFT JOIN ft2 WHERE ft1.x = ft2.x
FOR UPDATE OF ft1

Assume that ft1 performs late row locking.



If the SQL includes "FOR UPDATE of ft1", then it clearly performs
early row locking.  I assume you meant to omit that.


Right.  Sorry for my mistake.


If an EPQ recheck was invoked
due to a concurrent transaction on the remote server that changed only the
value x of the ft1 tuple previously retrieved, then we would have to
generate a fake ft1/ft2-join tuple with nulls for ft2. (Assume that the ft2
tuple previously retrieved was not a null tuple.) However, I'm not sure how
we can do that in ForeignRecheck; we can't know for example, which one is
outer and which one is inner, without an alternative local join execution
plan.  Maybe I'm missing something, though.



I would expect it to issue a new query like: SELECT * FROM ft1 LEFT
JOIN ft2 WHERE ft1.x = ft2.x AND ft1.tid = $0 AND ft2.tid = $1.


We assume here that ft1 uses late row locking, so I thought the above 
SQL should include "FOR UPDATE of ft1".  But I still don't think that 
that is right; the SQL with "FOR UPDATE of ft1" wouldn't generate the 
fake ft1/ft2-join tuple with nulls for ft2, as expected.  The reason for 
that is that the updated version of the ft1 tuple wouldn't satisfy the 
ft1.tid = $0 condition in an EPQ recheck, because the ctid for the 
updated version of the ft1 tuple has changed.  (IIUC, I think that if we 
use a TID scan for ft1, the SQL would generate the expected result, 
because I think that the TID condition would be ignored in the EPQ 
recheck, but I don't think it's guaranteed to use a TID scan for ft1.) 
Maybe I'm missing something, though.



This should be significantly more efficient than fetching the base
rows from each of two tables with two separate queries.


Maybe I think we could fix the SQL, so I have to admit that, but I'm 
just wondering (1) what would happen for the case when ft1 uses late row 
rocking and ft2 uses early row rocking and (2) that would be still more 
efficient than re-fetching only the base row from ft1.


What I thought to improve the efficiency in the secondary-plan approach 
that I proposed was that if we could parallelize re-fetching foreign 
rows in ExecLockRows and EvalPlanQualFetchRowMarks, we would be able to 
improve the efficiency not only for the case when performing a join of 
foreign tables remotely but for the case when performing the join locally.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Thu, Oct 29, 2015 at 6:05 AM, Kouhei Kaigai  wrote:
> In this case, the EPQ slot to store the joined tuple is still
> a challenge to be solved.
>
> Is it possible to use one or any of EPQ slots that are setup for
> base relations but represented by ForeignScan/CustomScan?

Yes, I proposed that exact thing upthread.

> In case when ForeignScan run a remote join that involves three
> base foreign tables (relid=2, 3, 5 for example), for example,
> no other code touches this slot. So, it is safe even if we put
> a joined tuple on EPQ slots of underlying base relations.
>
> In this case, EPQ slots are initialized as below:
>
>   es_epqTuple[0] ... EPQ tuple of base relation (relid=1)
>   es_epqTuple[1] ... EPQ of the joined tuple (for relis=2, 3 5)
>   es_epqTuple[2] ... EPQ of the joined tuple (for relis=2, 3 5), copy of above
>   es_epqTuple[3] ... EPQ tuple of base relation (relid=4)
>   es_epqTuple[4] ... EPQ of the joined tuple (for relis=2, 3 5), copy of above
>   es_epqTuple[5] ... EPQ tuple of base relation (relid=6)

You don't really need to initialize them all.  You can just initialize
es_epqTuple[1] and leave 2 and 4 unused.

> Then, if FDW/CSP is designed to utilize the preliminary joined
> tuples rather than local join, it can just raise the tuple kept
> in one of the EPQ slots for underlying base relations.
> If FDW/CSP prefers local join, it can perform as like local join
> doing; check join condition and construct a joined tuple by itself
> or by alternative plan.

Right.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> On Tue, Oct 20, 2015 at 12:39 PM, Etsuro Fujita
>  wrote:
> > Sorry, my explanation was not correct.  (Needed to take in caffeine.) What
> > I'm concerned about is the following:
> >
> > SELECT * FROM localtab JOIN (ft1 LEFT JOIN ft2 ON ft1.x = ft2.x) ON
> > localtab.id = ft1.id FOR UPDATE OF ft1
> >
> > LockRows
> > -> Nested Loop
> >  Join Filter: (localtab.id = ft1.id)
> >  -> Seq Scan on localtab
> >  -> Foreign Scan on 
> >   Remote SQL: SELECT * FROM ft1 LEFT JOIN ft2 WHERE ft1.x = ft2.x
> > FOR UPDATE OF ft1
> >
> > Assume that ft1 performs late row locking.
> 
> If the SQL includes "FOR UPDATE of ft1", then it clearly performs
> early row locking.  I assume you meant to omit that.
> 
> > If an EPQ recheck was invoked
> > due to a concurrent transaction on the remote server that changed only the
> > value x of the ft1 tuple previously retrieved, then we would have to
> > generate a fake ft1/ft2-join tuple with nulls for ft2. (Assume that the ft2
> > tuple previously retrieved was not a null tuple.) However, I'm not sure how
> > we can do that in ForeignRecheck; we can't know for example, which one is
> > outer and which one is inner, without an alternative local join execution
> > plan.  Maybe I'm missing something, though.
> 
> I would expect it to issue a new query like: SELECT * FROM ft1 LEFT
> JOIN ft2 WHERE ft1.x = ft2.x AND ft1.tid = $0 AND ft2.tid = $1.
> 
> This should be significantly more efficient than fetching the base
> rows from each of two tables with two separate queries.
>
In this case, the EPQ slot to store the joined tuple is still
a challenge to be solved.

Is it possible to use one or any of EPQ slots that are setup for
base relations but represented by ForeignScan/CustomScan?
In case when ForeignScan run a remote join that involves three
base foreign tables (relid=2, 3, 5 for example), for example,
no other code touches this slot. So, it is safe even if we put
a joined tuple on EPQ slots of underlying base relations.

In this case, EPQ slots are initialized as below:

  es_epqTuple[0] ... EPQ tuple of base relation (relid=1)
  es_epqTuple[1] ... EPQ of the joined tuple (for relis=2, 3 5)
  es_epqTuple[2] ... EPQ of the joined tuple (for relis=2, 3 5), copy of above
  es_epqTuple[3] ... EPQ tuple of base relation (relid=4)
  es_epqTuple[4] ... EPQ of the joined tuple (for relis=2, 3 5), copy of above
  es_epqTuple[5] ... EPQ tuple of base relation (relid=6)

Also, FDW/CSP shall be responsible to return a joined tuple
as a result for whole-row reference of underlying base relation.
(One other challenge is how to handle the case when user explicitly
required a whole-row reference...Hmm...)

Then, if FDW/CSP is designed to utilize the preliminary joined
tuples rather than local join, it can just raise the tuple kept
in one of the EPQ slots for underlying base relations.
If FDW/CSP prefers local join, it can perform as like local join
doing; check join condition and construct a joined tuple by itself
or by alternative plan.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Robert Haas]

On Tue, Oct 20, 2015 at 12:39 PM, Etsuro Fujita
 wrote:
> Sorry, my explanation was not correct.  (Needed to take in caffeine.) What
> I'm concerned about is the following:
>
> SELECT * FROM localtab JOIN (ft1 LEFT JOIN ft2 ON ft1.x = ft2.x) ON
> localtab.id = ft1.id FOR UPDATE OF ft1
>
> LockRows
> -> Nested Loop
>  Join Filter: (localtab.id = ft1.id)
>  -> Seq Scan on localtab
>  -> Foreign Scan on 
>   Remote SQL: SELECT * FROM ft1 LEFT JOIN ft2 WHERE ft1.x = ft2.x
> FOR UPDATE OF ft1
>
> Assume that ft1 performs late row locking.

If the SQL includes "FOR UPDATE of ft1", then it clearly performs
early row locking.  I assume you meant to omit that.

> If an EPQ recheck was invoked
> due to a concurrent transaction on the remote server that changed only the
> value x of the ft1 tuple previously retrieved, then we would have to
> generate a fake ft1/ft2-join tuple with nulls for ft2. (Assume that the ft2
> tuple previously retrieved was not a null tuple.) However, I'm not sure how
> we can do that in ForeignRecheck; we can't know for example, which one is
> outer and which one is inner, without an alternative local join execution
> plan.  Maybe I'm missing something, though.

I would expect it to issue a new query like: SELECT * FROM ft1 LEFT
JOIN ft2 WHERE ft1.x = ft2.x AND ft1.tid = $0 AND ft2.tid = $1.

This should be significantly more efficient than fetching the base
rows from each of two tables with two separate queries.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/10/20 9:36, Kouhei Kaigai wrote:

Even if we fetch whole-row of both side, join pushdown is exactly working
because we can receive less number of rows than local join + 2 of foreign-
scan. (If planner works well, we can expect join-path that increases number
of rows shall be dropped.)

One downside of my proposition is growth of width for individual rows.
It is a trade-off situation. The above approach takes no changes for
existing EPQ infrastructure, thus, its implementation design is clear.
On the other hands, your approach will reduce traffic over the network,
however, it is still unclear how we integrate scanrelid==0 with EPQ
infrastructure.


I agree with KaiGai-san that his proposition (or my proposition based on 
secondary plans) is still a performance improvement over the current 
implementation on local joining plus early row locking, since that that 
wouldn't have to transfer useless data that didn't satisfy join 
conditions at all!



On the other hands, in case of custom-scan that takes underlying local
scan-nodes, thus, any kind of ROW_MARK_* except for ROW_MARK_COPY will
happen. I think width of the joined tuples are relatively minor issue
than FDW cases. However, we cannot expect the fetched rows are protected
by early row-locking mechanism, so probability of re-fetching rows and
reconstruction of joined-tuple has relatively higher priority.


I see.


There is also some possible loss of efficiency with this approach.
Suppose that we have two tables ft1 and ft2 which are being joined,
and we push down the join.  They are being joined on an integer
column, and the join needs to select several other columns as well.
However, ft1 and ft2 are very wide tables that also contain some text
columns.   The query is like this:

SELECT localtab.a, ft1.p, ft2.p FROM localtab LEFT JOIN (ft1 JOIN ft2
ON ft1.x = ft2.x AND ft1.huge ~ 'stuff' AND f2.huge2 ~ 'nonsense') ON
localtab.q = ft1.q;

If we refetch each row individually, we will need a wholerow image of
ft1 and ft2 that includes all columns, or at least f1.huge and
f2.huge2.  If we just fetch a wholerow image of the join output, we
can exclude those.  The only thing we need to recheck is that it's
still the case that localtab.q = ft1.q (because the value of
localtab.q might have changed).


As KaiGai-san mentioned above, what we need to discuss more about with 
Robert's proposition is how to integrate that into the existing EPQ 
machinery.  For example, when, where, and how should we refetch the 
whole-row image of the join output in the case of late row locking?  IMV 
I think that that would need to add a new FDW API different from 
RefetchForeignRow, say RefetchForeignJoinRow.


IMO I think that another benefit from the proposition from KaiGai-san 
(or me) would be that that could provide the whole functionality for row 
locking in remote joins, without an additional development burden on an 
FDW author; the author only has to write GetForeignRowMarkType and 
RefetchForeignRow, which I think is relatively easy.  I think that in 
the proposition, the use of rowmark types such as ROW_MARK_SHARE or 
ROW_MARK_EXCLUSIVE for foreign tables in remote joins would be quite 
inefficient, but I think that the use of ROW_MARK_REFERENCE instead of 
ROW_MARK_COPY would be an option for the workload where EPQ rechecks are 
rarely invoked, because we just need to transfer ctids, not whole-row 
images.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/10/21 13:34, Kouhei Kaigai wrote:

On 2015/10/20 13:11, Etsuro Fujita wrote:

On 2015/10/20 5:34, Robert Haas wrote:

No.  You just need to populate fdw_recheck_quals correctly, same as
for the scan case.



As I said yesterday, that opinion of me is completely wrong.  Sorry for
the incorrectness.  Let me explain a little bit more.  I still think
that even if ROW_MARK_COPY is in use, we would need to locally rejoin
the tuples populated from the whole-row images for the foreign tables
involved in a remote join, using a secondary plan.  Consider eg,

SELECT localtab.*, ft2 from localtab, ft1, ft2
   WHERE ft1.x = ft2.x AND ft1.y = localtab.y FOR UPDATE

In this case, since the output of the foreign join would not include any
ft1 columns, I don't think we could do the same thing as for the scan
case, even if populating fdw_recheck_quals correctly.



As an aside, could you introduce the reason why you think so? It is
significant point in discussion, if we want to reach the consensus.



On the other hands, the joined-tuple we're talking about in this context
is a tuple prior to projection; formed according to the fdw_scan_tlist.
So, it contains all the necessary information to run scan/join qualifiers
towards the joined-tuple. It is not affected by the target-list of user
query.


After research into the planner, I noticed that I was still wrong; IIUC, 
the planner requires that the output of foreign join include the column 
ft1.y even for that case.  (I don't understand the reason why the 
planner requires that.)  So, as Robert mentioned, the clause ft1.y = 
localtab.y could be rechecked during an EPQ recheck, if populating 
fdw_recheck_quals correctly.  Sorry again for the incorrectness.



Even though I think the approach with joined-tuple reconstruction is
reasonable solution here, it is not a fair reason to introduce disadvantage
of Robert's suggestion.


Agreed.


Also, please don't mix up "what we do" and "how we do".

It is "what we do" to discuss which format of tuples shall be returned
to the core backend from the extension, because it determines the role
of interface. If our consensus is to return a joined-tuple, we need to
design the interface according to the consensus.

On the other hands, it is "how we do" discussion whether we should
enforce all the FDW/CSP extension to have alternative plan, or not.
Once we got a consensus in "what we do" discussion, there are variable
options to solve the requirement by the consensus, however, we cannot
prioritize "how we do" without "what we do".


Agreed.

Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Tuesday, October 20, 2015 1:11 PM
> To: Robert Haas
> Cc: Tom Lane; Kaigai Kouhei(海外 浩平); Kyotaro HORIGUCHI;
> pgsql-hackers@postgresql.org; Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On 2015/10/20 5:34, Robert Haas wrote:
> > On Mon, Oct 19, 2015 at 3:45 AM, Etsuro Fujita
> > <fujita.ets...@lab.ntt.co.jp> wrote:
> >> As Tom mentioned, just recomputing the original join tuple is not good
> >> enough.  We would need to rejoin the test tuples for the baserels even if
> >> ROW_MARK_COPY is in use.  Consider:
> >>
> >> A=# BEGIN;
> >> A=# UPDATE t SET a = a + 1 WHERE b = 1;
> >> B=# SELECT * from t, ft1, ft2
> >>   WHERE t.a = ft1.a AND t.b = ft2.b AND ft1.c = ft2.c FOR UPDATE;
> >> A=# COMMIT;
> >>
> >> where the plan for the SELECT FOR UPDATE is
> >>
> >> LockRows
> >> -> Nested Loop
> >> -> Seq Scan on t
> >> -> Foreign Scan on <ft1, ft2>
> >>  Remote SQL: SELECT * FROM ft1 JOIN ft2 WHERE ft1.c = ft2.c AND 
> >> ft1.a
> >> = $1 AND ft2.b = $2
> >>
> >> If an EPQ recheck is invoked by the A's UPDATE, just recomputing the
> >> original join tuple from the whole-row image that you proposed would output
> >> an incorrect result in the EQP recheck since the value a in the updated
> >> version of a to-be-joined tuple in t would no longer match the value ft1.a
> >> extracted from the whole-row image if the A's UPDATE has committed
> >> successfully.  So I think we would need to rejoin the tuples populated from
> >> the whole-row images for the baserels ft1 and ft2, by executing the
> >> secondary plan with the new parameter values for a and b.
> 
> > No.  You just need to populate fdw_recheck_quals correctly, same as
> > for the scan case.
> 
> Yeah, I think we can probably do that for the case where a pushed-down
> join clause is an inner-join one, but I'm not sure that we can do that
> for the case where that clause is an outer-join one.  Maybe I'm missing
> something, though.
>
Please check my message yesterday. The non-nullable side of outer-join is
always visible regardless of the join-clause pushed down, as long as it
satisfies the scan-quals pushed-down.

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


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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On Mon, Oct 19, 2015 at 3:45 AM, Etsuro Fujita
 wrote:

As Tom mentioned, just recomputing the original join tuple is not good
enough.  We would need to rejoin the test tuples for the baserels even if
ROW_MARK_COPY is in use.  Consider:

A=# BEGIN;
A=# UPDATE t SET a = a + 1 WHERE b = 1;
B=# SELECT * from t, ft1, ft2
   WHERE t.a = ft1.a AND t.b = ft2.b AND ft1.c = ft2.c FOR UPDATE;
A=# COMMIT;

where the plan for the SELECT FOR UPDATE is

LockRows
-> Nested Loop
 -> Seq Scan on t
 -> Foreign Scan on 
  Remote SQL: SELECT * FROM ft1 JOIN ft2 WHERE ft1.c = ft2.c AND ft1.a
= $1 AND ft2.b = $2

If an EPQ recheck is invoked by the A's UPDATE, just recomputing the
original join tuple from the whole-row image that you proposed would output
an incorrect result in the EQP recheck since the value a in the updated
version of a to-be-joined tuple in t would no longer match the value ft1.a
extracted from the whole-row image if the A's UPDATE has committed
successfully.  So I think we would need to rejoin the tuples populated from
the whole-row images for the baserels ft1 and ft2, by executing the
secondary plan with the new parameter values for a and b.


Robert Haas wrote:

No.  You just need to populate fdw_recheck_quals correctly, same as
for the scan case.


I wrote:

Yeah, I think we can probably do that for the case where a pushed-down
join clause is an inner-join one, but I'm not sure that we can do that
for the case where that clause is an outer-join one.  Maybe I'm missing
something, though.


On 2015/10/20 15:42, Kouhei Kaigai wrote:

Please check my message yesterday. The non-nullable side of outer-join is
always visible regardless of the join-clause pushed down, as long as it
satisfies the scan-quals pushed-down.


Sorry, my explanation was not correct.  (Needed to take in caffeine.) 
What I'm concerned about is the following:


SELECT * FROM localtab JOIN (ft1 LEFT JOIN ft2 ON ft1.x = ft2.x) ON 
localtab.id = ft1.id FOR UPDATE OF ft1


LockRows
-> Nested Loop
 Join Filter: (localtab.id = ft1.id)
 -> Seq Scan on localtab
 -> Foreign Scan on 
  Remote SQL: SELECT * FROM ft1 LEFT JOIN ft2 WHERE ft1.x = 
ft2.x FOR UPDATE OF ft1


Assume that ft1 performs late row locking.  If an EPQ recheck was 
invoked due to a concurrent transaction on the remote server that 
changed only the value x of the ft1 tuple previously retrieved, then we 
would have to generate a fake ft1/ft2-join tuple with nulls for ft2. 
(Assume that the ft2 tuple previously retrieved was not a null tuple.) 
However, I'm not sure how we can do that in ForeignRecheck; we can't 
know for example, which one is outer and which one is inner, without an 
alternative local join execution plan.  Maybe I'm missing something, though.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/10/20 13:11, Etsuro Fujita wrote:

On 2015/10/20 5:34, Robert Haas wrote:

On Mon, Oct 19, 2015 at 3:45 AM, Etsuro Fujita
 wrote:

As Tom mentioned, just recomputing the original join tuple is not good
enough.  We would need to rejoin the test tuples for the baserels
even if
ROW_MARK_COPY is in use.  Consider:

A=# BEGIN;
A=# UPDATE t SET a = a + 1 WHERE b = 1;
B=# SELECT * from t, ft1, ft2
  WHERE t.a = ft1.a AND t.b = ft2.b AND ft1.c = ft2.c FOR UPDATE;
A=# COMMIT;

where the plan for the SELECT FOR UPDATE is

LockRows
-> Nested Loop
-> Seq Scan on t
-> Foreign Scan on 
 Remote SQL: SELECT * FROM ft1 JOIN ft2 WHERE ft1.c = ft2.c
AND ft1.a
= $1 AND ft2.b = $2

If an EPQ recheck is invoked by the A's UPDATE, just recomputing the
original join tuple from the whole-row image that you proposed would
output
an incorrect result in the EQP recheck since the value a in the updated
version of a to-be-joined tuple in t would no longer match the value
ft1.a
extracted from the whole-row image if the A's UPDATE has committed
successfully.  So I think we would need to rejoin the tuples
populated from
the whole-row images for the baserels ft1 and ft2, by executing the
secondary plan with the new parameter values for a and b.



No.  You just need to populate fdw_recheck_quals correctly, same as
for the scan case.



Yeah, I think we can probably do that for the case where a pushed-down
join clause is an inner-join one, but I'm not sure that we can do that
for the case where that clause is an outer-join one.  Maybe I'm missing
something, though.


As I said yesterday, that opinion of me is completely wrong.  Sorry for 
the incorrectness.  Let me explain a little bit more.  I still think 
that even if ROW_MARK_COPY is in use, we would need to locally rejoin 
the tuples populated from the whole-row images for the foreign tables 
involved in a remote join, using a secondary plan.  Consider eg,


SELECT localtab.*, ft2 from localtab, ft1, ft2
 WHERE ft1.x = ft2.x AND ft1.y = localtab.y FOR UPDATE

In this case, since the output of the foreign join would not include any 
ft1 columns, I don't think we could do the same thing as for the scan 
case, even if populating fdw_recheck_quals correctly.  And I think we 
would need to rejoin the tuples, using a local join execution plan, 
which would have the parameterization for the to-be-pushed-down clause 
ft1.y = localtab.y.  I'm still missing something, though.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Wednesday, October 21, 2015 12:31 PM
> To: Robert Haas
> Cc: Tom Lane; Kaigai Kouhei(海外 浩平); Kyotaro HORIGUCHI;
> pgsql-hackers@postgresql.org; Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On 2015/10/20 13:11, Etsuro Fujita wrote:
> > On 2015/10/20 5:34, Robert Haas wrote:
> >> On Mon, Oct 19, 2015 at 3:45 AM, Etsuro Fujita
> >> <fujita.ets...@lab.ntt.co.jp> wrote:
> >>> As Tom mentioned, just recomputing the original join tuple is not good
> >>> enough.  We would need to rejoin the test tuples for the baserels
> >>> even if
> >>> ROW_MARK_COPY is in use.  Consider:
> >>>
> >>> A=# BEGIN;
> >>> A=# UPDATE t SET a = a + 1 WHERE b = 1;
> >>> B=# SELECT * from t, ft1, ft2
> >>>   WHERE t.a = ft1.a AND t.b = ft2.b AND ft1.c = ft2.c FOR UPDATE;
> >>> A=# COMMIT;
> >>>
> >>> where the plan for the SELECT FOR UPDATE is
> >>>
> >>> LockRows
> >>> -> Nested Loop
> >>> -> Seq Scan on t
> >>> -> Foreign Scan on <ft1, ft2>
> >>>  Remote SQL: SELECT * FROM ft1 JOIN ft2 WHERE ft1.c = ft2.c
> >>> AND ft1.a
> >>> = $1 AND ft2.b = $2
> >>>
> >>> If an EPQ recheck is invoked by the A's UPDATE, just recomputing the
> >>> original join tuple from the whole-row image that you proposed would
> >>> output
> >>> an incorrect result in the EQP recheck since the value a in the updated
> >>> version of a to-be-joined tuple in t would no longer match the value
> >>> ft1.a
> >>> extracted from the whole-row image if the A's UPDATE has committed
> >>> successfully.  So I think we would need to rejoin the tuples
> >>> populated from
> >>> the whole-row images for the baserels ft1 and ft2, by executing the
> >>> secondary plan with the new parameter values for a and b.
> 
> >> No.  You just need to populate fdw_recheck_quals correctly, same as
> >> for the scan case.
> 
> > Yeah, I think we can probably do that for the case where a pushed-down
> > join clause is an inner-join one, but I'm not sure that we can do that
> > for the case where that clause is an outer-join one.  Maybe I'm missing
> > something, though.
> 
> As I said yesterday, that opinion of me is completely wrong.  Sorry for
> the incorrectness.  Let me explain a little bit more.  I still think
> that even if ROW_MARK_COPY is in use, we would need to locally rejoin
> the tuples populated from the whole-row images for the foreign tables
> involved in a remote join, using a secondary plan.  Consider eg,
> 
> SELECT localtab.*, ft2 from localtab, ft1, ft2
>   WHERE ft1.x = ft2.x AND ft1.y = localtab.y FOR UPDATE
> 
> In this case, since the output of the foreign join would not include any
> ft1 columns, I don't think we could do the same thing as for the scan
> case, even if populating fdw_recheck_quals correctly.
>
As an aside, could you introduce the reason why you think so? It is
significant point in discussion, if we want to reach the consensus.

It looks to me the above introduction mix up the target-list of user
query and the target-list of remote query.
If EPQ mechanism requires joined tuple on ft1 and ft2, FDW driver can
make a remote query as follows:
  SELECT ft2, ft1.y, ft1.x, ft2.x FROM ft1.x = ft2.x FOR UPDATE
Thus, fdw_scan_tlist has four target-entries, but later two items are
resjunk=true because ForeignScan node drops these columns by projection
when it returns a tuple to upper node.
On the other hands, the joined-tuple we're talking about in this context
is a tuple prior to projection; formed according to the fdw_scan_tlist.
So, it contains all the necessary information to run scan/join qualifiers
towards the joined-tuple. It is not affected by the target-list of user
query.

Even though I think the approach with joined-tuple reconstruction is
reasonable solution here, it is not a fair reason to introduce disadvantage
of Robert's suggestion.

> And I think we
> would need to rejoin the tuples, using a local join execution plan,
> which would have the parameterization for the to-be-pushed-down clause
> ft1.y = localtab.y.  I'm still missing something, though.
>
Also, please don't mix up "what we do" and "how we do".

It is "what we do" to discuss which format of tuples shall be returned
to the core backend from the extension, because it determines the role
of interface. If our consensus is to return a joined-tuple, we 

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/10/17 9:58, Robert Haas wrote:

But with Etsuro Fujita's patch, and I think what you have proposed has
been similar, how are you going to do it?  The proposal is to call the
recheck method and hope for the best, but what is the recheck method
going to do?  Where is it going to get the previously-returned tuple?


As I explained in a previous email, just returning the 
previously-returned tuple is not good enough.



How will it know if it has already returned it during the lifetime of
this EPQ check?  Offhand, it looks to me like, at least in some
circumstances, you're probably going to return whatever tuple you
returned most recently (which has a good chance of being the right
one, but not necessarily) over and over again.  That's not going to
fly.


No.  Since the local join execution plan is created so that the scan 
slot for each foreign table involved in the pushed-down join looks at 
its EPQ slot, I think the plan can return at most one tuple.


Best regards,
Etsuro Fujita



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