On Wed, Aug 27, 2014 at 6:51 PM, Kouhei Kaigai <kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers