Re: [v9.5] Custom Plan API

> On Thu, Nov 20, 2014 at 7:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 <kaigai(at)ak(dot)jp(dot)nec(dot)com>