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

On Thu, Nov 20, 2014 at 7:10 PM, Tom Lane <tgl@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.

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

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company