Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API)

> On Tue, Nov 25, 2014 at 3:44 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)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-scan
> below.
> >
> > 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 instead
> of 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 for
> both 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 a
> particular 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(at)ak(dot)jp(dot)nec(dot)com>

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(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Kouhei Kaigai
> Sent: Wednesday, December 03, 2014 3:11 PM
> To: Robert Haas
> Cc: Tom Lane; pgsql-hackers(at)postgreSQL(dot)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(at)ak(dot)jp(dot)nec(dot)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-scan
> > below.
> > >
> > > 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 instead
> > of 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
> > > for
> > both 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 a
> > particular 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(at)ak(dot)jp(dot)nec(dot)com>

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

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

> > 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(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: Petr Jelinek [mailto:petr(at)2ndquadrant(dot)com]
> Sent: Wednesday, January 07, 2015 8:24 AM
> To: Jim Nasby; Kaigai Kouhei(海外 浩平); Robert Haas
> Cc: Tom Lane; pgsql-hackers(at)postgreSQL(dot)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

On Tue, Jan 6, 2015 at 9:17 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)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

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

2015-01-10 8:18 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(at)kaigai(dot)gr(dot)jp>

On 10/01/15 01:19, Kohei KaiGai wrote:
> 2015-01-10 8:18 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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

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

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

2015-01-10 9:56 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(at)kaigai(dot)gr(dot)jp>

On 1/9/15, 8:51 PM, Kohei KaiGai wrote:
> 2015-01-10 9:56 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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

2015-01-11 10:40 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>:
> On 1/9/15, 8:51 PM, Kohei KaiGai wrote:
>>
>> 2015-01-10 9:56 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(at)kaigai(dot)gr(dot)jp>

On 11/01/15 08:56, Kohei KaiGai wrote:
> 2015-01-11 10:40 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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

Petr Jelinek <petr(at)2ndquadrant(dot)com> writes:
> On 11/01/15 08:56, Kohei KaiGai wrote:
>> 2015-01-11 10:40 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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