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

> Robert Haas <robertmhaas@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 <kaigai@ak.jp.nec.com>