Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API)
On Tue, Nov 25, 2014 at 3:44 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
Today, I had a talk with Hanada-san to clarify which can be a common
portion of them and how to implement it. Then, we concluded both of
features can be shared most of the infrastructure.
Let me put an introduction of join replacement by foreign-/custom-scanbelow.
Its overall design intends to inject foreign-/custom-scan node instead
of the built-in join logic (based on the estimated cost). From the
viewpoint of core backend, it looks like a sub-query scan that
contains relations join internally.What we need to do is below:
(1) Add a hook add_paths_to_joinrel()
It gives extensions (including FDW drivers and custom-scan providers)
chance to add alternative paths towards a particular join of
relations, using ForeignScanPath or CustomScanPath, if it can run insteadof the built-in ones.
(2) Informs the core backend varno/varattno mapping One thing we need
to pay attention is, foreign-/custom-scan node that performs instead
of the built-in join node must return mixture of values come from both
relations. In case when FDW driver fetch a remote record (also, fetch
a record computed by external computing resource), the most reasonable
way is to store it on ecxt_scantuple of ExprContext, then kicks
projection with varnode that references this slot.
It needs an infrastructure that tracks relationship between original
varnode and the alternative varno/varattno. We thought, it shall be
mapped to INDEX_VAR and a virtual attribute number to reference
ecxt_scantuple naturally, and this infrastructure is quite helpful forboth of ForegnScan/CustomScan.
We'd like to add List *fdw_varmap/*custom_varmap variable to both of plan
nodes.
It contains list of the original Var node that shall be mapped on the
position according to the list index. (e.g, the first varnode is
varno=INDEX_VAR and
varattno=1)(3) Reverse mapping on EXPLAIN
For EXPLAIN support, above varnode on the pseudo relation scan needed
to be solved. All we need to do is initialization of dpns->inner_tlist
on
set_deparse_planstate() according to the above mapping.(4) case of scanrelid == 0
To skip open/close (foreign) tables, we need to have a mark to
introduce the backend not to initialize the scan node according to
table definition, but according to the pseudo varnodes list.
As earlier custom-scan patch doing, scanrelid == 0 is a
straightforward mark to show the scan node is not combined with aparticular real relation.
So, it also need to add special case handling around foreign-/custom-scan
code.
We expect above changes are enough small to implement basic join
push-down functionality (that does not involves external computing of
complicated expression node), but valuable to support in v9.5.Please comment on the proposition above.
I don't really have any technical comments on this design right at the moment,
but I think it's an important area where PostgreSQL needs to make some
progress sooner rather than later, so I hope that we can get something
committed in time for 9.5.
I tried to implement the interface portion, as attached.
Hanada-san may be under development of postgres_fdw based on this interface
definition towards the next commit fest.
Overall design of this patch is identical with what I described above.
It intends to allow extensions (FDW driver or custom-scan provider) to
replace a join by a foreign/custom-scan which internally contains a result
set of relations join externally computed. It looks like a relation scan
on the pseudo relation.
One we need to pay attention is, how setrefs.c fixes up varno/varattno
unlike regular join structure. I could find IndexOnlyScan already has
similar infrastructure that redirect references of varnode to a certain
column on ecxt_scantuple of ExprContext using a pair of INDEX_VAR and
alternative varattno.
This patch put a new field: fdw_ps_tlist of ForeignScan, and
custom_ps_tlist of CustomScan. It is extension's role to set a pseudo-
scan target-list (so, ps_tlist) of the foreign/custom-scan that replaced
a join.
If it is not NIL, set_plan_refs() takes another strategy to fix up them.
It calls fix_upper_expr() to map varnodes of expression-list on INDEX_VAR
according to the ps_tlist, then extension is expected to put values/isnull
pair on ss_ScanTupleSlot of scan-state according to the ps_tlist preliminary
constructed.
Regarding to the primary hook to add alternative foreign/custom-scan
path instead of built-in join paths, I added the following hook on
add_paths_to_joinrel().
/* Hook for plugins to get control in add_paths_to_joinrel() */
typedef void (*set_join_pathlist_hook_type) (PlannerInfo *root,
RelOptInfo *joinrel,
RelOptInfo *outerrel,
RelOptInfo *innerrel,
List *restrictlist,
JoinType jointype,
SpecialJoinInfo *sjinfo,
SemiAntiJoinFactors *semifactors,
Relids param_source_rels,
Relids extra_lateral_rels);
extern PGDLLIMPORT set_join_pathlist_hook_type set_join_pathlist_hook;
It shall give enough information for extensions to determine whether
it can offer alternative paths, or not.
One thing I concerned about is, fdw_handler to be called on joinrel is
not obvious, unlike custom-scan that hold reference to CustomScanMethods,
because joinrel is not managed by any FDW drivers.
So, I had to add "Oid fdw_handler" field onto RelOptInfo to track which
foreign-tables are involved in this relation join. This field shall have
oid of valid FDW handler if both inner/outer relation is managed by
same FDW handler. Elsewhere, InvalidOid. Even if either/both of them are
relations-join, fdw_handler shall be set as long as it is managed by
same FDW handler. It allows to replace join by foreign-scan that involves
more than two tables.
One new interface contract is case of scanrelid == 0. If foreign-/custom-
scan is not associated with a particular relation, ExecInitXXX() tries to
initialize ss_ScanTupleSlot according to the ps_tlist, and relations is
not opened.
Because the working example is still under development, this patch is
not tested/validated yet. However, it briefly implements the concept of
what we'd like to enhance foreign-/custom-scan functionality.
Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
pgsql-v9.5-custom-join.v1.patchapplication/octet-stream; name=pgsql-v9.5-custom-join.v1.patchDownload+240-44
Hello,
The attached patch is newer revision of custom-/foreign-join
interface.
I've ported my PG-Strom extension to fit the latest custom-scan
(+this patch) interface in this winter vacation.
The concept of "join replaced by foreign-/custom-scan" almost
works well, however, here are two small oversight on the v1
interface.
1. EXPLAIN didn't work when scanrelid==0.
ExplainNode() always called ExplainScanTarget() to T_ForeignScan
or T_CustomScan, however, foreign-/custom-scan node that replaced
join relation does not have a particular base relation.
So, I put a check to skip this call when scanrelid==0.
2. create_plan_recurse() needs to be available from extension.
In case when CustomScan node takes underlying plan nodes, its
PlanCustomPath() method is also responsible to invoke the plan
creation routine of the underlying path-node. However, existing
code declared create_plan_recurse() as static function.
So, this patch re-declared it as external function.
Also, one other point I'd like to have in this interface.
In case when foreign-/custom-scan node has pseudo-scan
targetlist, it may contain the target-entries which are not
actually in use, but need to be here to lookup column name on
EXPLAIN command.
I'd like to add a flag to indicate the core backend to ignore
target-entries in the pseudo-scan tlist if resjunk=true, when
it initialized the foreign-/custom-scan-state node, and setting
up scan type descriptor.
It will reduce unnecessary projection, if foreign-/custom-scan
node can produce a tuple based on the expectation of tlist.
I'd like to see the comment around this point.
Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>
Show quoted text
-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Kouhei Kaigai
Sent: Wednesday, December 03, 2014 3:11 PM
To: Robert Haas
Cc: Tom Lane; pgsql-hackers@postgreSQL.org; Shigeru Hanada
Subject: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)On Tue, Nov 25, 2014 at 3:44 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com>
wrote:
Today, I had a talk with Hanada-san to clarify which can be a common
portion of them and how to implement it. Then, we concluded both of
features can be shared most of the infrastructure.
Let me put an introduction of join replacement by
foreign-/custom-scanbelow.
Its overall design intends to inject foreign-/custom-scan node
instead of the built-in join logic (based on the estimated cost).
From the viewpoint of core backend, it looks like a sub-query scan
that contains relations join internally.What we need to do is below:
(1) Add a hook add_paths_to_joinrel() It gives extensions (including
FDW drivers and custom-scan providers) chance to add alternative
paths towards a particular join of relations, using ForeignScanPath
or CustomScanPath, if it can run insteadof the built-in ones.
(2) Informs the core backend varno/varattno mapping One thing we
need to pay attention is, foreign-/custom-scan node that performs
instead of the built-in join node must return mixture of values come
from both relations. In case when FDW driver fetch a remote record
(also, fetch a record computed by external computing resource), the
most reasonable way is to store it on ecxt_scantuple of ExprContext,
then kicks projection with varnode that references this slot.
It needs an infrastructure that tracks relationship between original
varnode and the alternative varno/varattno. We thought, it shall be
mapped to INDEX_VAR and a virtual attribute number to reference
ecxt_scantuple naturally, and this infrastructure is quite helpful
forboth of ForegnScan/CustomScan.
We'd like to add List *fdw_varmap/*custom_varmap variable to both of
plannodes.
It contains list of the original Var node that shall be mapped on
the position according to the list index. (e.g, the first varnode is
varno=INDEX_VAR and
varattno=1)(3) Reverse mapping on EXPLAIN
For EXPLAIN support, above varnode on the pseudo relation scan
needed to be solved. All we need to do is initialization of
dpns->inner_tlist on
set_deparse_planstate() according to the above mapping.(4) case of scanrelid == 0
To skip open/close (foreign) tables, we need to have a mark to
introduce the backend not to initialize the scan node according to
table definition, but according to the pseudo varnodes list.
As earlier custom-scan patch doing, scanrelid == 0 is a
straightforward mark to show the scan node is not combined with aparticular real relation.
So, it also need to add special case handling around
foreign-/custom-scancode.
We expect above changes are enough small to implement basic join
push-down functionality (that does not involves external computing
of complicated expression node), but valuable to support in v9.5.Please comment on the proposition above.
I don't really have any technical comments on this design right at the
moment, but I think it's an important area where PostgreSQL needs to
make some progress sooner rather than later, so I hope that we can get
something committed in time for 9.5.I tried to implement the interface portion, as attached.
Hanada-san may be under development of postgres_fdw based on this interface
definition towards the next commit fest.Overall design of this patch is identical with what I described above.
It intends to allow extensions (FDW driver or custom-scan provider) to
replace a join by a foreign/custom-scan which internally contains a result
set of relations join externally computed. It looks like a relation scan
on the pseudo relation.One we need to pay attention is, how setrefs.c fixes up varno/varattno unlike
regular join structure. I could find IndexOnlyScan already has similar
infrastructure that redirect references of varnode to a certain column on
ecxt_scantuple of ExprContext using a pair of INDEX_VAR and alternative
varattno.This patch put a new field: fdw_ps_tlist of ForeignScan, and custom_ps_tlist
of CustomScan. It is extension's role to set a pseudo- scan target-list
(so, ps_tlist) of the foreign/custom-scan that replaced a join.
If it is not NIL, set_plan_refs() takes another strategy to fix up them.
It calls fix_upper_expr() to map varnodes of expression-list on INDEX_VAR
according to the ps_tlist, then extension is expected to put values/isnull
pair on ss_ScanTupleSlot of scan-state according to the ps_tlist
preliminary constructed.Regarding to the primary hook to add alternative foreign/custom-scan path
instead of built-in join paths, I added the following hook on
add_paths_to_joinrel()./* Hook for plugins to get control in add_paths_to_joinrel() */
typedef void (*set_join_pathlist_hook_type) (PlannerInfo *root,
RelOptInfo *joinrel,
RelOptInfo *outerrel,
RelOptInfo *innerrel,
List *restrictlist,
JoinType jointype,
SpecialJoinInfo *sjinfo,
SemiAntiJoinFactors
*semifactors,
Relids
param_source_rels,
Relids
extra_lateral_rels);
extern PGDLLIMPORT set_join_pathlist_hook_type set_join_pathlist_hook;It shall give enough information for extensions to determine whether it
can offer alternative paths, or not.One thing I concerned about is, fdw_handler to be called on joinrel is not
obvious, unlike custom-scan that hold reference to CustomScanMethods,
because joinrel is not managed by any FDW drivers.
So, I had to add "Oid fdw_handler" field onto RelOptInfo to track which
foreign-tables are involved in this relation join. This field shall have
oid of valid FDW handler if both inner/outer relation is managed by same
FDW handler. Elsewhere, InvalidOid. Even if either/both of them are
relations-join, fdw_handler shall be set as long as it is managed by same
FDW handler. It allows to replace join by foreign-scan that involves more
than two tables.One new interface contract is case of scanrelid == 0. If foreign-/custom-
scan is not associated with a particular relation, ExecInitXXX() tries to
initialize ss_ScanTupleSlot according to the ps_tlist, and relations is
not opened.Because the working example is still under development, this patch is not
tested/validated yet. However, it briefly implements the concept of what
we'd like to enhance foreign-/custom-scan functionality.Thanks,
--
NEC OSS Promotion Center / PG-Strom Project KaiGai Kohei
<kaigai@ak.jp.nec.com>
Attachments:
pgsql-v9.5-custom-join.v2.patchapplication/octet-stream; name=pgsql-v9.5-custom-join.v2.patchDownload+244-47
On 1/6/15, 8:17 AM, Kouhei Kaigai wrote:
The attached patch is newer revision of custom-/foreign-join
interface.
Shouldn't instances of
scan_relid > 0
be
scan_relid != InvalidOid
?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/01/15 00:05, Jim Nasby wrote:
On 1/6/15, 8:17 AM, Kouhei Kaigai wrote:
The attached patch is newer revision of custom-/foreign-join
interface.Shouldn't instances of
scan_relid > 0
be
scan_relid != InvalidOid
Ideally, they should be OidIsValid(scan_relid)
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
scan_relid != InvalidOid
Ideally, they should be OidIsValid(scan_relid)
Scan.scanrelid is an index of range-tables list, not an object-id.
So, InvalidOid or OidIsValid() are not a good choice.
The bare relation oid has to be saved on relid of RangeTblEntry
which can be pulled using rt_fetch(scanrelid, range_tables).
I could found an assertion below at ExecScanFetch().
Assert(scanrelid > 0);
Probably, it is a usual manner for this.
Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>
-----Original Message-----
From: Petr Jelinek [mailto:petr@2ndquadrant.com]
Sent: Wednesday, January 07, 2015 8:24 AM
To: Jim Nasby; Kaigai Kouhei(海外 浩平); Robert Haas
Cc: Tom Lane; pgsql-hackers@postgreSQL.org; Shigeru Hanada
Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan
API)On 07/01/15 00:05, Jim Nasby wrote:
On 1/6/15, 8:17 AM, Kouhei Kaigai wrote:
The attached patch is newer revision of custom-/foreign-join
interface.Shouldn't instances of
scan_relid > 0
be
scan_relid != InvalidOid
Ideally, they should be OidIsValid(scan_relid)
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jan 6, 2015 at 9:17 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
The attached patch is newer revision of custom-/foreign-join
interface.
It seems that the basic purpose of this patch is to allow a foreign
scan or custom scan to have scanrelid == 0, reflecting the case where
we are scanning a joinrel rather than a baserel. The major problem
that seems to create is that we can't set the target list from the
relation descriptor, because there isn't one. To work around that,
you've added fdw_ps_list and custom_ps_tlist, which the FDW or
custom-plan provider must set. I don't know off-hand whether that's a
good interface or not. How does the FDW know what to stick in there?
There's a comment that seems to be trying to explain this:
+ * An optional fdw_ps_tlist is used to map a reference to an attribute of
+ * underlying relation(s) on a pair of INDEX_VAR and alternative varattno.
+ * It looks like a scan on pseudo relation that is usually result of
+ * relations join on remote data source, and FDW driver is responsible to
+ * set expected target list for this. If FDW returns records as foreign-
+ * table definition, just put NIL here....but I can't understand what that's telling me.
You've added an "Oid fdw_handler" field to the ForeignScan and
RelOptInfo structures. I think this is the OID of the pg_proc entry
for the handler function; and I think we need it because, if scanrelid
== 0 then we don't have a relation that we can trace to a foreign
table, to a server, to an FDW, and then to a handler. So we need to
get that information some other way. When building joinrels, the
fdw_handler OID, and the associated routine, are propagated from any
two relations that share the same fdw_handler OID to the resulting
joinrel. I guess that's reasonable, although it feels a little weird
that we're copying around both the OID and the structure-pointer.
For non-obvious reasons, you've made create_plan_recurse() non-static.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jan 6, 2015 at 9:17 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
The attached patch is newer revision of custom-/foreign-join
interface.It seems that the basic purpose of this patch is to allow a foreign scan
or custom scan to have scanrelid == 0, reflecting the case where we are
scanning a joinrel rather than a baserel. The major problem that seems
to create is that we can't set the target list from the relation descriptor,
because there isn't one. To work around that, you've added fdw_ps_list
and custom_ps_tlist, which the FDW or custom-plan provider must set. I
don't know off-hand whether that's a good interface or not. How does the
FDW know what to stick in there?
In the most usual scenario, FDP/CSP will make a ps_tlist according to the
target-list of the joinrel (that contains mixture of var-nodes to left-side
and right-side), and qualifier's expression tree if any.
As long as FDW can construct a remote query, it knows which attributes shall
be returned and which relation does it come from. It is equivalent to what
ps_tlist tries to inform the core optimizer.
There's a comment that seems to be trying to explain this:
+ * An optional fdw_ps_tlist is used to map a reference to an attribute + of + * underlying relation(s) on a pair of INDEX_VAR and alternative varattno. + * It looks like a scan on pseudo relation that is usually result of + * relations join on remote data source, and FDW driver is responsible + to + * set expected target list for this. If FDW returns records as + foreign- + * table definition, just put NIL here....but I can't understand what that's telling me.
Sorry, let me explain in another expression.
A joinrel has a target-list that can/may contain references to both of
the left and right relations. These are eventually mapped to either
INNER_VAR or OUTER_VAR, then executor switch the TupleTableSlot
(whether ecxt_innertuple or ecxt_outertuple) according to the special
varno.
On the other hands, because ForeignScan/CustomScan is a scan plan, it
shall have just one TupleTableSlot on execution time. Thus, we need a
mechanism that maps attributes from both of the relations on a certain
location of the slot; that shall be eventually translated to var-node
with INDEX_VAR to reference ecxt_scantuple.
Of course, ps_tlist is not necessary if ForeignScan/CustomScan scans
on a base relation as literal. In this case, the interface contract
expects NIL is set on the ps_tlist field.
You've added an "Oid fdw_handler" field to the ForeignScan and RelOptInfo
structures. I think this is the OID of the pg_proc entry for the handler
function; and I think we need it because, if scanrelid == 0 then we don't
have a relation that we can trace to a foreign table, to a server, to an
FDW, and then to a handler. So we need to get that information some other
way. When building joinrels, the fdw_handler OID, and the associated
routine, are propagated from any two relations that share the same
fdw_handler OID to the resulting joinrel. I guess that's reasonable,
although it feels a little weird that we're copying around both the OID
and the structure-pointer.
Unlike CustomScan node, ForeignScan node does not have function pointers.
In addition, it is dynamically allocated by palloc(), so we have no
guarantee the pointer constructed on plan-stage is valid on beginning
of the executor.
It is the reason why I put OID of the FDW handler routine.
Any other better idea?
For non-obvious reasons, you've made create_plan_recurse() non-static.
When custom-scan node replaced a join-plan, it shall have at least two
child plan-nodes. The callback handler of PlanCustomPath needs to be
able to call create_plan_recurse() to transform the underlying path-nodes
to plan-nodes, because this custom-scan node may take other built-in
scan or sub-join nodes as its inner/outer input.
In case of FDW, it shall kick any underlying scan relations to remote
side, thus we may not expect ForeignScan has underlying plans...
Thanks,
--
NEC OSS Promotion Center / 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
Import Notes
Resolved by subject fallback
On 1/6/15, 5:43 PM, Kouhei Kaigai wrote:
scan_relid != InvalidOid
Ideally, they should be OidIsValid(scan_relid)
Scan.scanrelid is an index of range-tables list, not an object-id.
So, InvalidOid or OidIsValid() are not a good choice.
I think the name needs to change then; scan_relid certainly looks like the OID of a relation.
scan_index?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2015-01-10 8:18 GMT+09:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/6/15, 5:43 PM, Kouhei Kaigai wrote:
scan_relid != InvalidOid
Ideally, they should be OidIsValid(scan_relid)
Scan.scanrelid is an index of range-tables list, not an object-id.
So, InvalidOid or OidIsValid() are not a good choice.I think the name needs to change then; scan_relid certainly looks like the
OID of a relation.scan_index?
Yep, I had a same impression when I looked at the code first time,
however, it is defined as below. Not a manner of custom-scan itself.
/*
* ==========
* Scan nodes
* ==========
*/
typedef struct Scan
{
Plan plan;
Index scanrelid; /* relid is index into the range table */
} Scan;
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/01/15 01:19, Kohei KaiGai wrote:
2015-01-10 8:18 GMT+09:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/6/15, 5:43 PM, Kouhei Kaigai wrote:
scan_relid != InvalidOid
Ideally, they should be OidIsValid(scan_relid)
Scan.scanrelid is an index of range-tables list, not an object-id.
So, InvalidOid or OidIsValid() are not a good choice.I think the name needs to change then; scan_relid certainly looks like the
OID of a relation.scan_index?
Yep, I had a same impression when I looked at the code first time,
however, it is defined as below. Not a manner of custom-scan itself./*
* ==========
* Scan nodes
* ==========
*/
typedef struct Scan
{
Plan plan;
Index scanrelid; /* relid is index into the range table */
} Scan;
Yeah there are actually several places in the code where "relid" means
index in range table and not oid of relation, it still manages to
confuse me. Nothing this patch can do about that.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/9/15, 6:44 PM, Petr Jelinek wrote:
Yep, I had a same impression when I looked at the code first time,
however, it is defined as below. Not a manner of custom-scan itself./*
* ==========
* Scan nodes
* ==========
*/
typedef struct Scan
{
Plan plan;
Index scanrelid; /* relid is index into the range table */
} Scan;Yeah there are actually several places in the code where "relid" means index in range table and not oid of relation, it still manages to confuse me. Nothing this patch can do about that.
Well, since it's confused 3 of us now... should we change it (as a separate patch)? I'm willing to do that work but don't want to waste time if it'll just be rejected.
Any other examples of this I should fix too?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/9/15, 6:54 PM, Jim Nasby wrote:
On 1/9/15, 6:44 PM, Petr Jelinek wrote:
Yep, I had a same impression when I looked at the code first time,
however, it is defined as below. Not a manner of custom-scan itself./*
* ==========
* Scan nodes
* ==========
*/
typedef struct Scan
{
Plan plan;
Index scanrelid; /* relid is index into the range table */
} Scan;Yeah there are actually several places in the code where "relid" means index in range table and not oid of relation, it still manages to confuse me. Nothing this patch can do about that.
Well, since it's confused 3 of us now... should we change it (as a separate patch)? I'm willing to do that work but don't want to waste time if it'll just be rejected.
Any other examples of this I should fix too?
Sorry, to clarify... any other items besides Scan.scanrelid that I should fix?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2015-01-10 9:56 GMT+09:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/9/15, 6:54 PM, Jim Nasby wrote:
On 1/9/15, 6:44 PM, Petr Jelinek wrote:
Yep, I had a same impression when I looked at the code first time,
however, it is defined as below. Not a manner of custom-scan itself./*
* ==========
* Scan nodes
* ==========
*/
typedef struct Scan
{
Plan plan;
Index scanrelid; /* relid is index into the range table
*/
} Scan;Yeah there are actually several places in the code where "relid" means
index in range table and not oid of relation, it still manages to confuse
me. Nothing this patch can do about that.Well, since it's confused 3 of us now... should we change it (as a
separate patch)? I'm willing to do that work but don't want to waste time if
it'll just be rejected.Any other examples of this I should fix too?
Sorry, to clarify... any other items besides Scan.scanrelid that I should
fix?
This naming is a little bit confusing, however, I don't think it "should" be
changed because this structure has been used for a long time, so reworking
will prevent back-patching when we find bugs around "scanrelid".
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/9/15, 8:51 PM, Kohei KaiGai wrote:
2015-01-10 9:56 GMT+09:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/9/15, 6:54 PM, Jim Nasby wrote:
On 1/9/15, 6:44 PM, Petr Jelinek wrote:
Yep, I had a same impression when I looked at the code first time,
however, it is defined as below. Not a manner of custom-scan itself./*
* ==========
* Scan nodes
* ==========
*/
typedef struct Scan
{
Plan plan;
Index scanrelid; /* relid is index into the range table
*/
} Scan;Yeah there are actually several places in the code where "relid" means
index in range table and not oid of relation, it still manages to confuse
me. Nothing this patch can do about that.Well, since it's confused 3 of us now... should we change it (as a
separate patch)? I'm willing to do that work but don't want to waste time if
it'll just be rejected.Any other examples of this I should fix too?
Sorry, to clarify... any other items besides Scan.scanrelid that I should
fix?This naming is a little bit confusing, however, I don't think it "should" be
changed because this structure has been used for a long time, so reworking
will prevent back-patching when we find bugs around "scanrelid".
We can still backpatch; it just requires more work. And how many bugs do we actually expect to find around this anyway?
If folks think this just isn't worth fixing fine, but I find the backpatching argument rather dubious.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2015-01-11 10:40 GMT+09:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/9/15, 8:51 PM, Kohei KaiGai wrote:
2015-01-10 9:56 GMT+09:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/9/15, 6:54 PM, Jim Nasby wrote:
On 1/9/15, 6:44 PM, Petr Jelinek wrote:
Yep, I had a same impression when I looked at the code first time,
however, it is defined as below. Not a manner of custom-scan itself./*
* ==========
* Scan nodes
* ==========
*/
typedef struct Scan
{
Plan plan;
Index scanrelid; /* relid is index into the range
table
*/
} Scan;Yeah there are actually several places in the code where "relid" means
index in range table and not oid of relation, it still manages to
confuse
me. Nothing this patch can do about that.Well, since it's confused 3 of us now... should we change it (as a
separate patch)? I'm willing to do that work but don't want to waste
time if
it'll just be rejected.Any other examples of this I should fix too?
Sorry, to clarify... any other items besides Scan.scanrelid that I should
fix?This naming is a little bit confusing, however, I don't think it "should"
be
changed because this structure has been used for a long time, so reworking
will prevent back-patching when we find bugs around "scanrelid".We can still backpatch; it just requires more work. And how many bugs do we
actually expect to find around this anyway?If folks think this just isn't worth fixing fine, but I find the
backpatching argument rather dubious.
Even though here is no problem around Scan structure itself, a bugfix may
use the variable name of "scanrelid" to fix it. If we renamed it on v9.5, we
also need a little adjustment to apply this bugfix on prior versions.
It seems to me a waste of time for committers.
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 11/01/15 08:56, Kohei KaiGai wrote:
2015-01-11 10:40 GMT+09:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
Yeah there are actually several places in the code where "relid" means
index in range table and not oid of relation, it still manages to
confuse
me. Nothing this patch can do about that.Well, since it's confused 3 of us now... should we change it (as a
separate patch)? I'm willing to do that work but don't want to waste
time if
it'll just be rejected.Any other examples of this I should fix too?
Sorry, to clarify... any other items besides Scan.scanrelid that I should
fix?This naming is a little bit confusing, however, I don't think it "should"
be
changed because this structure has been used for a long time, so reworking
will prevent back-patching when we find bugs around "scanrelid".We can still backpatch; it just requires more work. And how many bugs do we
actually expect to find around this anyway?If folks think this just isn't worth fixing fine, but I find the
backpatching argument rather dubious.Even though here is no problem around Scan structure itself, a bugfix may
use the variable name of "scanrelid" to fix it. If we renamed it on v9.5, we
also need a little adjustment to apply this bugfix on prior versions.
It seems to me a waste of time for committers.
I tend to agree, especially as there is multiple places in code this
would affect - RelOptInfo and RestrictInfo have same issue, etc.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Petr Jelinek <petr@2ndquadrant.com> writes:
On 11/01/15 08:56, Kohei KaiGai wrote:
2015-01-11 10:40 GMT+09:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
Yeah there are actually several places in the code where "relid" means
index in range table and not oid of relation, it still manages to
confuse me. Nothing this patch can do about that.
Well, since it's confused 3 of us now... should we change it (as a
separate patch)? I'm willing to do that work but don't want to waste
time if it'll just be rejected.
It seems to me a waste of time for committers.
I tend to agree, especially as there is multiple places in code this
would affect - RelOptInfo and RestrictInfo have same issue, etc.
Generally speaking, if you're not sure whether a "relid" variable in the
planner is meant to be a table OID or a rangetable index, you can tell by
noting whether it's declared as type Oid or type int. So I'm also -1 on
any wholesale renaming, especially given the complete lack of an obviously
superior naming convention to change to.
If there are any places where such variables are improperly declared, then
of course we ought to fix that.
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
On Fri, Jan 9, 2015 at 10:51 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
When custom-scan node replaced a join-plan, it shall have at least two
child plan-nodes. The callback handler of PlanCustomPath needs to be
able to call create_plan_recurse() to transform the underlying path-nodes
to plan-nodes, because this custom-scan node may take other built-in
scan or sub-join nodes as its inner/outer input.
In case of FDW, it shall kick any underlying scan relations to remote
side, thus we may not expect ForeignScan has underlying plans...
Do you have an example of this?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jan 9, 2015 at 10:51 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
When custom-scan node replaced a join-plan, it shall have at least two
child plan-nodes. The callback handler of PlanCustomPath needs to be
able to call create_plan_recurse() to transform the underlying
path-nodes to plan-nodes, because this custom-scan node may take other
built-in scan or sub-join nodes as its inner/outer input.
In case of FDW, it shall kick any underlying scan relations to remote
side, thus we may not expect ForeignScan has underlying plans...Do you have an example of this?
Yes, even though full code set is too large for patch submission...
https://github.com/pg-strom/devel/blob/master/src/gpuhashjoin.c#L1880
This create_gpuhashjoin_plan() is PlanCustomPath callback of GpuHashJoin.
It takes GpuHashJoinPath inherited from CustomPath that has multiple
underlying scan/join paths.
Once it is called back from the backend, it also calls create_plan_recurse()
to make inner/outer plan nodes according to the paths.
In the result, we can see the following query execution plan that CustomScan
takes underlying scan plans.
postgres=# EXPLAIN SELECT * FROM t0 NATURAL JOIN t1 NATURAL JOIN t2;
QUERY PLAN
----------------------------------------------------------------------------------
Custom Scan (GpuHashJoin) (cost=2968.00..140120.31 rows=3970922 width=143)
Hash clause 1: (aid = aid)
Hash clause 2: (bid = bid)
Bulkload: On
-> Custom Scan (GpuScan) on t0 (cost=500.00..57643.00 rows=4000009 width=77)
-> Custom Scan (MultiHash) (cost=734.00..734.00 rows=40000 width=37)
hash keys: aid
nBatches: 1 Buckets: 46000 Memory Usage: 99.99%
-> Seq Scan on t1 (cost=0.00..734.00 rows=40000 width=37)
-> Custom Scan (MultiHash) (cost=734.00..734.00 rows=40000 width=37)
hash keys: bid
nBatches: 1 Buckets: 46000 Memory Usage: 49.99%
-> Seq Scan on t2 (cost=0.00..734.00 rows=40000 width=37)
(13 rows)
Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>
-----Original Message-----
From: Robert Haas [mailto:robertmhaas@gmail.com]
Sent: Thursday, January 15, 2015 2:07 AM
To: Kaigai Kouhei(海外 浩平)
Cc: Tom Lane; pgsql-hackers@postgreSQL.org; Shigeru Hanada
Subject: ##freemail## Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5]
Custom Plan API)On Fri, Jan 9, 2015 at 10:51 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
When custom-scan node replaced a join-plan, it shall have at least two
child plan-nodes. The callback handler of PlanCustomPath needs to be
able to call create_plan_recurse() to transform the underlying
path-nodes to plan-nodes, because this custom-scan node may take other
built-in scan or sub-join nodes as its inner/outer input.
In case of FDW, it shall kick any underlying scan relations to remote
side, thus we may not expect ForeignScan has underlying plans...Do you have an example of this?
--
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
Import Notes
Resolved by subject fallback
On Thu, Jan 15, 2015 at 8:02 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
On Fri, Jan 9, 2015 at 10:51 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com>
wrote:
When custom-scan node replaced a join-plan, it shall have at least two
child plan-nodes. The callback handler of PlanCustomPath needs to be
able to call create_plan_recurse() to transform the underlying
path-nodes to plan-nodes, because this custom-scan node may take other
built-in scan or sub-join nodes as its inner/outer input.
In case of FDW, it shall kick any underlying scan relations to remote
side, thus we may not expect ForeignScan has underlying plans...Do you have an example of this?
Yes, even though full code set is too large for patch submission...
https://github.com/pg-strom/devel/blob/master/src/gpuhashjoin.c#L1880
This create_gpuhashjoin_plan() is PlanCustomPath callback of GpuHashJoin.
It takes GpuHashJoinPath inherited from CustomPath that has multiple
underlying scan/join paths.
Once it is called back from the backend, it also calls
create_plan_recurse()
to make inner/outer plan nodes according to the paths.In the result, we can see the following query execution plan that
CustomScan
takes underlying scan plans.postgres=# EXPLAIN SELECT * FROM t0 NATURAL JOIN t1 NATURAL JOIN t2;
QUERY PLAN----------------------------------------------------------------------------------
Custom Scan (GpuHashJoin) (cost=2968.00..140120.31 rows=3970922
width=143)
Hash clause 1: (aid = aid)
Hash clause 2: (bid = bid)
Bulkload: On
-> Custom Scan (GpuScan) on t0 (cost=500.00..57643.00 rows=4000009
width=77)
-> Custom Scan (MultiHash) (cost=734.00..734.00 rows=40000 width=37)
hash keys: aid
nBatches: 1 Buckets: 46000 Memory Usage: 99.99%
-> Seq Scan on t1 (cost=0.00..734.00 rows=40000 width=37)
-> Custom Scan (MultiHash) (cost=734.00..734.00 rows=40000
width=37)
hash keys: bid
nBatches: 1 Buckets: 46000 Memory Usage: 49.99%
-> Seq Scan on t2 (cost=0.00..734.00 rows=40000 width=37)
(13 rows)
Where are we on this? AFAIK, we have now a feature with no documentation
and no example in-core to test those custom routine APIs, hence moved to
next CF.
--
Michael
