Re: WIP: Upper planner pathification

Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> On Wed, Mar 16, 2016 at 2:47 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I do not, however, like the proposal to expose wflists and so forth.
>> Those are internal data structures in grouping_planner and I absolutely
>> refuse to promise that they're going to stay stable.

> Hmm... It's not easy to imagine a case that extension wants own idea
> to extract window functions from the target list and select active
> windows, even if extension wants to have own executor and own cost
> estimation logic.
> In case when extension tries to add WindowPath + CustomPath(Sort),
> extension is interested in alternative sort task, but not window
> function itself. It is natural to follow the built-in implementation,
> thus, it motivates extension author to take copy & paste the code.
> select_active_windows() is static, so extension needs to have same
> routine on their side.

Well, to be perfectly blunt about it, I have said from day one that this
notion that a CustomScan extension will be able to cause arbitrary planner
behavior changes is loony. We are simply not going to drop a hook into
every tenth line of the planner for you, nor de-static-ify every internal
function, nor (almost equivalently) expose the data structures those
functions produce, because it would cripple core planner development to
try to keep the implied APIs stable. And I continue to maintain that any
actually-generally-useful ideas would be better handled by submitting them
as patches to the core planner, rather than trying to implement them in an
arms-length extension.

In the case at hand, I notice that the WindowFuncLists struct is
actually from find_window_functions in clauses.c, so an extension
that needed to get hold of that would be unlikely to do any copying
and pasting anyhow -- it'd just call find_window_functions again.
(That only needs to search the targetlist, so it's not that expensive.)
The other lists you mention are all tightly tied to specific, and not
terribly well-designed, implementation strategies for grouping sets and
window functions. Those are *very* likely to change in the near future;
and even if they don't, I'm skeptical that an external implementor of
grouping sets or window functions would want to use exactly those same
implementation strategies. Maybe the only reason you're there at all
is you want to be smarter about the order of doing window functions,
for example.

So I really don't want to export any of that stuff.

As for other details of the proposed patch, I was intending to put
all the hook calls into grouping_planner for consistency, rather than
scattering them between grouping_planner and its subroutines. So that
would probably mean calling the hook for a given step *before* we
generate the core paths for that step, not after. Did you have a
reason to want the other order? (If you say "so the hook can look
at the core-made paths", I'm going to say "that's a bad idea". It'd
further increase the coupling between a CustomScan extension and core.)

regards, tom lane