Re: Foreign join pushdown vs EvalPlanQual

Started by KaiGai Koheiover 10 years ago83 messageshackers
Jump to latest
#1KaiGai Kohei
kaigai@ak.jp.nec.com

On Thu, Oct 29, 2015 at 6:05 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> 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 <kaigai@ak.jp.nec.com>

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

#2Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#1)

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

#3KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#2)

-----Original Message-----
From: Etsuro Fujita [mailto:fujita.etsuro@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 <kaigai@ak.jp.nec.com>

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

#4Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#3)

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

#5Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#1)

On Tue, Nov 3, 2015 at 8:15 AM, Kouhei Kaigai <kaigai@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:

/messages/by-id/9A28C8860F777E439AA12E8AEA7694F80114B89D@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

#6KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#5)

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@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 <kaigai@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 :-)

/messages/by-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 <kaigai@ak.jp.nec.com>

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

#7Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#6)

On Fri, Nov 6, 2015 at 9:42 AM, Kouhei Kaigai <kaigai@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.

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

#8KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#7)

On Fri, Nov 6, 2015 at 9:42 AM, Kouhei Kaigai <kaigai@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.

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

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

#9KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: KaiGai Kohei (#8)

-----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 <kaigai@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.

----------------<cut 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;
+	}
 	return ExecQual(node->fdw_recheck_quals, econtext, false);
 }
----------------<cut here>----------------

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 <kaigai@ak.jp.nec.com>

Attachments:

pgsql-fdw-epq-recheck.v3.patchapplication/octet-stream; name=pgsql-fdw-epq-recheck.v3.patchDownload+148-7
#10Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#9)

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]/messages/by-id/5624D583.10202@lab.ntt.co.jp, 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!

----------------<cut 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;
+	}
return ExecQual(node->fdw_recheck_quals, econtext, false);
}
----------------<cut here>----------------

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]/messages/by-id/5624D583.10202@lab.ntt.co.jp. Maybe I'm missing something, though.

Best regards,
Etsuro Fujita

[1]: /messages/by-id/5624D583.10202@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

#11KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#10)
----------------<cut 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;
+	}
return ExecQual(node->fdw_recheck_quals, econtext, false);
}
----------------<cut here>----------------

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 <kaigai@ak.jp.nec.com>

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

#12Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#11)

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]/messages/by-id/CA+TgmoZdPU_fcSpOzXxpD1xvyq3cZCAwD7-x3aVWbKgSFoHvRA@mail.gmail.com. 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]: /messages/by-id/CA+TgmoZdPU_fcSpOzXxpD1xvyq3cZCAwD7-x3aVWbKgSFoHvRA@mail.gmail.com
/messages/by-id/CA+TgmoZdPU_fcSpOzXxpD1xvyq3cZCAwD7-x3aVWbKgSFoHvRA@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

#13KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#12)

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:
/messages/by-id/9A28C8860F777E439AA12E8AEA7694F8011617C6@BPXM15GP.gisp.nec.co.jp

And, why Robert thought here is a tough challenge:
/messages/by-id/CA+TgmoY5Lf+vYy1Bha=U7__S3qtMQP7d+gSSfd+LN4Xz6Fybkg@mail.gmail.com

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

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

#14Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#10)

On Sun, Nov 8, 2015 at 11:13 PM, Etsuro Fujita
<fujita.etsuro@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. 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

#15Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#14)

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

On Sun, Nov 8, 2015 at 11:13 PM, Etsuro Fujita
<fujita.etsuro@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. 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

#16KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#15)

-----Original Message-----
From: Etsuro Fujita [mailto:fujita.etsuro@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.etsuro@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 <kaigai@ak.jp.nec.com>

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

#17Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Etsuro Fujita (#15)

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

#18Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#16)

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.etsuro@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?

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

#19Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Kyotaro Horiguchi (#17)

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

#20KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#18)

-----Original Message-----
From: Etsuro Fujita [mailto:fujita.etsuro@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.etsuro@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.

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.

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

I can agree with we ought to provide a utility routine to construct
a local alternative subplan, however, we are in beta2 stage for v9.5.
So, I'd like to suggest only callback on v9.5 (FDW driver can handle
its subplan by itself, no need to path the backend), then design the
utility routine for this.

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

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

#21Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: KaiGai Kohei (#20)
#22Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#20)
#23Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Kyotaro Horiguchi (#21)
#24KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#23)
#25Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#15)
#26Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#9)
#27KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#26)
#28KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: KaiGai Kohei (#27)
#29Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#25)
#30Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#27)
#31KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#30)
#32Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#28)
#33Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#29)
#34Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#31)
#35Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#33)
#36KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#35)
#37Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#36)
#38Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#35)
#39Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#38)
#40KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#37)
#41Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#32)
#42KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#41)
#43Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#40)
#44KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#43)
#45Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#43)
#46Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#42)
#47Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#9)
#48KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#45)
#49Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: KaiGai Kohei (#48)
#50Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#48)
#51KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Kyotaro Horiguchi (#49)
#52KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#50)
#53Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#51)
#54KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#53)
#55Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#52)
#56Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#50)
#57Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#55)
#58Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#54)
#59Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#57)
#60Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#48)
#61Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Robert Haas (#60)
#62Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: KaiGai Kohei (#48)
#63Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#56)
#64KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#63)
#65KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Etsuro Fujita (#63)
#66Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#57)
#67Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#65)
#68Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#58)
#69Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#63)
#70Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#69)
#71Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#70)
#72Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#71)
#73Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Tom Lane (#72)
#74Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#73)
#75Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#74)
#76Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#75)
#77KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#76)
#78Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: KaiGai Kohei (#77)
#79Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#78)
#80Michael Paquier
michael@paquier.xyz
In reply to: Robert Haas (#79)
#81Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Michael Paquier (#80)
#82Michael Paquier
michael@paquier.xyz
In reply to: Amit Langote (#81)
#83Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#82)