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.

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).

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.

Comments?
        regards, tom lane

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.