> Robert Haas <robertmh...@gmail.com> writes: > > I've committed parts 1 and 2 of this, without the documentation, and > > with some additional cleanup. I am not sure that this feature is > > sufficiently non-experimental that it deserves to be documented, but > > if we're thinking of doing that then the documentation needs a lot > > more work. I think part 3 of the patch is mostly useful as a > > demonstration of how this API can be used, and is not something we > > probably want to commit. So I'm not planning, at this point, to spend > > any more time on this patch series, and will mark it Committed in the > > CF app. > > 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. > If core backend can know which field contains expression nodes but processed by custom-scan provider, FinalizedCustomScan might be able to rid. However, rid of SetCustomScanRef makes unavailable a significant use case I intend. In case when tlist contains complicated expression node (thus it takes many cpu cycles) and custom-scan provider has a capability to compute this expression node externally, SetCustomScanRef hook allows to replace this complicate expression node by a simple Var node that references a value being externally computed.
Because only custom-scan provider can know how this "pseudo" varnode is mapped to the original expression, it needs to call the hook to assign correct varno/varattno. We expect it assigns a special vano, like OUTER_VAR, and it is solved with GetSpecialCustomVar. One other idea is, core backend has a facility to translate relationship between the original expression and the pseudo varnode according to the map information given by custom-scan provider. > 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). > Yep, its original proposition last year followed the FDW manner. It has custom_private field to store the private data of custom-scan provider, however, I was suggested to change the current form because it added a couple of routines to encode / decode Bitmapset that may lead other encode / decode routines for other data types. I'm neutral for this design choice. Either of them people accept is better for me. > 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. > As I described above, as long as core backend has a facility to manage the relationship between a pseudo varnode and complicated expression node, I think we can rid this callback. > 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. > I agree with this. Is it a good to write up a wikipage to brush up the documentation draft? 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
