> On Thu, Nov 20, 2014 at 7:10 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > > I've done some preliminary cleanup on this patch, but I'm still pretty > > desperately unhappy about some aspects of it, in particular the way > > that it gets custom scan providers directly involved in setrefs.c, > > finalize_primnode, and replace_nestloop_params processing. I don't > > want any of that stuff exported outside the core, as freezing those > > APIs would be a very nasty restriction on future planner development. > > What's more, it doesn't seem like doing that creates any value for > > custom-scan providers, only a requirement for extra boilerplate code > > for them to provide. > > > > ISTM that we could avoid that by borrowing the design used for FDW > > plans, namely that any expressions you would like planner > > post-processing services for should be stuck into a predefined List > > field (fdw_exprs for the ForeignScan case, perhaps custom_exprs for the > CustomScan case?). > > This would let us get rid of the SetCustomScanRef and > > FinalizeCustomScan callbacks as well as simplify the API contract for > PlanCustomPath. > > Ah, that makes sense. I'm not sure I really understand what's so bad about > the current system, but I have no issue with revising it for consistency. > > > I'm also wondering why this patch didn't follow the FDW lead in terms > > of expecting private data to be linked from specialized "private" fields. > > The design as it stands (with an expectation that CustomPaths, > > CustomPlans etc would be larger than the core code knows about) is not > > awful, but it seems just randomly different from the FDW precedent, > > and I don't see a good argument why it should be. If we undid that we > > could get rid of CopyCustomScan callbacks, and perhaps also > > TextOutCustomPath and TextOutCustomScan (though I concede there might > > be some argument to keep the latter two anyway for debugging reasons). > > OK. > So, the existing form shall be revised as follows?
* CustomScan shall not be a base type of custom data-type managed by extension, instead of private data field. * It also eliminates CopyCustomScan and TextOutCustomPath/Scan callback. * Expression nodes that will not be processed by core backend, but processed by extension shall be connected to special field, like fdw_exprs in FDW. * Translation between a pseudo varnode and original expression node shall be informed to the core backend, instead of SetCustomScanRef and GetSpecialCustomVar. > > Lastly, I'm pretty unconvinced that the GetSpecialCustomVar mechanism > > added to ruleutils.c is anything but dead weight that we'll have to > > maintain forever. It seems somewhat unlikely that anyone will figure > > out how to use it, or indeed that it can be used for anything very > > interesting. I suppose the argument for it is that you could stick > > "custom vars" into the tlist of a CustomScan plan node, but you > > cannot, at least not without a bunch of infrastructure that isn't > > there now; in particular how would such an expression ever get matched > > by setrefs.c to higher-level plan tlists? I think we should rip that > > out and wait to see a complete use-case before considering putting it > back. > > I thought this was driven by a suggestion from you, but maybe KaiGai can > comment. > > > PS: with no documentation it's arguable that the entire patch is just > > dead weight. I'm not very happy that it went in without any. > > As I said, I wasn't sure we wanted to commit to the API enough to document > it, and by the time you get done whacking the stuff above around, the > documentation KaiGai wrote for it (which was also badly in need of editing > by a native English speaker) would have been mostly obsolete anyway. But > I'm willing to put some effort into it once you get done rearranging the > furniture, if that's helpful. > For people's convenient, I'd like to set up a wikipage to write up a draft of SGML documentation for easy updates by native English speakers. Thanks, -- NEC OSS Promotion Center / PG-Strom Project KaiGai Kohei <kai...@ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
