Re: [v9.5] Custom Plan API

On Sun, Aug 31, 2014 at 12:54 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> 2014-08-29 13:33 GMT-04:00 Robert Haas <robertmhaas@gmail.com>:
>> On Wed, Aug 27, 2014 at 6:51 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
>>>> > I'd like to follow this direction, and start stripping the DDL support.
>>>>
>>>> ...please make it so.
>>>>
>>> The attached patch eliminates DDL support.
>>>
>>> Instead of the new CREATE CUSTOM PLAN PROVIDER statement,
>>> it adds an internal function; register_custom_scan_provider
>>> that takes custom plan provider name and callback function
>>> to add alternative scan path (should have a form of CustomPath)
>>> during the query planner is finding out the cheapest path to
>>> scan the target relation.
>>> Also, documentation stuff is revised according to the latest
>>> design.
>>> Any other stuff keeps the previous design.
>>
>> Comments:
>>
>> 1. There seems to be no reason for custom plan nodes to have MultiExec
>> support; I think this as an area where extensibility is extremely
>> unlikely to work out.  The MultiExec mechanism is really only viable
>> between closely-cooperating nodes, like Hash and HashJoin, or
>> BitmapIndexScan, BitmapAnd, BitmapOr, and BitmapHeapScan; and arguably
>> those things could have been written as a single, more complex node.
>> Are we really going to want to support a custom plan that can
>> substitute for a Hash or BitmapAnd node?  I really doubt that's very
>> useful.
>>
> This intends to allows a particular custom-scan provider to exchange
> its internal data when multiple custom-scan node is stacked.
> So, it can be considered a facility to implement closely-cooperating nodes;
> both of them are managed by same custom-scan provider.
> An example is gpu-accelerated version of hash-join that takes underlying
> custom-scan node that will returns a hash table with gpu preferable data
> structure, but should not be a part of row-by-row interface.
> I believe it is valuable for some use cases, even though I couldn't find
> a use-case in ctidscan example.

Color me skeptical.  Please remove that part for now, and we can
revisit it when, and if, a plausible use case emerges.

>> 3. Is it really a good idea to invoke custom scan providers for RTEs
>> of every type?  It's pretty hard to imagine that a custom scan
>> provider can do anything useful with, say, RTE_VALUES.  Maybe an
>> accelerated scan of RTE_CTE or RTE_SUBQUERY is practical somehow, but
>> even that feels like an awfully big stretch.  At least until clear use
>> cases emerge, I'd be inclined to restrict this to RTE_RELATION scans
>> where rte->relkind != RELKIND_FOREIGN_TABLE; that is, put the logic in
>> set_plain_rel_pathlist() rather than set_rel_pathlist().
>>
> I'd like to agree. Indeed, it's not easy to assume a use case of
> custom-logic for non-plain relations.
>
>> (We might even want to consider whether the hook in
>> set_plain_rel_pathlist() ought to be allowed to inject a non-custom
>> plan; e.g. substitute a scan of relation B for a scan of relation A.
>> For example, imagine that B contains all rows from A that satisfy some
>> predicate. This could even be useful for foreign tables; e.g.
>> substitute a scan of a local copy of a foreign table for a reference
>> to that table.  But I put all of these ideas in parentheses because
>> they're only good ideas to the extent that they don't sidetrack us too
>> much.)
>>
> Hmm... It seems to me we need another infrastructure to take
> a substitute scan, because add_path() is called towards a certain
> RelOpInfo that is associated with the relation A.
> As long as custom-scan provider "internally" redirect a request for
> scan of A by substitute scan B (with taking care of all other stuff
> like relation locks), I don't think we need to put some other hooks
> outside from the set_plain_rel_pathlist().

OK, I see.  So this would have to be implemented as some new kind of
path anyway.  It might be worth allowing custom paths for scanning a
foreign table as well as a plain table, though - so any RTE_RELATION
but not other types of RTE.

> It came from the discussion I had long time before during patch
> reviewing of postgres_fdw. I suggested to use static table of
> FdwRoutine but I got a point that says some compiler raise
> error/warning to put function pointers on static initialization.
> I usually use GCC only, so I'm not sure whether this argue is
> right or not, even though the postgres_fdw_handler() allocates
> FdwRoutine using palloc() then put function pointers for each.

That's odd, because aset.c has used static initializers since forever,
and I'm sure someone would have complained by now if there were a
problem with that usage.

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