Re: [v9.5] Custom Plan API - Mailing list pgsql-hackers

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.

2. This patch is still sort of on the fence about whether we're
implementing custom plans (of any type) or custom scans (thus, of some
particular relation).  I previously recommended that we confine
ourselves initially to the task of adding custom *scans* and leave the
question of other kinds of custom plan nodes to a future patch.  After
studying the latest patch, I'm inclined to suggest a slightly revised
strategy.  This patch is really adding THREE kinds of custom objects:
CustomPlanState, CustomPlan, and CustomPath. CustomPlanState inherits
from ScanState, so it is not really a generic CustomPlan, but
specifically a CustomScan; likewise, CustomPlan inherits from Scan,
and is therefore a CustomScan, not a CustomPlan.  But CustomPath is
different: it's just a Path.  Even if we only have the hooks to inject
CustomPaths that are effectively scans at this point, I think that
part of the infrastructure could be somewhat generic.  Perhaps
eventually we have CustomPath which can generate either CustomScan or
CustomJoin which in turn could generate CustomScanState and
CustomJoinState.

For now, I propose that we rename CustomPlan and CustomPlanState to
CustomScan and CustomScanState, because that's what they are; but that
we leave CustomPath as-is.  For ease of review, I also suggest
splitting this into a series of three patches: (1) add support for
CustomPath; (2) add support for CustomScan and CustomScanState; (3)
ctidscan.

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

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

4. Department of minor nitpicks.  You've got a random 'xs' in the
comments for ExecSupportsBackwardScan. And, in contrib/ctidscan,
ctidscan_path_methods, ctidscan_plan_methods, and
ctidscan_exec_methods can have static initializers; there's no need to
initialize them at run time in _PG_init().

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