Re: query_planner() API change

Atri Sharma <atri(dot)jiit(at)gmail(dot)com> writes:
>> While we could complicate query_planner()'s API even more to add some
>> understanding of unnecessary resjunk items, I think this is probably
>> the straw that breaks the camel's back for the current approach here.
>> There is already a comment like this in query_planner():
>>
>> * This introduces some undesirable coupling between this code and
>> * grouping_planner, but the alternatives seem even uglier; we couldn't
>> * pass back completed paths without making these decisions here.

> I agree with the idea,but am trying to understand why adding
> understanding of resjunk columns is a bad idea. Just for understanding
> purpose, could you please elaborate a bit on it?

It's just that doing it that way would require making both planner.c and
planmain.c intimately involved in the decision about whether suppressing
resjunk ORDER BY targets is a win. Really, anything to do with
ordering/grouping implementation decisions is grouping_planner's business.
So putting chunks of that logic in a completely different file doesn't
seem like a great design, especially not if it requires weighing down
query_planner()'s API even more. query_planner should only be concerned
with scan/join planning.

Basically, we'd be moving knowledge of how to dig the best paths out of a
RelOptInfo from query_planner to grouping_planner --- which when you think
about it seems like mostly a wash from a modularity standpoint, anyway.
Having done that, we can get query_planner's fingers out of a number of
issues that are really grouping_planner's business. Returning the
RelOptInfo also eliminates the baked-into-the-API assumption that only one
of the presorted path(s) could be of interest to grouping_planner, which
is something I've long suspected would become a problem someday.

On balance I'm feeling like this is a win even without considering the
proposed changes for resjunk targets.

regards, tom lane