Re: Custom Scan APIs (Re: Custom Plan node) - Mailing list pgsql-hackers

2014-02-25 20:32 GMT+09:00 Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>:
>
> On Tue, Feb 25, 2014 at 3:39 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
>>
>> > Sorry for jumping into this late.
>> >
>> > Instead of custom node, it might be better idea to improve FDW
>> > infrastructure
>> > to push join. For the starters, is it possible for the custom scan node
>> > hooks to create a ForeignScan node? In general, I think, it might be
>> > better
>> > for the custom scan hooks to create existing nodes if they serve the
>> > purpose.
>> >
>> It does not work well because existing FDW infrastructure is designed to
>> perform on foreign tables, not regular tables. Probably, it needs to
>> revise
>> much our assumption around the background code, if we re-define the
>> purpose
>> of FDW infrastructure. For example, ForeignScan is expected to return a
>> tuple
>> according to the TupleDesc that is exactly same with table definition.
>> It does not fit the requirement if we replace a join-node by ForeignScan
>> because its TupleDesc of joined relations is not predefined.
>>
>
> If one wants to push joins, aggregates, grouping across to other data
> sources capable of handling them, that will need to change. But, at the same
> time, letting custom scan node being able to decide that doesn't seem to be
> a very good idea. Although, through custom scan nodes, we can see the
> potential in adding these features.
>
Of course, existing form of custom-scan node is designed to support
scan or join relations, as a first step. It will also need some enhancement
to support other class of execution node in the future version.
I'm not certain why it is problematic.

>> I'd like to define these features are designed for individual purpose.
>> FDW is designed to intermediate an external data source and internal heap
>> representation according to foreign table definition. In other words, its
>> role is to generate contents of predefined database object on the fly.
>> On the other hands, custom-scan is designed to implement alternative ways
>> to scan / join relations in addition to the methods supported by built-in
>> feature.
>>
>> I'm motivated to implement GPU acceleration feature that works
>> transparently
>> for application. Thus, it has to be capable on regular tables, because
>> most
>> of application stores data on regular tables, not foreign ones.
>>
>
> It looks like my description was misleading. In some cases, it might be
> possible that the ultimate functionality that a particular instantiation of
> custom node scan is already available as a Plan node in PG, but PG optimizer
> is not able to optimize the operation that way. In such case, custom scan
> node infrastructure should produce the corresponding Path node and not
> implement that functionality itself.
>
You are suggesting that CustomSort, CustomAgg, CustomAppend and
so on should be supported in the future version, for better integration with
the plan optimizer. Right?
It is probably a good idea if optimizer needs to identify CustomXXXX node
using node tag, rather than something others like custom-scan provider
name,
Right now, custom-scan feature focuses on optimization of relation scan
and join as its first scope, and does not need to identify the class of
corresponding Path node.
On the upthread of this discussion, I initially proposed to have separated
CustomScan and CustomJoin node, however, our consensus was that
CustomScan can perform as like a scan on the result set of joined
relations, so I dropped multiple node types from the first version.

>> > Since a custom node is open implementation, it will be important to pass
>> > as much information down to the hooks as possible; lest the hooks will
>> > be
>> > constrained.  Since the functions signatures within the planner,
>> > optimizer
>> > will change from time to time, so the custom node hook signatures will
>> > need
>> > to change from time to time. That might turn out to be maintenance
>> > overhead.
>> >
>> Yes. You are also right. But it also makes maintenance overhead if hook
>> has
>> many arguments nobody uses.
>> Probably, it makes sense to list up the arguments that cannot be
>> reproduced
>> from other information, can be reproduced but complicated steps, and can
>> be
>> reproduced easily.
>>
>> Below is the information we cannot reproduce:
>>  - PlannerInfo *root
>>  - RelOptInfo *joinrel
>>  - RelOptInfo *outerrel
>>  - RelOptInfo *innerrel
>>  - JoinType jointype
>>  - SpecialJoinInfo *sjinfo
>>  - List *restrictlist
>>
>
> Most of this information is available through corresponding RelOptInfo, or
> we should make RelOptInfo contain all the information related to every
> relation required to be computed during the query. So, any function which
> creates paths can just take that RelOptInfo as an argument and produce the
> path/s. That way there is lesser chance that the function signatures change.
>
Uhmm.... It is inconvenience to write extensions. I want the variables
in the first and second groups being delivered to the hook, even though
it may have minor modification in the future release.
Relations join is one of the heart of RDBMS, so I'd like to believe these
arguments are one of the most stable stuffs.

>> Below is the information we can reproduce but complicated steps:
>>  - List *mergeclause_list
>>  - bool mergejoin_allow
>>  - Relids param_source_rels
>>  - Relids extra_lateral_rels
>>
>> Below is the information we can reproduce easily:
>>  - SemiAntiJoinFactors *semifactors
>>
>> I think, the first two categories or the first category (if functions to
>> reproduce the second group is exposed) should be informed to extension,
>> however, priority of the third group is not high.
>>
>>
>> > BTW, is it a good idea for custom nodes to also affect other paths like
>> > append, group etc.? Will it need separate hooks for each of those?
>> >
>> Yes. I plan to support above plan node, in addition to scan / join only.
>> The custom-scan node is thin abstraction towards general executor
>> behavior,
>> so I believe it is not hard to enhance this node, without new plan node
>> for each of them.
>> Of course, it will need separate hook to add alternative path on the
>> planner
>> stage, but no individual plan nodes. (Sorry, it was unclear for me what
>> does the "hook" mean.)
>>
>
> If we represent all the operation like grouping, sorting, aggregation, as
> some sort of relation, we can create paths for each of the relation like we
> do (I am heavily borrowing from Tom's idea of pathifying those operations).
> We will need much lesser hooks in custom scan node.
>
> BTW, from the patch, I do not see this change to be light weight. I was
> expecting more of a list of hooks to be defined by the user and this
> infrastructure just calling them at appropriate places.
>
Let's focus on scan and join that we are currently working on.
Even if we need separate node type for grouping or sorting, it will not
be necessary to construct whole of the framework from the scratch.
For example, definition of CustomProvider table will be able to reuse
for other class of operations, because most of them are thin abstraction
of existing executor's interface.

Thanks,

>> Thanks,
>> --
>> NEC OSS Promotion Center / PG-Strom Project
>> KaiGai Kohei <kaigai@ak.jp.nec.com>
>>
>>
>> > -----Original Message-----
>> > From: Ashutosh Bapat [mailto:ashutosh.bapat@enterprisedb.com]
>> > Sent: Tuesday, February 25, 2014 5:59 PM
>> > To: Kohei KaiGai
>> > Cc: Kaigai, Kouhei(海外, 浩平); Stephen Frost; Shigeru Hanada; Jim
>> > Mlodgenski; Robert Haas; Tom Lane; PgHacker; Peter Eisentraut
>> > Subject: Re: Custom Scan APIs (Re: [HACKERS] Custom Plan node)
>> >
>> >
>> >
>> >
>> > On Sun, Feb 23, 2014 at 6:54 PM, Kohei KaiGai <kaigai@kaigai.gr.jp>
>> > wrote:
>> >
>> >
>> >       Folks,
>> >
>> >       Let me remind the custom-scan patches; that is a basis feature of
>> >       remote join of postgres_fdw, cache-only scan, (upcoming) GPU
>> >       acceleration feature or various alternative ways to scan/join
>> > relations.
>> >       Unfortunately, small amount of discussion we could have in this
>> > commit
>> >       fest, even though Hanada-san volunteered to move the patches into
>> >       "ready for committer" state at the CF-Nov.
>> >
>> >
>> >
>> > Sorry for jumping into this late.
>> >
>> > Instead of custom node, it might be better idea to improve FDW
>> > infrastructure
>> > to push join. For the starters, is it possible for the custom scan node
>> > hooks to create a ForeignScan node? In general, I think, it might be
>> > better
>> > for the custom scan hooks to create existing nodes if they serve the
>> > purpose.
>> >
>> >
>> >
>> >
>> >       Prior to time-up, I'd like to ask hacker's opinion about its
>> > potential
>> >       arguable points (from my standpoint) if it needs to be fixed up.
>> >       One is hook definition to add alternative join path, and the other
>> > one
>> >       is a special varno when a custom scan replace a join node.
>> >       I'd like to see your opinion about them while we still have to
>> > change
>> >       the design if needed.
>> >
>> >       (1) Interface to add alternative paths in addition to built-in
>> > join
>> > paths
>> >
>> >
>> >       This patch adds "add_join_path_hook" on add_paths_to_joinrel to
>> > allow
>> >       extensions to provide alternative scan path in addition to the
>> > built-in
>> >       join paths. Custom-scan path being added is assumed to perform to
>> > scan
>> >       on a (virtual) relation that is a result set of joining relations.
>> >       My concern is its arguments to be pushed. This hook is declared
>> > as follows:
>> >
>> >       /* Hook for plugins to add custom join path, in addition to
>> > default
>> > ones */
>> >       typedef void (*add_join_path_hook_type)(PlannerInfo *root,
>> >                                               RelOptInfo *joinrel,
>> >                                               RelOptInfo *outerrel,
>> >                                               RelOptInfo *innerrel,
>> >                                               JoinType jointype,
>> >                                               SpecialJoinInfo
>> > *sjinfo,
>> >                                               List *restrictlist,
>> >                                               List *mergeclause_list,
>> >                                               SemiAntiJoinFactors
>> > *semifactors,
>> >                                               Relids
>> > param_source_rels,
>> >                                               Relids
>> > extra_lateral_rels);
>> >       extern PGDLLIMPORT add_join_path_hook_type add_join_path_hook;
>> >
>> >       Likely, its arguments upper than restrictlist should be informed
>> > to extensions,
>> >       because these are also arguments of add_paths_to_joinrel().
>> >       However, I'm not 100% certain how about other arguments should be
>> > informed.
>> >       Probably, it makes sense to inform param_source_rels and
>> > extra_lateral_rels
>> >       to check whether the path is sensible for parameterized paths.
>> >       On the other hand, I doubt whether mergeclause_list is usuful to
>> > deliver.
>> >       (It may make sense if someone tries to implement their own
>> > merge-join
>> >       implementation??)
>> >
>> >       I'd like to seem idea to improve the current interface
>> > specification.
>> >
>> >
>> >
>> >
>> > Since a custom node is open implementation, it will be important to pass
>> > as much information down to the hooks as possible; lest the hooks will
>> > be
>> > constrained.  Since the functions signatures within the planner,
>> > optimizer
>> > will change from time to time, so the custom node hook signatures will
>> > need
>> > to change from time to time. That might turn out to be maintenance
>> > overhead.
>> >
>> >
>> > BTW, is it a good idea for custom nodes to also affect other paths like
>> > append, group etc.? Will it need separate hooks for each of those?
>> >
>> >
>> >
>> >
>> >       (2) CUSTOM_VAR for special Var reference
>> >
>> >       @@ -134,6 +134,7 @@ typedef struct Expr
>> >        #define    INNER_VAR       65000       /* reference to inner
>> > subplan */
>> >        #define    OUTER_VAR       65001       /* reference to outer
>> > subplan */
>> >        #define    INDEX_VAR       65002       /* reference to index
>> > column */
>> >       +#define    CUSTOM_VAR      65003       /* reference to custom
>> > column */
>> >
>> >       I newly added CUSTOM_VAR to handle a case when custom-scan
>> > override
>> >       join relations.
>> >       Var-nodes within join plan are adjusted to refer either
>> > ecxt_innertuple or
>> >       ecxt_outertuple of ExprContext. It makes a trouble if custom-scan
>> > runs
>> >       instead of built-in joins, because its tuples being fetched are
>> > usually
>> >       stored on the ecxt_scantuple, thus Var-nodes also need to have
>> > right
>> >       varno neither inner nor outer.
>> >
>> >       SetPlanRefCustomScan callback, being kicked on set_plan_refs,
>> > allows
>> >       extensions to rewrite Var-nodes within custom-scan node to
>> > indicate
>> >       ecxt_scantuple using CUSTOM_VAR, instead of inner or outer.
>> >       For example, a var-node with varno=CUSTOM_VAR and varattno=3 means
>> >       this node reference the third attribute of the tuple in
>> > ecxt_scantuple.
>> >       I think it is a reasonable solution, however, I'm not 100% certain
>> >       whether people have more graceful idea to implement it.
>> >
>> >       If you have comments around above two topic, or others, please
>> > give
>> >       your ideas.
>> >
>> >       Thanks,
>> >
>> >
>> >       2014-01-28 9:14 GMT+09:00 Kouhei Kaigai <kaigai@ak.jp.nec.com>:
>> >
>> >       > Hi Stephen,
>> >       >
>> >       > Thanks for your comments.
>> >       >
>> >       >> * Kouhei Kaigai (kaigai@ak.jp.nec.com) wrote:
>> >       >> > Is somebody available to volunteer to review the custom-scan
>> > patch?
>> >       >>
>> >       >> I looked through it a bit and my first take away from it was
>> > that the patches
>> >       >> to actually use the new hooks were also making more changes to
>> > the backend
>> >       >> code, leaving me with the impression that the proposed
>> > interface
>> > isn't
>> >       >> terribly stable.  Perhaps those changes should have just been
>> > in the first
>> >       >> patch, but they weren't and that certainly gave me pause.
>> >       >>
>> >       > Yes, the part-1 patch provides a set of interface portion to
>> > interact
>> >       > between the backend code and extension code. Rest of part-2 and
>> > part-3
>> >       > portions are contrib modules that implements its feature on top
>> > of
>> >       > custom-scan API.
>> >       >
>> >       >> I'm also not entirely convinced that this is the direction to
>> > go in when
>> >       >> it comes to pushing down joins to FDWs.  While that's certainly
>> > a goal that
>> >       >> I think we all share, this seems to be intending to add a
>> > completely different
>> >       >> feature which happens to be able to be used for that.  For
>> > FDWs,
>> > wouldn't
>> >       >> we only present the FDW with the paths where the foreign tables
>> > for that
>> >       >> FDW, or perhaps just a given foreign server, are being joined?
>> >       >>
>> >       > FDW's join pushing down is one of the valuable use-cases of this
>> > interface,
>> >       > but not all. As you might know, my motivation is to implement
>> > GPU acceleration
>> >       > feature on top of this interface, that offers alternative way
>> > to scan or join
>> >       > relations or potentially sort or aggregate.
>> >       > Probably, it is too stretch interpretation if we implement
>> > radix-sort on top
>> >       > of FDW. I'd like you to understand the part-3 patch (FDW's join
>> > pushing-down)
>> >       > is a demonstration of custom-scan interface for application, but
>> > not designed
>> >       > for a special purpose.
>> >       >
>> >       > Right now, I put all the logic to interact CSI and FDW driver
>> > on postgres_fdw
>> >       > side, it might be an idea to have common code (like a logic to
>> > check whether
>> >       > the both relations to be joined belongs to same foreign server)
>> > on the backend
>> >       > side as something like a gateway of them.
>> >       >
>> >       >
>> >       > As an aside, what should be the scope of FDW interface?
>> >       > In my understanding, it allows extension to implement
>> > "something"
>> > on behalf of
>> >       > a particular data structure being declared with CREATE FOREIGN
>> > TABLE.
>> >       > In other words, extension's responsibility is to generate a view
>> > of "something"
>> >       > according to PostgreSQL' internal data structure, instead of the
>> > object itself.
>> >       > On the other hands, custom-scan interface allows extensions to
>> > implement
>> >       > alternative methods to scan or join particular relations, but
>> > it is not a role
>> >       > to perform as a target being referenced in queries. In other
>> > words,
>> > it is methods
>> >       > to access objects.
>> >       > It is natural both features are similar because both of them
>> > intends extensions
>> >       > to hook the planner and executor, however, its purpose is
>> > different.
>> >       >
>> >       > Thanks,
>> >       > --
>> >       > NEC OSS Promotion Center / PG-Strom Project
>> >       > KaiGai Kohei <kaigai@ak.jp.nec.com>
>> >
>> >
>> >
>> >       --
>> >
>> >       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
>> >
>> >
>> >
>> >
>> >
>> >
>> > --
>> >
>> > Best Wishes,
>> > Ashutosh Bapat
>> > EnterpriseDB Corporation
>> > The Postgres Database Company
>>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company



-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>